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

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.

@ihasdapie ihasdapie mentioned this pull request Aug 10, 2022
Closed
14 tasks
@ihasdapie ihasdapie requested a review from jacobperron August 18, 2022 18:22
jacobperron
jacobperron reviewed Aug 18, 2022
View reviewed changes
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
jacobperron
jacobperron reviewed Aug 31, 2022
View reviewed changes
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
rosidl_typesupport_tests/test/rosidl_typesupport_cpp/test_service_typesupport.cpp 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

jacobperron
jacobperron reviewed Sep 30, 2022
View reviewed changes
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
rosidl_typesupport_tests/CMakeLists.txt Outdated
"${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.

rosidl_typesupport_tests/test/rosidl_typesupport_cpp/test_service_typesupport.cpp Outdated
}
}

// 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.

ivanpauno
ivanpauno reviewed Nov 11, 2022
View reviewed changes
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
@ivanpauno ivanpauno force-pushed the service_introspection branch from 2d15999 to f1bca72 Compare November 11, 2022 20:27
@ivanpauno ivanpauno requested a review from wjwwood November 16, 2022 14:09
@ivanpauno ivanpauno mentioned this pull request Dec 15, 2022
Merged
wjwwood
wjwwood reviewed Dec 21, 2022
View reviewed changes
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.

@clalancette clalancette force-pushed the service_introspection branch from c22d98d to 296d5f7 Compare January 25, 2023 18:29
@ihasdapie @clalancette
preliminary c ts impl for service introspection msg gen
b45bf00 
Signed-off-by: Brian Chen <[email protected]>
@ihasdapie @clalancette
preliminary cpp ts impl for service introspection msg gen
0e9197e 
Signed-off-by: Brian Chen <[email protected]>
@ihasdapie @clalancette
use event_msg->info directly
5a47f94 
Signed-off-by: Brian Chen <[email protected]>
@ihasdapie @clalancette
enable/disable event msg payload
659fa71 
Signed-off-by: Brian Chen <[email protected]>
@ihasdapie @clalancette
remove debug code
acd7ae5 
Signed-off-by: Brian Chen <[email protected]>
@ihasdapie @clalancette
use suffix from rosidl_parser.definition
200fa79 
Signed-off-by: Brian Chen <[email protected]>
@ihasdapie @clalancette
const void* service introspection message create handle
c481c6d 
Signed-off-by: Brian Chen <[email protected]>
@ihasdapie @clalancette
update tests to reflect rosidl_service_type_support_t
3137d0f 
Signed-off-by: Brian Chen <[email protected]>
@ihasdapie @clalancette
fix tests with new service typesupport definition
a4d426a 
Signed-off-by: Brian Chen <[email protected]>
ihasdapie and others added 25 commits January 31, 2023 15:10
@ihasdapie @clalancette
remove zero_allocate calls
fee9c5d 
Signed-off-by: Brian Chen <[email protected]>
@ihasdapie @clalancette
remove ifdef cpp
709b831 
Signed-off-by: Brian Chen <[email protected]>
@ihasdapie @clalancette
remove unnecessary rcutils_reset_error
dc3410b 
Signed-off-by: Brian Chen <[email protected]>
@ihasdapie @clalancette
fix tests
b67d241 
Signed-off-by: Brian Chen <[email protected]>
@jacobperron @clalancette
Fix test + more comprehensive error checking
ac0a4f3 
Signed-off-by: Jacob Perron <[email protected]>
@jacobperron @clalancette
Lint
6f79f4f 
Signed-off-by: Jacob Perron <[email protected]>
@jacobperron @clalancette
Cleanup package.xml and CMakeLists.txt
7180857 
Remove unnecessary dependencies

Signed-off-by: Jacob Perron <[email protected]>
@jacobperron @clalancette
Update TODO
7cdf466 
Signed-off-by: Jacob Perron <[email protected]>
@jacobperron @clalancette
Lint and fix warning
e301055 
Signed-off-by: Jacob Perron <[email protected]>
@ivanpauno @clalancette
Remove completed TODO
5a6e355 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno @clalancette
Use create/destroy event function generated in rosidl_generator_c
54e65f3 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno @clalancette
Remove unneeded changes
b6863bb 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno @clalancette
Use common service event create/destroy function in cpp
c85bbba 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno @clalancette
fix
109f9b3 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno @clalancette
fixup test
1e90346 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno @clalancette
please linters
d9e1678 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno @clalancette
Add missing initializers
863030c 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno @clalancette
Don't use designated initializers in c++
2a44577 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno @clalancette
fix
7fe5d18 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno @clalancette
please uncrustify
7389d83 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno @clalancette
service typesupport struct field renaming
27bbb09 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno @clalancette
Complete testing
5190714 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno @clalancette
Fix windows CI
c71797f 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@clalancette
Switch to using the client_gid.
55745b0 
It should not be a UUID.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Small fixes to fix tests.
c3c80ad 
Signed-off-by: Chris Lalancette <[email protected]>
@clalancette clalancette force-pushed the service_introspection branch from 296d5f7 to c3c80ad Compare January 31, 2023 15:12
@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
@Crola1702 Crola1702 mentioned this pull request Feb 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivanpauno ivanpauno ivanpauno left review comments

@jacobperron jacobperron jacobperron left review comments

@wjwwood wjwwood wjwwood left review comments

@mjcarroll mjcarroll mjcarroll approved these changes

@quarkytale quarkytale Awaiting requested review from quarkytale

@sloretz sloretz Awaiting requested review from sloretz

Assignees

@jacobperron jacobperron

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ihasdapie @jacobperron @ivanpauno @clalancette @wjwwood @mjcarroll