Conversation
rmw/include/rmw/rmw.h
Outdated
| * 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 |
There was a problem hiding this comment.
Could use the doxygen \sa to reference them.
| */ | ||
| RMW_PUBLIC | ||
| RMW_WARN_UNUSED | ||
| const char * |
There was a problem hiding this comment.
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.
|
I am not sure if "encoding" is the right term here. Isn't "serialization" more appropriate? E.g. |
|
Yeah I had the same concern. Especially as we might use the term "encoding" when talking about e.g. strings |
|
I'm fine with |
|
The CDR spec also uses the expression of a "CDR encoding", but I really don't care.
|
|
I'd be fine with |
|
CI ( win: |
|
I changed encoding to |
rmw/include/rmw/rmw.h
Outdated
| * 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 |
There was a problem hiding this comment.
Looks like this still needs to be addressed in a fixup.
wjwwood
left a comment
There was a problem hiding this comment.
Other than the lingering fixup, this lgtm.
|
CI: osx: |
|
@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. |
* API doc * use doxygen \sa * encoding -> serialization format * new lines in comment Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
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).