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

Extend CLI daemon's API #493

Merged
merged 6 commits into from
Apr 27, 2020
Merged

Extend CLI daemon's API #493

merged 6 commits into from
Apr 27, 2020

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Apr 22, 2020

Split from #421. This pull request extends the CLI daemon API to better match that of a regular rclpy.node.Node.

@hidmic
Extend CLI daemon's API further.
2889c28 
Support ROS graph API for actions plus basic node queries.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Fix NodeStrategy __getattr__ bug.
a074011 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Add xmlrpc marshalling for custom rclpy types.
66bc686 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Add daemon API tests.
385281b 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
dirk-thomas
dirk-thomas reviewed Apr 22, 2020
View reviewed changes
Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

From reading the patch it is unclear to me why all the marshaling logic is needed / where it is being used?

ros2cli/package.xml Outdated Show resolved Hide resolved
ros2cli/ros2cli/daemon/__init__.py Outdated Show resolved Hide resolved
ros2cli/ros2cli/node/strategy.py Show resolved Hide resolved
ros2cli/ros2cli/xmlrpc/__init__.py Outdated Show resolved Hide resolved
ros2cli/ros2cli/xmlrpc/__init__.py Outdated Show resolved Hide resolved
ros2cli/ros2cli/xmlrpc/marshal/__init__.py Outdated Show resolved Hide resolved
This was referenced Apr 22, 2020
Merged
Closed
@hidmic
Copy link
Contributor Author

hidmic commented Apr 22, 2020 •
edited
Loading

From reading the patch it is unclear to me why all the marshaling logic is needed / where it is being used?

rclpy.node.Node API for graph introspection now includes some custom types that xmlrpc is unable to (un)marshal on its own e.g. TopicEndpointInfo and QoSProfile. This logic adds support for them. I have to say I'm not particularly fond of xmlrpc means to register such logic (it's actually quite limited), but that's how it is.

@hidmic
Address peer review comments.
7750f7c 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Minimize intermediate imports
e923320 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Apr 24, 2020 •
edited
Loading

CI up to ros2cli:

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

@hidmic hidmic merged commit fd48f11 into master Apr 27, 2020
@delete-merged-branch delete-merged-branch bot deleted the hidmic/extend-daemon-api branch April 27, 2020 18:47
@dirk-thomas dirk-thomas mentioned this pull request May 4, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dirk-thomas dirk-thomas dirk-thomas approved these changes

@jacobperron jacobperron Awaiting requested review from jacobperron

@ivanpauno ivanpauno Awaiting requested review from ivanpauno

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hidmic @dirk-thomas