-
Couldn't load subscription status.
- Fork 61
Updates to ROS2 support #143
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
Conversation
Co-authored-by: gonzalocasas <[email protected]>
Co-authored-by: gonzalocasas <[email protected]>
Co-authored-by: gonzalocasas <[email protected]>
|
@sea-bass @EzraBrooks could you also take a look at this one, pls? thx! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
As a nitpick, there are many comments throughout this PR that say "ROS2" instead of the preferred styling "ROS 2" with the space. If you'd like to update those, that would be appreciated.
src/roslibpy/core.py
Outdated
| Args: | ||
| request (:class:`.ServiceRequest`): Service request. | ||
| Args: | ||
| goal (:class:`.ActionGoal`): Action goal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's just .Goal in ROS 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise it's just .Request and .Response for services
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the action-related classes to remove the Action prefix at 7c52a8d
For now, if you agree, I would prefer to keep ServiceRequest and ServiceResponse names as they are to minimize the breaking changes.
If that's ok, then I will merge this PR
styling fixed at 2c1ee67 |
|
@sea-bass thanks!! merging! |
Before releasing 2.0, I wanted to clean up, and move some stuff around regarding the ROS2 action support:
wait_goalto wait for an action goalactionlibinsideroslibpy.ros1to avoid confusion with having to identically named things inroslibpyandroslibpy.actionlib, so, now usage of ROS1's actionlib is explicit. This is a breaking change.What type of change is this?
Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.CHANGELOG.rstfile in theUnreleasedsection under the most fitting heading (e.g.Added,Changed,Removed).invoke test).invoke check).