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

msg: introduce subset of versioned messages #23850

Merged
merged 5 commits into from
Dec 13, 2024

Conversation

GuillaumeLaine
Copy link
Contributor

@GuillaumeLaine GuillaumeLaine commented Oct 24, 2024

This is the first part of the larger effort to provide compatibility between PX4 and ROS2 messages versions. See the high-level design document for this effort, authored by @bkueng

Changes:

  • Partitions PX4 messages into a stable versioned subset and a larger remainder of more flexible messages
    • Versioned message definitions are moved to the msg/versioned/ directory, but output json / headers / source files were not moved
  • Introduces a version field MESSAGE_VERSION = 0 to all versioned messages

Docs

Wip: PX4/PX4-user_guide#3465

@GuillaumeLaine GuillaumeLaine marked this pull request as draft October 24, 2024 12:07
@GuillaumeLaine GuillaumeLaine marked this pull request as ready for review October 24, 2024 12:34
@GuillaumeLaine GuillaumeLaine force-pushed the add_message_versioning branch 2 times, most recently from 9d3e6b7 to eba5c19 Compare October 24, 2024 13:08
@PetervdPerk-NXP
Copy link
Member

If we migrate the dds-xrce serializer to the cyclonedds cdr serializer already used in the zenoh bridge. We could simply transport the cdr serialization opcodes for each message which are already inside the mcu flash. Removes the need of versioning or hashing.

@GuillaumeLaine GuillaumeLaine force-pushed the add_message_versioning branch 2 times, most recently from e3d79c2 to 8e08ab3 Compare October 28, 2024 09:20
@dagar dagar self-requested a review October 30, 2024 15:24
@dagar
Copy link
Member

dagar commented Oct 30, 2024

If we migrate the dds-xrce serializer to the cyclonedds cdr serializer already used in the zenoh bridge. We could simply transport the cdr serialization opcodes for each message which are already inside the mcu flash. Removes the need of versioning or hashing.

This is interesting, do you have any more detail on hand or should we discuss later?

@dagar
Copy link
Member

dagar commented Oct 30, 2024

I'm not sure aux/core is really the right distinction, it's really just versioned vs non.

What if we start with something like msgs/versioned (with initial MESSAGE_VERSION = 0) and leave the rest alone? Then we could start migrating incrementally with a review and MESSAGE_VERSION.

@PetervdPerk-NXP
Copy link
Member

If we migrate the dds-xrce serializer to the cyclonedds cdr serializer already used in the zenoh bridge. We could simply transport the cdr serialization opcodes for each message which are already inside the mcu flash. Removes the need of versioning or hashing.

This is interesting, do you have any more detail on hand or should we discuss later?

If you build the zenoh target then in te build you get the generated code i.e.
build/px4_fmu-v6x_zenoh/msg/px4/msg/Wind.c
Which contains px4_msg_Wind_ops

static const uint32_t px4_msg_Wind_ops [] =
{
  /* Wind */
  DDS_OP_ADR | DDS_OP_TYPE_8BY, offsetof (px4_msg_Wind, timestamp),
  DDS_OP_ADR | DDS_OP_TYPE_8BY, offsetof (px4_msg_Wind, timestamp_sample),
  DDS_OP_ADR | DDS_OP_TYPE_4BY | DDS_OP_FLAG_FP, offsetof (px4_msg_Wind, windspeed_north),
  DDS_OP_ADR | DDS_OP_TYPE_4BY | DDS_OP_FLAG_FP, offsetof (px4_msg_Wind, windspeed_east),
  DDS_OP_ADR | DDS_OP_TYPE_4BY | DDS_OP_FLAG_FP, offsetof (px4_msg_Wind, variance_north),
  DDS_OP_ADR | DDS_OP_TYPE_4BY | DDS_OP_FLAG_FP, offsetof (px4_msg_Wind, variance_east),
  DDS_OP_ADR | DDS_OP_TYPE_4BY | DDS_OP_FLAG_FP, offsetof (px4_msg_Wind, tas_innov),
  DDS_OP_ADR | DDS_OP_TYPE_4BY | DDS_OP_FLAG_FP, offsetof (px4_msg_Wind, tas_innov_var),
  DDS_OP_ADR | DDS_OP_TYPE_4BY | DDS_OP_FLAG_FP, offsetof (px4_msg_Wind, beta_innov),
  DDS_OP_ADR | DDS_OP_TYPE_4BY | DDS_OP_FLAG_FP, offsetof (px4_msg_Wind, beta_innov_var),
  DDS_OP_RTS
};

You can simply transfer the ops from ROM and wire to validate the serialization/deserialization used for each message. We could also schedule meeting/brainstorm session to more into depth into this.

@bkueng
Copy link
Member

bkueng commented Oct 31, 2024

Thanks @PetervdPerk-NXP. What we need goes a bit further than just version detection. You find the details under the linked doc above: https://docs.google.com/document/d/18_RxV1eEjt4haaa5QkFZAlIAJNv9w5HED2aUEiG7PVQ/edit
Having the (de)-serialization info can be useful to build a fully dynamic serialization approach. That is not implemented in ROS yet, and it also leaves open how to specifically convert between message versions. There's plans in ROS to provide a mechanism for that, and once that's available I think it makes sense to use that (the doc contains more details).
Yes if you want, we can do a meeting.

@GuillaumeLaine there's also some scripts under Tools/ that enumerate the .msg files directly, e.g. https://github.com/PX4/PX4-Autopilot/blob/main/Tools/msg/generate_msg_docs.py#L28

@hamishwillee
Copy link
Contributor

Very cool. Have you thought about how this will be integrated/documented?

@GuillaumeLaine GuillaumeLaine changed the title msg: add versioning and split into core/aux messages msg: introduce subset of versioned messages Nov 6, 2024
Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

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

I found another reference to msg/: https://github.com/PX4/PX4-Autopilot/blob/main/src/modules/uxrce_dds_client/generate_dds_topics.py#L46. The argument isn't used anymore and can just be removed.

Jenkinsfile Outdated
sh('rm -f px4_msgs/srv/*.srv')
sh('cp msg/*.msg px4_msgs/msg/')
sh('cp msg/versioned/*.msg px4_msgs/versioned/msg/')
Copy link
Member

Choose a reason for hiding this comment

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

This will require a change in the px4_msgs repo too: https://github.com/PX4/px4_msgs/blob/main/CMakeLists.txt#L21.
Either way is fine with me, but maybe simpler to keep them merged for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I recall @dagar being in favor of visibly splitting the messages, so I've opened a PR in px4_msgs to address your comment thanks

msg/CMakeLists.txt Outdated Show resolved Hide resolved
@GuillaumeLaine GuillaumeLaine force-pushed the add_message_versioning branch 2 times, most recently from f2dc913 to 96ae5f0 Compare November 20, 2024 11:05
@DronecodeBot
Copy link

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/px4-sync-q-a-nov-20-2024/42478/1

bkueng
bkueng previously approved these changes Nov 21, 2024
Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

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

Looks good from my side

@dagar
Copy link
Member

dagar commented Dec 12, 2024

If we bring this in now do we have any mechanism for enforcing the version bump? Who is then responsible for handling the field translation?

@bkueng
Copy link
Member

bkueng commented Dec 13, 2024

There's a follow-up for that which adds CI to check if versioning got added when a message changes. Here's the branch: https://github.com/PX4/PX4-Autopilot/tree/message_versioning_and_translation

@bkueng
Copy link
Member

bkueng commented Dec 13, 2024

Let's get this in then while there's no conflicts, and I'll do the follow-up.

The container CI build failure is a permission failure (probably because the PR comes from an external repo).

@bkueng bkueng merged commit 2e16637 into PX4:main Dec 13, 2024
56 of 57 checks passed
@hamishwillee
Copy link
Contributor

From a docs point of view this does two things:

  1. In the top level index it groups the topics by versioned and not versioned.
  2. In each topic it fixes up the source link to go to the correct target, whether versioned or not.

There is no obvious way to tell that a file is versioned or not:

  • In the page itself (unless you already know about MESSAGE_VERSION
  • From the sidebar

Should their be?

I don't think we need this information in the sidebar, but I think you might well be interested if you're looking at an individual page. Thoughts @bkueng ?

@bkueng
Copy link
Member

bkueng commented Dec 19, 2024

Yes it could be added to the individual page.

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.

6 participants