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

Runtime Interface Reflection: rosidl #728

Merged
merged 11 commits into from
Apr 8, 2023
Merged

Conversation

methylDragon
Copy link
Contributor

@methylDragon methylDragon commented Mar 16, 2023

Depends (and is based on): #727
This PR is part of the runtime interface reflection subscription feature of REP-2011: ros2/ros2#1374

⚠️ NOTE: This PR has been rebased on the remote mirror of the emersonknapp/type-descripton-runtime branch it builds ontop of to help with reviewing. But when #727 gets merged, this should then retarget rolling

Description

This PR adds utilities for manipulating the rosidl_runtime_c__type_description structs defined in #727

It was migrated (and further modified from): ros2/rcl_interfaces#155

Functionalities:

  • Handy print functions
  • Hash map construction
  • Sorting and pruning of referenced types (to prep them for hashing)
  • Checking of type validity
  • Appending fields and referenced type descriptions
  • Creating TypeDescriptions from referenced type descriptions (description subsetting)
  • I also pre-generate the type_description_interfaces sources and headers to create a new package (type_description_interfaces_static) to remove a dependency cycle downstream. (That's why there are so many lines.)

Asan runs green too!

clear && \
colcon build --packages-select rosidl_runtime_c \
  --build-base=build-asan --install-base=install-asan \
  --cmake-args -DOSRF_TESTING_TOOLS_CPP_DISABLE_MEMORY_TOOLS=ON \
               -DINSTALL_EXAMPLES=OFF -DSECURITY=ON --no-warn-unused-cli \
               -DCMAKE_BUILD_TYPE=Debug \
  --mixin asan-gcc && \
colcon test --build-base=build-asan --install-base=install-asan \
  --event-handlers sanitizer_report+ --packages-select rosidl_runtime_c

There are some less important tests waiting to be written (printing), and some tests that are already implicitly covered by other tests (but are not explicit yet.)

TODO

  • More minor tests
  • 100% docstring coverage

Open Questions

  • This only operates on the C version of the type description structs... Do I need to write a C++ version? (Is there a way to convert between the two versions of the message, or are they otherwise interchangeable?)
    • Perhaps that C++ utils library would just have a conversion function, since we know the layout of the struct already???

@methylDragon methylDragon force-pushed the runtime_interface_reflection branch 3 times, most recently from 9115cea to 74d35b0 Compare March 18, 2023 09:06
@methylDragon methylDragon force-pushed the runtime_interface_reflection branch 2 times, most recently from 4155c36 to a1cc3d8 Compare March 28, 2023 22:30
@methylDragon methylDragon changed the base branch from rolling to emersonknapp/type-description-runtime March 28, 2023 22:30
@methylDragon methylDragon force-pushed the runtime_interface_reflection branch 7 times, most recently from 61a04cd to 27d6e78 Compare April 4, 2023 20:05
@methylDragon methylDragon changed the base branch from emersonknapp/type-description-runtime to emersonknapp/type-description-struct April 5, 2023 23:35
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.

Initial review with a few requested changes, but mostly non-blocking nitpicks.

We're still trying to nail down the double-free issues in some of the tests.

rosidl_runtime_c/src/type_description_utils.c Outdated Show resolved Hide resolved
rosidl_runtime_c/src/type_description_utils.c Outdated Show resolved Hide resolved
rosidl_runtime_c/src/type_description_utils.c Show resolved Hide resolved
rosidl_runtime_c/src/type_description_utils.c Outdated Show resolved Hide resolved
rosidl_runtime_c/src/type_description_utils.c Show resolved Hide resolved
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon methylDragon force-pushed the runtime_interface_reflection branch 2 times, most recently from f759287 to a448392 Compare April 7, 2023 02:08
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon
Copy link
Contributor Author

I'm not too sure why DCO is kicking up a fuss

@methylDragon methylDragon changed the base branch from emersonknapp/type-description-struct to rolling April 7, 2023 05:33
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
@wjwwood wjwwood merged commit 6da8660 into rolling Apr 8, 2023
@delete-merged-branch delete-merged-branch bot deleted the runtime_interface_reflection branch April 8, 2023 18:57
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.

None yet

2 participants