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.

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

get encoding identifier #143

Merged
merged 4 commits into from
Jul 27, 2018
Merged

get encoding identifier #143

merged 4 commits into from
Jul 27, 2018

Conversation

Karsten1987
Copy link
Contributor

With respect to the upcoming development for rosbags, we need to extract the encoding format for each RMW middleware.

The encoding identifier differs from the implementation identifier in the sense that each implementation has to be uniquely identified, but multiple RMW can use the same encoding (e.g. cdr for DDS implementations).

* In contrast to the implementation identifier, the encoding identifier can be
* equal between multiple RMW implementations. This means, that the same binary
* messages can be deserialized by RMW implementations with the same encoding ID.
* See also rmw_serialize, rmw_deserialize
Copy link
Member

Choose a reason for hiding this comment

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

Could use the doxygen \sa to reference them.

*/
RMW_PUBLIC
RMW_WARN_UNUSED
const char *
Copy link
Member

Choose a reason for hiding this comment

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

Is it ever going to be a performance problem to compare strings here? Unlike the implementation identifier we cannot just compare pointer values because the same encoding string might be defined in multiple places. Not sure what else we would do, maybe some kind of hash or a "registry" of encodings? Just a thought, not a blocker.

@dirk-thomas
Copy link
Member

I am not sure if "encoding" is the right term here. Isn't "serialization" more appropriate? E.g. CDR is a serialization format.

@mikaelarguedas
Copy link
Member

Yeah I had the same concern. Especially as we might use the term "encoding" when talking about e.g. strings

@wjwwood
Copy link
Member

wjwwood commented Jul 16, 2018

I'm fine with serialization_format instead. Some people (at least) seem to draw some difference between the two:

@Karsten1987
Copy link
Contributor Author

The CDR spec also uses the expression of a "CDR encoding", but I really don't care.
So what's the correct function name?

rmw_get_serialization_format_identifier ?

@wjwwood
Copy link
Member

wjwwood commented Jul 16, 2018

I'd be fine with rmw_get_serialization_format.

@Karsten1987
Copy link
Contributor Author

CI (--packages-up-to rmw rmw_implementation rmw_connext_cpp rmw_fastrtps_cpp rmw_opensplice_cpp)

win: Build Status
osx: Build Status
aarch: Build Status
linux: Build Status

@Karsten1987 Karsten1987 added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jul 17, 2018
@Karsten1987
Copy link
Contributor Author

I changed encoding to serialization_format as requested. Putting this in review.

* Return the format in which binary data is serialized.
* One middleware can only have one encoding.
* In contrast to the implementation identifier, the serialization format can be
* equal between multiple RMW implementations. This means, that the same binary
Copy link
Member

Choose a reason for hiding this comment

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

New sentence, new line.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this still needs to be addressed in a fixup.

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

Other than the lingering fixup, this lgtm.

@Karsten1987
Copy link
Contributor Author

CI:

osx: Build Status
linux: Build Status (unrelated)
win: Build Status

@Karsten1987 Karsten1987 merged commit ff8071e into master Jul 27, 2018
@Karsten1987 Karsten1987 deleted the get_encoding_identifier branch July 27, 2018 22:24
@Karsten1987 Karsten1987 removed the in review Waiting for review (Kanban column) label Jul 27, 2018
@dirk-thomas
Copy link
Member

dirk-thomas commented Jul 27, 2018

@ros2/rmw_implementations FYI these PRs added new API to the rmw interface. For FastRTPS, Connext and OpenSplice the referenced PRs implement the new API (returning "Cdr" as the format). Other RMW implementations should be updated to implement this new API in order to stay compatible.


If you are maintaining a RMW impl but are not part of the mentioned team please feel free to reach out to be added to the team. That way you can stay in the loop for future changes.

dabonnie pushed a commit to aws-ros-dev/rmw that referenced this pull request Apr 2, 2019
* API doc

* use doxygen \sa

* encoding -> serialization format

* new lines in comment

Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants