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

Type hash in interface codegen (rep2011) #722

Merged
merged 65 commits into from
Mar 15, 2023

Conversation

emersonknapp
Copy link
Collaborator

@emersonknapp emersonknapp commented Jan 24, 2023

Related to ros-infrastructure/rep#358
Part of ros2/ros2#1159
Related to ros2/rcl#1027
No dependencies.

Feature addition: New generator package rosidl_generator_type_hash that creates JSON representations and their corresponding SHA256 hashes for ROS 2 interfaces during code generation. A new type rosidl_type_hash_t that contains these versioned hash values and can be used to store them in generated code and be used in RMW discovery.

Change brief by repository:

  • rosidl_cmake - Add new argument to generator
  • rosidl_generator_c - depend on new typehash generator, embed hashes in generated C code using the new rosidl_runtime_c struct
  • rosidl_generator_cpp - depend on typehash generator, embed hashes in generated C++ code using the new rosidl_runtime_c struct
  • rosidl_generator_tests - new tests for the typehash generator
  • rosidl_generator_type_hash - new generator, reads ROS IDL representations, serializes to hashable JSON format, and produces SHA256 hashes for all interface types, according to REP-2011. See README.md for more details
  • rosidl_pycommon - utility for loading and embedding hashes in code generators
  • rosidl_runtime_c - struct rosidl_type_hash_t for representing type hashes, with string utilities for encoding
  • rosidl_typesupport_introspection_c - add the type hash to introspection alongside type name for use by RMW implementation
  • rosidl_typesupport_introspection_cpp - add the type hash to introspection alongside type name for use by RMW implementation

The hashes are provided to the install tree, and so are available to language code generators (C, C++, Python) to be embedded in generated code for calculation-free use at runtime.

For Messages, a simple hash is output. For Services and Actions, all generated subinterfaces are also represented and hashed, so that every Service and Message in the resulting installation has its own unique hash. See the rosidl_generator_type_hash/resource/HashedTypeDescription.schema.json file, as mentioned in the README for a strict description of the structure.

Attached are archives containing the output tree of build/example_interfaces/rosidl_generator_type_hash/rosidl_generator_c for representative output.

rosidl_generator_c.tar.gz

@emersonknapp
Copy link
Collaborator Author

@clalancette @wjwwood This still needs testing but I feel good about it functionally - feedback, especially on high level approach and architecture would be very valuable now

@emersonknapp emersonknapp marked this pull request as ready for review February 10, 2023 04:15
@emersonknapp emersonknapp changed the title [WIP] REP-2011 Type Version Hash in interface codegen Type Version Hash in interface codegen (REP-2011 ) Feb 10, 2023
@emersonknapp emersonknapp changed the title Type Version Hash in interface codegen (REP-2011 ) Type Version Hash in interface codegen (REP-2011) Feb 10, 2023
Copy link
Contributor

@methylDragon methylDragon left a comment

Choose a reason for hiding this comment

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

Some small comments

rosidl_generator_type_hash/setup.cfg Outdated Show resolved Hide resolved
rosidl_pycommon/rosidl_pycommon/__init__.py Outdated Show resolved Hide resolved
rosidl_generator_type_hash/package.xml Outdated Show resolved Hide resolved
rosidl_generator_c/resource/idl__struct.h.em Outdated Show resolved Hide resolved
@methylDragon
Copy link
Contributor

methylDragon commented Feb 16, 2023

Also additional note: The hashes aren't getting populated/generated for rosidl_generator_py, but I don't think we need it to at the moment

@emersonknapp
Copy link
Collaborator Author

@methylDragon see ros2/rosidl_python#191 for rosidl_generator_py - but that one is fairly simple and is just a quick copy of the patterns established here

@methylDragon methylDragon self-assigned this Feb 17, 2023
@emersonknapp emersonknapp marked this pull request as draft February 20, 2023 20:08
@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Feb 20, 2023

Converting back to draft while I do the CLI and figure out a couple other bits I'm discovering that I need downstream for RMW implementations to distribute the type hash during discovery.

Didn't hear any major architectural concerns, so I am thinking the main thing needed here is documentation and tests, not overhaul.

@emersonknapp emersonknapp changed the title Type Version Hash in interface codegen (REP-2011) [WIP] Type Version Hash in interface codegen (REP-2011) Feb 20, 2023
@emersonknapp emersonknapp changed the title [WIP] Type Version Hash in interface codegen (REP-2011) [WIP] Type hash in interface codegen (rep2011) Feb 21, 2023
@emersonknapp emersonknapp force-pushed the emersonknapp/type-version-hash branch from 6433ecd to 9035b23 Compare February 23, 2023 00:39
@emersonknapp
Copy link
Collaborator Author

The Rpr runner is showing an interesting side effect right now - since the PR runner only builds packages in this repository. The rosidl_typesupport_introspection_tests package defines a service and therefore refers to ServiceEventInfo.msg (this is just the first example that comes up in the build). But, since the tree underneath that test has not been built with this PR, those messages don't have the generated hashes that are expected by our generated code locally. Do we reasonably expect that a new generator will require that all interfaces packages must be rebuilt, or should it be able to handle this case?

@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Feb 23, 2023

Sanity check time!

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

@emersonknapp emersonknapp marked this pull request as ready for review February 23, 2023 01:10
@emersonknapp emersonknapp changed the title [WIP] Type hash in interface codegen (rep2011) Type hash in interface codegen (rep2011) Feb 23, 2023
@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Feb 23, 2023

@clalancette @wjwwood @methylDragon @james-rms
This is now ready for review. It's unfortunately gotten large, but some percentage of that is copyright headers... I've updated the description up top with a brief on the changes.

I have prototypes working donwstream of this passing type hashes in FastDDS and CycloneDDS via user_data, which validates to me that this PR is now providing all required interfaces to enable Type Hash Distribution.

@emersonknapp
Copy link
Collaborator Author

@methylDragon yes that is true - referenced type descriptions are sorted alphabetically (rather than, for example, some topological ordering), but fields are added in the order they are specified in each individual type definition

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 done a first review pass here. I generally understand what you are doing in here, and I don't see any major flaws. I've left a few things to fix up, and I also need to go back and look at the empy things more carefully, but we can at least start with this stuff.

rosidl_generator_c/rosidl_generator_c/__init__.py Outdated Show resolved Hide resolved
rosidl_runtime_c/CMakeLists.txt Outdated Show resolved Hide resolved
rosidl_runtime_c/src/type_hash.c Outdated Show resolved Hide resolved
rosidl_runtime_c/src/type_hash.c Outdated Show resolved Hide resolved
rosidl_runtime_c/src/type_hash.c Outdated Show resolved Hide resolved
rosidl_runtime_c/src/type_hash.c Outdated Show resolved Hide resolved
rosidl_runtime_c/src/type_hash.c Outdated Show resolved Hide resolved
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp emersonknapp force-pushed the emersonknapp/type-version-hash branch from fb5af8d to 1ff7937 Compare March 8, 2023 05:15
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp emersonknapp force-pushed the emersonknapp/type-version-hash branch from 1ff7937 to 3fce7b5 Compare March 8, 2023 05:55
@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Mar 8, 2023

Next phase sanity checking (windows not ready for it just yet)

  • Linux Build Status

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
…ts for them

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

emersonknapp commented Mar 10, 2023

  • Windows Build Status

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.

OK, I did a more thorough review and also looked at the generated output.

Despite the fact that I left quite a few comments, they are almost all minor or questions. Overall, this looks really fantastic. I think once we get these things solved and also get Windows happy, we can proceed with this.

rosidl_generator_c/resource/idl__struct.h.em Outdated Show resolved Hide resolved
rosidl_generator_c/resource/msg__struct.h.em Outdated Show resolved Hide resolved
rosidl_generator_tests/package.xml Show resolved Hide resolved
rosidl_generator_tests/CMakeLists.txt Outdated Show resolved Hide resolved
rosidl_runtime_c/src/type_hash.c Outdated Show resolved Hide resolved
rosidl_runtime_c/src/type_hash.c Outdated Show resolved Hide resolved
rosidl_runtime_c/src/type_hash.c Outdated Show resolved Hide resolved
rosidl_runtime_c/src/type_hash.c Outdated Show resolved Hide resolved
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
Copy link
Collaborator Author

Gist: https://gist.githubusercontent.com/emersonknapp/190b20823a334c72086b26bd139e29d6/raw/adb81acaee0b780fc260e3461ca5f8efba166a46/ros2.repos
BUILD args: --packages-above-and-dependencies rosidl_cmake rosidl_generator_c rosidl_generator_cpp rosidl_generator_tests rosidl_generator_type_description rosidl_pycommon rosidl_runtime_c rosidl_typesupport_introspection_c rosidl_typesupport_introspection_cpp
TEST args: --packages-above rosidl_cmake rosidl_generator_c rosidl_generator_cpp rosidl_generator_tests rosidl_generator_type_description rosidl_pycommon rosidl_runtime_c rosidl_typesupport_introspection_c rosidl_typesupport_introspection_cpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11586

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

@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Mar 10, 2023

OK - I think I've pushed an update to all the actionable comments and answered all the questions. Green CI!

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

OK, I went back through, and there is one minor renaming thing left. After that, this is good to merge.

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
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.

Thanks for all of the back-and-forth here!

This now looks good to me. I'll suggest we run CI with just this PR in place, since we'll only be merging this one right away.

Once that CI is green, I'm going to wait until after our weekly meeting to merge this in.

@emersonknapp
Copy link
Collaborator Author

Gist: https://gist.githubusercontent.com/emersonknapp/54b2023bfdc0fc52f06b54c83afd1d59/raw/adb81acaee0b780fc260e3461ca5f8efba166a46/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11595

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

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

I skimmed the C code part and it lgtm, with some nitpick/comments, but nothing blocking.


uint8_t nibble = 0;
char * dest = NULL;
for (size_t i = 0; i < ROSIDL_TYPE_HASH_SIZE; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

nitpick:

Suggested change
for (size_t i = 0; i < ROSIDL_TYPE_HASH_SIZE; i++) {
for (size_t i = 0; i < ROSIDL_TYPE_HASH_SIZE; ++i) {

https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement

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