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.

Sign up for GitHub

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

Jump to bottom

Type hash distribution in discovery (rep2011) #671

Merged
merged 17 commits into from
Mar 26, 2023
Merged

Type hash distribution in discovery (rep2011) #671

merged 17 commits into from
Mar 26, 2023

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
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp Outdated Show resolved Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/qos.hpp Show resolved Hide resolved
rmw_fastrtps_dynamic_cpp/src/publisher.cpp Outdated Show resolved Hide resolved
rmw_fastrtps_dynamic_cpp/src/rmw_client.cpp Outdated Show resolved Hide resolved
rmw_fastrtps_dynamic_cpp/src/rmw_service.cpp Outdated Show resolved Hide resolved
rmw_fastrtps_dynamic_cpp/src/subscription.cpp Outdated Show resolved Hide resolved
rmw_fastrtps_shared_cpp/src/qos.cpp Show resolved Hide resolved
rmw_fastrtps_shared_cpp/src/qos.cpp Outdated Show resolved Hide resolved
rmw_fastrtps_shared_cpp/src/qos.cpp Show resolved Hide resolved
rmw_fastrtps_shared_cpp/test/test_rmw_qos_to_dds_attributes.cpp Outdated Show resolved Hide resolved
@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.

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp Show resolved Hide resolved
rmw_fastrtps_shared_cpp/src/qos.cpp Show resolved Hide resolved
@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

  • 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
@@ -314,7 +314,9 @@ rmw_create_client(
reader_qos.data_sharing().off();
}

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

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

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

rmw_fastrtps_shared_cpp/src/qos.cpp Outdated Show resolved Hide resolved
rmw_fastrtps_shared_cpp/test/test_rmw_qos_to_dds_attributes.cpp Outdated Show resolved Hide resolved
@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

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.

None yet
3 participants
@emersonknapp @clalancette @wjwwood