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

Service introspection #127

Merged
merged 36 commits into from
Feb 2, 2023
Merged

Service introspection #127

merged 36 commits into from
Feb 2, 2023

Conversation

ihasdapie
Copy link
Member

This PR is part of a prototype implementation for ros-infrastructure/rep#360 ros2/ros2#1285. It implements the new methods defined in rosidl_runtime_cpp in ros2/rosidl#700 allowing for rcl to populate service_event message payloads.

Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM, just some minor comments and a bikeshed question about using "event" vs. "introspection".

rosidl_typesupport_cpp/resource/srv__type_support.cpp.em Outdated Show resolved Hide resolved
rosidl_typesupport_cpp/CMakeLists.txt Outdated Show resolved Hide resolved
rosidl_typesupport_c/resource/srv__type_support.cpp.em Outdated Show resolved Hide resolved
rosidl_typesupport_c/resource/srv__type_support.cpp.em Outdated Show resolved Hide resolved
rosidl_typesupport_c/resource/srv__type_support.cpp.em Outdated Show resolved Hide resolved
rosidl_typesupport_c/resource/srv__type_support.cpp.em Outdated Show resolved Hide resolved
rosidl_typesupport_c/resource/srv__type_support.cpp.em Outdated Show resolved Hide resolved
rosidl_typesupport_cpp/resource/srv__type_support.cpp.em Outdated Show resolved Hide resolved
rosidl_typesupport_cpp/resource/srv__type_support.cpp.em Outdated Show resolved Hide resolved
rosidl_typesupport_cpp/resource/srv__type_support.cpp.em Outdated Show resolved Hide resolved
rosidl_typesupport_cpp/src/type_support_dispatch.hpp Outdated Show resolved Hide resolved
@jacobperron jacobperron self-assigned this Sep 1, 2022
@ihasdapie
Copy link
Member Author

@jacobperron Pinging to request review & help with changes introduced in d48c0ee to split typesupport tests into a new package so that we can test the generated typesupport files. I'm stuck on some linker issues while building, so some feedback would be much appreciated.

@jacobperron
Copy link
Member

rebased on rolling

Copy link
Member

@jacobperron jacobperron 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 fixed the test and expanded on it. Also did some other minor cleanup.

I've left comments for things that still need to be addressed.

rosidl_typesupport_c/resource/srv__type_support.cpp.em Outdated Show resolved Hide resolved
rosidl_typesupport_c/CMakeLists.txt Outdated Show resolved Hide resolved
rosidl_typesupport_cpp/resource/srv__type_support.cpp.em Outdated Show resolved Hide resolved
"${cpp_typesupport_target}"
)

# TODO(jacobperron): Add tests for rosidl_typesupport_c
Copy link
Member

Choose a reason for hiding this comment

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

We can add equivalent tests for C-typesupport to their own test file.

}
}

// TODO(jacobperron): Add test for action service
Copy link
Member

Choose a reason for hiding this comment

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

May as well add tests with an action service.

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

note to self: check open discussions again, review tests.

Otherwise LGTM

rosidl_typesupport_c/resource/srv__type_support.cpp.em Outdated Show resolved Hide resolved
rosidl_typesupport_cpp/resource/srv__type_support.cpp.em Outdated Show resolved Hide resolved
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.

lgtm, with test todo's addressed

@ivanpauno
Copy link
Member

lgtm, with test todo's addressed

Tests completed in f273831.

Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
ihasdapie and others added 25 commits January 31, 2023 15:10
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Remove unnecessary dependencies

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
It should not be a UUID.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Contributor

CI for this is in ros2/ros2#1285 (comment)

@clalancette clalancette merged commit 0d4a26b into rolling Feb 2, 2023
@delete-merged-branch delete-merged-branch bot deleted the service_introspection branch February 2, 2023 14:34
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

6 participants