-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Message versioning and ROS 2 message translation #3465
base: main
Are you sure you want to change the base?
Conversation
a0c25d8
to
ac87603
Compare
ac87603
to
66ced5a
Compare
41873cf
to
825115e
Compare
3c869fb
to
2a3af80
Compare
2a3af80
to
22a480e
Compare
@hamishwillee I think the PR is in a review ready state. I left a few |
22a480e
to
8d080d2
Compare
en/middleware/uorb.md
Outdated
Message versioning was introduced to address the challenges of maintaining compatibility across systems where different versions of message definitions may be in use, such as between PX4 and external systems like ROS 2 applications. | ||
|
||
This versioning mechanism supports the [ROS 2 Message Translation Node](../ros2/px4_ros2_msg_translation_node.md), which enables seamless communication between PX4 and ROS 2 applications; when different versions of message definitions are in use, the ROS 2 translation node ensures that messages can be converted and exchanged correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little messy and perhaps can be simplified. In particular
- ROS2 Message Translation node supports versioning, not the versioning supporting the translation node.
Perhaps something like:
Message versioning was introduced to address the challenges of maintaining compatibility across systems where different versions of message definitions may be in use, such as between PX4 and external systems like ROS 2 applications. | |
This versioning mechanism supports the [ROS 2 Message Translation Node](../ros2/px4_ros2_msg_translation_node.md), which enables seamless communication between PX4 and ROS 2 applications; when different versions of message definitions are in use, the ROS 2 translation node ensures that messages can be converted and exchanged correctly. | |
Message versioning has been in introduced in PX4 v1.16 to make it easier to maintain compatibility between PX4 and ROS 2 versions. | |
This versioning mechanism uses the [ROS 2 Message Translation Node](../ros2/px4_ros2_msg_translation_node.md) to convert between different versions of messages, allowing seamless communication between PX4 and ROS 2 applications; when different versions of message definitions are in use. |
Perhaps add a diagram showing where the magic happens?
It would also be good "somewhere here" to mention any forward thinking about the ROS 2 native translation system that they have been talking about for a couple of years. Perhaps as a note at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I've added a diagram and a note concerning ROS native translation.
ROS2 Message Translation node supports versioning, not the versioning supporting the translation node.
I may be thinking of this wrong, but in my mind message versioning works as a standalone feature (it's just about some messages having a version number incremented, others not). The translation node builds on top of versioned messages. In that sense, the translation supporting the versioning sounds backwards to me. How do you see this?
- Add a version translation by adding a new translation header. Examples: (TODO: update GitHub urls) | ||
- [`translations/example_translation_direct_v1.h`](https://github.com/PX4/PX4-Autopilot/blob/message_versioning_and_translation/msg/translation_node/translations/example_translation_direct_v1.h) | ||
- [`translations/example_translation_multi_v2.h`](https://github.com/PX4/PX4-Autopilot/blob/message_versioning_and_translation/msg/translation_node/translations/example_translation_multi_v2.h) | ||
- [`translations/example_translation_service_v1.h`](https://github.com/PX4/PX4-Autopilot/blob/message_versioning_and_translation/msg/translation_node/translations/example_translation_service_v1.h) | ||
- Include the added header in [`translations/all_translations.h`](https://github.com/PX4/PX4-Autopilot/blob/message_versioning_and_translation/msg/translation_node/translations/all_translations.h). | ||
|
||
For the second last step and for topics, there are two options: | ||
|
||
1. Direct translations: these translate a single topic between two different versions. | ||
This is the simpler case and should be preferred if possible. | ||
2. Generic case: this allows a translation between N input topics and M output topics. | ||
This can be used for merging or splitting a message. | ||
Or for example when moving a field from one message to another, a single translation should be added with the two older message versions as input and the two newer versions as output. | ||
This way there is no information lost when translating forward or backward. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked at the example, but it seems to me that this might require some worked examples of each type. If the headers are specific to each type, perhaps move them under the following sections?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I've gone and added a worked example also to make the steps clearer
|
||
## Limitations | ||
|
||
- The current implementation depends on a service API that is not yet available in ROS Humble, and therefore does not support services when built for ROS Humble. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what versions will it work for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jazzy or later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GuillaumeLaine So ROS Jazzy is a dependency - if this is never going to be in ROS Humble we should maybe say
- The current implementation depends on a service API that is not yet available in ROS Humble, and therefore does not support services when built for ROS Humble. | |
- The current implementation depends on a service API that is present from ROS Jazzy (ROS applications build for Humble and earlier cannot be used with the node). |
I added a version for ROS Jazzy above on this doc. We should also add it in the guide, as noted in https://github.com/PX4/PX4-user_guide/pull/3465/files#r1924601823
## Limitations | ||
|
||
- The current implementation depends on a service API that is not yet available in ROS Humble, and therefore does not support services when built for ROS Humble. | ||
- Services only support a linear history, i.e. no message splitting or merging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean services as in something different than message/topics? Perhaps this is what you mean by .svr above? Is there information on what those mean anywhere?
Either way, so are you saying that services require a linear history, but you can split or merge messages as you like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, ROS distinguishes between services and topics like so: https://docs.ros.org/en/foxy/How-To-Guides/Topics-Services-Actions.html. In PX4, we have the srv/ services folder vs the msg/ topics folder.
Messages define the data format used to communicate over either topics or via services. The confusing part is that .srv
files define service messages, and .msg
files define topic messages (the file extension introduces confusion and make them seem more generic)
And finally yes, currently services require a linear history to translate while topics (.msg
) can be split and merged
Thanks for the comment, I realized I was also confused by this too. I'll do my best to make this clear in the docs
|
||
- The current implementation depends on a service API that is not yet available in ROS Humble, and therefore does not support services when built for ROS Humble. | ||
- Services only support a linear history, i.e. no message splitting or merging | ||
- Having both publishers and subscribers for two different versions of a topic is currently not handled by the translation node and would trigger infinite circular publications. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So a ROS application must compile against just one version of a topic? What if there are two ROS applications running on the network compiled against different versions - is this the problem case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So a ROS application must compile against just one version of a topic?
Yes
What if there are two ROS applications running on the network compiled against different versions - is this the problem case?
No, only a specific subset, like:
- app 1: pub topic_v1, sub topic_v1
- app 2: pub topic_v2, sub topic_v2
In practice there's rarely (if any) setup that will run into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite get what is being said here. I think you're saying this is OK:
app 1: pub topic_v1, sub topic_v1
app 2: pub topic_v2, sub topic_v2
But this is not
app 1: pub topic_v1, sub topic_v2
Right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite, it needs both apps with these topics to be a problem. It's really quite specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, lots of comments here. They don't reflect that this is pretty good structure. Just I'm using myself as the straw man, and I don't know much ROS 2.
When you've had a look at the comments and tried address them, can you loop in Beat and Benja to have a look? I ask because I probably will be on holiday when you do this. Back around week of Jan 20th.
22a0459
to
1bf239e
Compare
1d84269
to
ab1c88d
Compare
Maybe worth adding to the 1.16 release wip page? @hamishwillee |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, thanks for going through this
|
||
## Limitations | ||
|
||
- The current implementation depends on a service API that is not yet available in ROS Humble, and therefore does not support services when built for ROS Humble. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jazzy or later
|
||
- The current implementation depends on a service API that is not yet available in ROS Humble, and therefore does not support services when built for ROS Humble. | ||
- Services only support a linear history, i.e. no message splitting or merging | ||
- Having both publishers and subscribers for two different versions of a topic is currently not handled by the translation node and would trigger infinite circular publications. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So a ROS application must compile against just one version of a topic?
Yes
What if there are two ROS applications running on the network compiled against different versions - is this the problem case?
No, only a specific subset, like:
- app 1: pub topic_v1, sub topic_v1
- app 2: pub topic_v2, sub topic_v2
In practice there's rarely (if any) setup that will run into this.
@hamishwillee Thanks for the thorough review! I've expanded on what was there, hopefully this addresses most of your comments. Let me know if parts are still unclear when you're back. In the meantime @beniaminopozzan would you like to have a look? Hamish told me might be interested in being kept in the loop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @GuillaumeLaine !
I just pointed out a few typo. Nothing else for me to report - just exited to see it finished!
Co-authored-by: bkueng <beat-kueng@gmx.net> Co-authored-by: Hamish Willee <hamishwillee@gmail.com>
822035c
to
5c0b2d4
Compare
@@ -264,6 +267,13 @@ To create and build the workspace: | |||
git clone https://github.com/PX4/px4_ros_com.git | |||
``` | |||
|
|||
1. **(Optional)** From PX4 v1.16 (main), include the [Message Translation Node](../ros2/px4_ros2_msg_translation_node.md) into your workspace by running the following script. | |||
This step is needed only if using `px4_msgs` that do not match your target version of PX4. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified this line. Please check it is correct.
FWIW I think the updates to this doc work well.
|
||
Versioned and non-versioned messages are separated in the file system: | ||
|
||
- Non-versioned topic message files and service message files remain in the [`msg/`](https://github.com/PX4/PX4-Autopilot/tree/main/msg) and [`srv/`](https://github.com/PX4/PX4-Autopilot/tree/main/srv) directories, respectively. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've subedited this a little, mainly "to my taste".
It is still a problem for me that this is the first and only mention of "topic message files and service message files" and that there is no definition of what these are for, or where they are from. If you're from ROS-land perhaps you know, but I can assure you that many readers will not.
Can you create a section in this topic which outlines the differences/similarities, and when you should create one or the other. Can be a different PR.
|
||
The translation node has access to all message versions previously defined by PX4. It dynamically observes the DDS data space, monitoring the publications, subscriptions and services originating from either PX4 via the [uXRCE-DDS Bridge](../middleware/uxrce_dds.md), or ROS 2 applications. When necessary, it converts messages to the current versions expected by both applications and PX4, ensuring compatibility. | ||
|
||
![Overview ROS 2 Message Translation Node](../../assets/middleware/ros2/px4_ros2_interface_lib/translation_node.svg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before making changes to a versioned message (in `msg/versioned/` or `srv/versioned/`), we first create an archived copy of it, and update existing translation code to point to the archived version definition. | ||
We then update the versioned message definition to increment the version number, add any new fields or other changes, and then add a new translation file to convert between it and the previous (now archived) version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all quite hard to describe because the terminology is messy - the versioned message is both the new and old message at different points. I have tried to make this more clear by defining versioned message as "the file in this location". Similarly tried to do the same with the step points.
Not sure if this was strictly necessary, because your examples in each step make things very clear.
Note that in the example above and in most cases, step 4 only requires the developer to create a direct translation for the definition change. | ||
This is because the changes only involved a single message. | ||
In more complex cases of splitting, merging and/or moving definitions then a generic translation must be created. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to have an example of a generic translation and a service translation. This could be minimal, or a pointer to an example in the codebase once one exists.
No flaws found |
This documents the newly versioned subset of PX4 messages, and the ROS 2 translation node that enables seamless communication between PX4 and ROS 2 applications that may be using different message definition versions.
Related Work
To Do
TODO
s in the markdown of this PR