Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Yarp types names #2231

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

[WIP] Yarp types names #2231

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1e84e33
wip-thrift-type
drdanz Mar 12, 2020
7628738
modified thrift yarp_generator to add name and version to yarp data t…
randaz81 Mar 13, 2020
08a3bdb
comments addressed
randaz81 Mar 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions src/idls/thrift/src/t_yarp_generator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,9 @@ class t_yarp_generator : public t_oop_generator
void generate_struct_read_connectionreader(t_struct* tstruct, std::ostringstream& f_h_, std::ostringstream& f_cpp_);
void generate_struct_write_wirereader(t_struct* tstruct, std::ostringstream& f_h_, std::ostringstream& f_cpp_);
void generate_struct_write_connectionreader(t_struct* tstruct, std::ostringstream& f_h_, std::ostringstream& f_cpp_);
void generate_struct_typeconstexpr(t_struct* tstruct, std::ostringstream& f_h_, const std::string& type, const std::string& version);
void generate_struct_tostring(t_struct* tstruct, std::ostringstream& f_h_, std::ostringstream& f_cpp_);
void generate_struct_gettype(t_struct* tstruct, std::ostringstream& f_h_, std::ostringstream& f_cpp_);
void generate_struct_unwrapped_helper(t_struct* tstruct, std::ostringstream& f_h_, std::ostringstream& f_cpp_);
void generate_struct_editor(t_struct* tstruct, std::ostringstream& f_h_, std::ostringstream& f_cpp_);
void generate_struct_editor_default_constructor(t_struct* tstruct, std::ostringstream& f_h_, std::ostringstream& f_cpp_);
Expand Down Expand Up @@ -1840,6 +1842,22 @@ void t_yarp_generator::generate_struct(t_struct* tstruct)
yarp_api_keyword = annotations.at("yarp.api.keyword");
}

std::string yarp_type_name{};
if (annotations.find("yarp.type.name") != annotations.end()) {
yarp_type_name = annotations.at("yarp.type.name");
}
else {
yarp_type_name = "yarp/"+name;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced about this default value, I'd rather enable this only if specified by the user (at least for now), or everyone who defined some type in some other repo will have some "yarp" types.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Better name? or just an empty string?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty string and do not add the type part if not defined

}

std::string yarp_type_version{};
if (annotations.find("yarp.type.version") != annotations.end()) {
yarp_type_version = annotations.at("yarp.type.version");
}
else {
yarp_type_version = "1.0";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same about the default version, 1.0 is not a good value, since an unstable type with no annotation defined will have a stable version. I'd stick with enabling this part only if explicitly added by the user.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0.0? or empty string?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I'd just leave it empty, and add the relative calls when generating the code only if it is defined

}

// Open header file
std::string f_header_name = get_out_dir() + get_include_prefix(program_) + name + ".h";
ofstream_with_content_based_conditional_update f_h_;
Expand Down Expand Up @@ -1874,6 +1892,7 @@ void t_yarp_generator::generate_struct(t_struct* tstruct)

f_h_ << "#include <yarp/os/Wire.h>\n";
f_h_ << "#include <yarp/os/idl/WireTypes.h>\n";
f_h_ << "#include <yarp/os/Type.h>\n";
if (need_common_) {
f_h_ << '\n';
f_h_ << "#include <" << get_include_prefix(program_) << program_->get_name() << "_common.h>" << '\n';
Expand Down Expand Up @@ -1915,7 +1934,9 @@ void t_yarp_generator::generate_struct(t_struct* tstruct)
generate_struct_read_connectionreader(tstruct, f_h_, f_cpp_);
generate_struct_write_wirereader(tstruct, f_h_, f_cpp_);
generate_struct_write_connectionreader(tstruct, f_h_, f_cpp_);
generate_struct_typeconstexpr(tstruct, f_h_, yarp_type_name, yarp_type_version);
generate_struct_tostring(tstruct, f_h_, f_cpp_);
generate_struct_gettype(tstruct, f_h_, f_cpp_);
generate_struct_unwrapped_helper(tstruct, f_h_, f_cpp_);

// Add editor class, if not disabled
Expand Down Expand Up @@ -2218,6 +2239,48 @@ void t_yarp_generator::generate_struct_tostring(t_struct* tstruct, std::ostrings
assert(indent_count_cpp() == 0);
}

void t_yarp_generator::generate_struct_typeconstexpr(t_struct* tstruct, std::ostringstream& f_h_, const std::string& yarp_type_name, const std::string& yarp_type_version)
{
THRIFT_DEBUG_COMMENT(f_h_);

const auto& name = tstruct->get_name();

f_h_ << indent_h() << "//The name and the version for this message\n";
f_h_ << indent_h() << "static constexpr const char* typeName = \"" << yarp_type_name << "\";\n";
f_h_ << indent_h() << "static constexpr const char* typeVersion = \"" << "1.0" << "\";\n";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.0 shouldn't be hardcoded here

f_h_ << '\n';

assert(indent_count_h() == 1);
}

void t_yarp_generator::generate_struct_gettype(t_struct* tstruct, std::ostringstream& f_h_, std::ostringstream& f_cpp_)
{
THRIFT_DEBUG_COMMENT(f_h_);
THRIFT_DEBUG_COMMENT(f_cpp_);

const auto& name = tstruct->get_name();

f_h_ << indent_h() << "// Get the message type\n";
f_h_ << indent_h() << "yarp::os::Type getType() const;\n";
f_h_ << '\n';

f_cpp_ << indent_cpp() << "// Convert to a printable string\n";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems a copy-paste from somewhere else

f_cpp_ << indent_cpp() << "yarp::os::Type " << name << "::getType() const\n";
f_cpp_ << indent_cpp() << "{\n";
indent_up_cpp();
{
f_cpp_ << indent_cpp() << " yarp::os::Type typ = yarp::os::Type::byNameOnWire(typeName);\n";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extra space before yarp::os::Type (and 2 lines below before return) shouldn't be there

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yarp::os::Type::byNameOnWire(typeName) is equivalent to yarp::os::Type::byName("yarp/bottle", typeName). We should definitely investigate what's the meaning of the first string, and if it is actually correct to use byNameOnWire, or it should be yarp::os::Type::byName(typeName, typeName)

f_cpp_ << indent_cpp() << "typ.setVersion(typeVersion);\n";
f_cpp_ << indent_cpp() << " return typ;\n";
}
indent_down_cpp();
f_cpp_ << indent_cpp() << "}\n";
f_cpp_ << '\n';

assert(indent_count_h() == 1);
assert(indent_count_cpp() == 0);
}

void t_yarp_generator::generate_struct_unwrapped_helper(t_struct* tstruct, std::ostringstream& f_h_, std::ostringstream& f_cpp_)
{
THRIFT_DEBUG_COMMENT(f_h_);
Expand Down
2 changes: 2 additions & 0 deletions src/libYARP_dev/src/idl/LaserScan2D.thrift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,4 +37,6 @@ struct LaserScan2D
(
yarp.api.include = "yarp/dev/api.h"
yarp.api.keyword = "YARP_dev_API"
yarp.type.name = "yarp/LaserScan2D"
yarp.type.version = "1.0"
)
9 changes: 9 additions & 0 deletions src/libYARP_dev/src/idl_generated_code/yarp/dev/LaserScan2D.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ bool LaserScan2D::read(yarp::os::ConnectionReader& connection)
// Write structure on a Wire
bool LaserScan2D::write(const yarp::os::idl::WireWriter& writer) const
{
//write_type();

if (!write_angle_min(writer)) {
return false;
}
Expand Down Expand Up @@ -121,6 +123,13 @@ std::string LaserScan2D::toString() const
return b.toString();
}

yarp::os::Type LaserScan2D::getType() const
{
yarp::os::Type typ = yarp::os::Type::byNameOnWire(typeName);
typ.setVersion(typeVersion);
return typ;
}

// Editor: default constructor
LaserScan2D::Editor::Editor()
{
Expand Down
7 changes: 7 additions & 0 deletions src/libYARP_dev/src/idl_generated_code/yarp/dev/LaserScan2D.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,13 @@ class YARP_dev_API LaserScan2D :
// If you want to serialize this class without nesting, use this helper
typedef yarp::os::idl::Unwrapped<LaserScan2D> unwrapped;


// The name for this message, ROS will need this
static constexpr const char* typeName = "yarp/LaserScan2D";
static constexpr const char* typeVersion = "1.0";

yarp::os::Type getType() const override;

class Editor :
public yarp::os::Wire,
public yarp::os::PortWriter
Expand Down
28 changes: 27 additions & 1 deletion src/libYARP_os/src/yarp/os/Type.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ class Type::Private
Property* prop{nullptr};
std::string name;
std::string name_on_wire;
std::string version;
};


Expand All @@ -108,6 +109,21 @@ Type::~Type()
delete mPriv;
}

std::string Type::getVersion() const
{
return mPriv->version;
}

size_t Type::getMajorVersion() const
{
return 1;
}

size_t Type::getMinorVersion() const
{
return 1;
}

Type& Type::operator=(const Type& rhs)
{
if (&rhs != this) {
Expand Down Expand Up @@ -154,9 +170,14 @@ bool Type::hasName() const
return !mPriv->name.empty();
}

bool Type::hasVersion() const
{
return !mPriv->version.empty();
}

bool Type::isValid() const
{
return hasName();
return hasName() && hasVersion();
}

std::string Type::toString() const
Expand All @@ -170,6 +191,11 @@ std::string Type::toString() const
return "null";
}

Type& Type::setVersion(const std::string& version)
{
mPriv->version = version;
return *this;
}

Type Type::byName(const char* name)
{
Expand Down
10 changes: 10 additions & 0 deletions src/libYARP_os/src/yarp/os/Type.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,16 @@ class YARP_os_API Type

std::string getNameOnWire() const;

std::string getVersion() const;

size_t getMajorVersion() const;

size_t getMinorVersion() const;

bool hasName() const;

bool hasVersion() const;

bool isValid() const;

std::string toString() const;
Expand All @@ -84,6 +92,8 @@ class YARP_os_API Type

Type& addProperty(const char* key, const Value& val);

Type& setVersion(const std::string& vesion);

/** @} */
/** @{ */

Expand Down