search

ros2 / rmw_dds_common

Apache License 2.0
11 stars 20 forks source link

Type hash in GraphCache, user_data encoding tools #70

Closed emersonknapp closed 1 year ago

emersonknapp commented 1 year ago

Part of ros2/ros2#1159 Depends on ros2/rosidl#722 Depends on ros2/rmw#348

Updates the GraphCache's EntityInfo struct with a new field topic_type_hash that can be filled out during discovery by implementations, which is passed through to rmw_topic_endpoint_info.

Adds utility functions parse_type_hash_from_user_data_qos and encode_type_hash_for_user_data_qos for using this value as a USER_DATA QoS value during discovery.

emersonknapp commented 1 year ago

@ivanpauno this is ready for review, it's only blocked on dependencies but the types seem to be finalized

emersonknapp commented 1 year ago

Same run combined for PRs ros2/rmw_dds_common#70 ros2/rmw_fastrtps#671 ros2/rmw_cyclonedds#437 ros2/rmw_connextdds#104

Gist: https://gist.githubusercontent.com/emersonknapp/597121a37b82d9418b14472de760c8f0/raw/b0412d81c2ae2368cb592c810496d6f92650ad68/ros2.repos BUILD args: TEST args: ROS Distro: rolling Job: ci_launcher ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11623

clalancette commented 1 year ago

The windows failed tests are known flakes, so no worries there.

The ones on Linux are odd, though. I don't recognize them from the nightlies, so it bears looking at or trying again.

emersonknapp commented 1 year ago

The Linux failures were due to outdated rmw_connextdds branch - I hadn't rebased on the INCOMPATIBLE_TYPE change, so it was printing an error about unhandled publisher event. Rebased and restarting CI now (aarch64 was ok because connext doesn't run there)

emersonknapp commented 1 year ago
emersonknapp commented 1 year ago

@clalancette even if that's all green, I think we should actually hold off merging until the Tuesday design conversation - I want to propose a rosidl API change for the next phase of features that would expose the type hash slightly differently and therefore require a slight change in the RMW implementation PRs

emersonknapp commented 1 year ago

@clalancette sorry for the runaround, these are actually fine. I hadn't finished working through that other idea, it's not the best way after all. Let's move forward with these as they are

Edit: yep let's just discuss in the meeting first before pulling the trigger

emersonknapp commented 1 year ago

Gist: https://gist.githubusercontent.com/emersonknapp/c36ca5985030eccbcaac0d0384236fbd/raw/374f6ef848452efc7e6a2b079129195ece75aeec/ros2.repos BUILD args: --packages-above-and-dependencies rmw_dds_common TEST args: --packages-above rmw_dds_common ROS Distro: rolling

clalancette commented 1 year ago

FYI: the currently running build has CI_USE_CONNEXTDDS set to "False", so it isn't building/testing with Connext. So we should restart the job. While we are at it, we may as well just build and test everything (i.e. no BUILD or TEST args).

clalancette commented 1 year ago

FYI: the currently running build has CI_USE_CONNEXTDDS set to "False", so it isn't building/testing with Connext. So we should restart the job. While we are at it, we may as well just build and test everything (i.e. no BUILD or TEST args).

Ah, never mind. I was looking at the aarch64 job, which should have it set to False. The amd64 job has it set to True. Never mind me.

In any case, rerunning and testing "everything" is a good idea here anyway.

emersonknapp commented 1 year ago

Building with no package specifier args, all default DDS options