Skip to content

Type hash distribution in discovery (rep2011)#671

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

Type hash distribution in discovery (rep2011)#671
clalancette merged 17 commits intoros2:rollingfrom
emersonknapp:emersonknapp/type-version-hash

Conversation

@emersonknapp
Copy link
Contributor

@emersonknapp emersonknapp commented Feb 17, 2023 •
edited
Loading

Part of ros2/ros2#1159

Depends on ros2/rmw_dds_common#70
Depends on ros2/rosidl#729

Features:

  • Retrieve type version hash from generated code, and use the USER_DATA QoS policy to distribute it for all readers/writers. Format of the USER_DATA uses common utility functions in rmw_dds_common
  • On discovering new endpoints, parse the USER_DATA to set rmw_topic_endpoint_info_t.type_hash

This was referenced Feb 17, 2023
Closed
Merged
@emersonknapp emersonknapp changed the title [WIP] Type Hash Distribution [WIP] Type hash distribution in discovery (rep2011) Feb 21, 2023
@emersonknapp emersonknapp changed the title [WIP] Type hash distribution in discovery (rep2011) Type hash distribution in discovery (rep2011) Feb 23, 2023
@emersonknapp emersonknapp marked this pull request as ready for review February 23, 2023 21:51
@emersonknapp emersonknapp force-pushed the emersonknapp/type-version-hash branch 2 times, most recently from 326cc22 to b1967cd Compare March 13, 2023 07:13
clalancette
clalancette requested changes Mar 15, 2023
View reviewed changes
@emersonknapp emersonknapp force-pushed the emersonknapp/type-version-hash branch 2 times, most recently from ec3df77 to 37b65f6 Compare March 15, 2023 19:52
@emersonknapp emersonknapp requested review from clalancette and removed request for gbiggs and sloretz March 16, 2023 01:31
@emersonknapp emersonknapp force-pushed the emersonknapp/type-version-hash branch 2 times, most recently from 4cc09ad to 2fb92a1 Compare March 16, 2023 02:28
clalancette
clalancette requested changes Mar 16, 2023
View reviewed changes
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.

I've left two more things I think should be changed. Otherwise this looks good to me.

@emersonknapp
Copy link
Contributor Author

emersonknapp commented Mar 17, 2023 •
edited
Loading

Same run combined for PRs ros2/rmw_dds_common#70 #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

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

This was referenced Mar 17, 2023
Merged
Merged
Merged
@emersonknapp
Copy link
Contributor Author

emersonknapp commented Mar 21, 2023

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

@emersonknapp emersonknapp force-pushed the emersonknapp/type-version-hash branch from 4aa2acb to ba7eaf6 Compare March 22, 2023 07:24
wjwwood
wjwwood reviewed Mar 23, 2023
View reviewed changes
rmw_fastrtps_cpp/src/rmw_client.cpp

if (!get_datareader_qos(adapted_qos_policies, reader_qos)) {
if (!get_datareader_qos(
adapted_qos_policies, *type_supports->response_typesupport->type_hash, reader_qos))
Copy link
Member

@wjwwood wjwwood Mar 23, 2023

Choose a reason for hiding this comment

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

nitpick: I don't know if this package has our linters run over it, but I would have expected this to use two spaces of indent, not four

Copy link
Contributor Author

@emersonknapp emersonknapp Mar 23, 2023

Choose a reason for hiding this comment

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

linters did run - your intuition is my intuition, but uncrustify wants it this way. I believe the logic is "2 times the number of opening braces" - in this case there's if (( so it expects 4 spaces

@emersonknapp
WIP just making the branch and noting the place where code goes
aefd05d 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Basic test workflow, not getting actual type hash value yet
dd124b3 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Adding type hash to user data
c4b18ff 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Receive type hash (and debug print)
3e9feea 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Use common helpers for type hash user_data encoding
6d38682 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
User data key-value strings
0b598a0 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Type hash struct first pass
41fe673 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Fix debug print
04788cd 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Add dynamic_cpp implementations
765021c 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Remove debug logging
740421f 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Update fn name
1112a0f 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Fix linter
fba5d60 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Use new signatures
f22e400 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Address review comments
a39da37 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Add warnings on failed type hash operations
f723920 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Use new typesupport hashes
f7b2aab 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
review nits
40afb80 
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp emersonknapp force-pushed the emersonknapp/type-version-hash branch from b6a69f2 to 40afb80 Compare March 24, 2023 17:05
@emersonknapp
Copy link
Contributor Author

emersonknapp commented Mar 24, 2023 •
edited
Loading

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

clalancette commented Mar 26, 2023

CI is in ros2/rmw_dds_common#70 (comment)

@clalancette clalancette merged commit 901339f into ros2:rolling Mar 26, 2023
@sloretz sloretz mentioned this pull request Mar 30, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wjwwood wjwwood wjwwood left review comments

@clalancette clalancette clalancette approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@emersonknapp @clalancette @wjwwood