Skip to content

Type hash in GraphCache, user_data encoding tools#70

Merged
clalancette merged 16 commits intoros2:rollingfrom
emersonknapp:emersonknapp/type-version-hash
Mar 26, 2023
Merged

Type hash in GraphCache, user_data encoding tools#70
clalancette merged 16 commits intoros2:rollingfrom
emersonknapp:emersonknapp/type-version-hash

Conversation

@emersonknapp
Copy link
Contributor

@emersonknapp emersonknapp commented Feb 14, 2023

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 emersonknapp changed the base branch from master to rolling February 14, 2023 23:57
@emersonknapp emersonknapp changed the title WIP start adding entrypoint to fill out type version hash [WIP] start adding entrypoint to fill out type version hash Feb 14, 2023
@emersonknapp emersonknapp changed the title [WIP] start adding entrypoint to fill out type version hash [WIP] Add topic_type_hash to EntityInfo Feb 15, 2023
@emersonknapp emersonknapp changed the title [WIP] Add topic_type_hash to EntityInfo [WIP] Add topic_type_hash to GraphCache Feb 20, 2023
@emersonknapp emersonknapp changed the title [WIP] Add topic_type_hash to GraphCache [WIP] Type hash in GraphCache, user_data encoding tools Feb 21, 2023
@emersonknapp emersonknapp changed the title [WIP] Type hash in GraphCache, user_data encoding tools Type hash in GraphCache, user_data encoding tools Feb 23, 2023
@emersonknapp emersonknapp marked this pull request as ready for review February 23, 2023 21:49
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp emersonknapp force-pushed the emersonknapp/type-version-hash branch from d615b71 to 7564a5e Compare March 12, 2023 02:33
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Copy link
Contributor Author

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

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

A few things to fix in here.

…o more error handling

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp emersonknapp requested review from clalancette and removed request for ivanpauno March 14, 2023 03:18
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

3 really minor nits about error handling, otherwise this looks good to me.

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

This looks good to me now. Unfortunately I realized we can't merge this in until we have the rmw_* changes reviewed and approved, since that would lead to deprecation warnings. So those will be the next step.

@emersonknapp
Copy link
Contributor Author

emersonknapp commented Mar 17, 2023

@clalancette
Copy link
Contributor

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
Copy link
Contributor Author

emersonknapp commented Mar 20, 2023

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
Copy link
Contributor Author

emersonknapp commented Mar 20, 2023

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@emersonknapp
Copy link
Contributor Author

emersonknapp commented Mar 20, 2023

@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
Copy link
Contributor Author

emersonknapp commented Mar 20, 2023

@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

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Copy link
Contributor Author

emersonknapp commented Mar 24, 2023

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

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

Building with no package specifier args, all default DDS options

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette clalancette merged commit 0dc8af3 into ros2:rolling Mar 26, 2023
christophebedard added a commit that referenced this pull request Sep 29, 2025
They were deprecated in #70 for Iron and safe to remove in Jazzy.

Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
ahcorde pushed a commit that referenced this pull request Sep 30, 2025
They were deprecated in #70 for Iron and safe to remove in Jazzy.

Signed-off-by: Christophe Bedard <bedard.christophe@gmail.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.

3 participants