-
Notifications
You must be signed in to change notification settings - Fork 382
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
Add ROS 2 action support #645
Conversation
2fabf2e
to
0742dba
Compare
fa04cd7
to
a6ae65c
Compare
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.
looks good/sensical to me from a first pass, though I'll admit that my pre-class
-keyword JavaScript is a bit rusty. Just some minor nits that I think all relate to the waffling over keeping ros1 actionlib support earlier on.
I've tested this along with RobotWebTools/rosbridge_suite#886 and DefinitelyTyped/DefinitelyTyped#67407 in our UI and all seems to work in order. Inclined to merge this soon, as it looks ready to go now! |
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 have a few questions before merging this one:
- The ROS2 example, doesn't contain setting parameters. Is this still possible? In case it is, could we add it to the example?
- The changes in the rosbridge_server aren't released yet, correct? Therefore we can merge this one, but we shouldn't release it yet. Correct?
- This is not really a breaking change, but it only works with the ROS2 rosbridge_server correct? So therefore a major version bump might be needed. If that is the case, we should combine multiple breaking changes at once. (Releasing, not this PR)
I think it's still possible though the
Correct -- nothing is released yet, so we should wait until it's all merged in together for ROS 2. BTW for those using TypeScript, like ourselves, I also had to add these changes in support of this PR: DefinitelyTyped/DefinitelyTyped#67407 How does this play with having separate releases/branches between |
Update: Params did work, and I've added an example into the |
@sea-bass I think I want to make one final release for v1.X. I will rename the master branch to v1. I will also create a v2 branch, which acts as the release branch for all future v2.x releases. After which we should rebase this PR on the current develop branch. As the ros2 branch is just behind the develop branch. So we should also merge this PR into the develop branch. |
f77ef8a
to
afa5e4a
Compare
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 other than one question about not avoiding the type checker
7080bd9
to
1eb9666
Compare
1eb9666
to
34e4b81
Compare
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.
Code LGTM and we've been using this successfully in our project!
@MatthijsBurgh would you like to give this another look before we merge? All seems to work great for us, except that since we got rid of the build output a few days ago we can't |
Before creating a release, I suggest we figure out whether we want to provide compiled files attached to the release, etc. I will create an issue to discuss. |
I'm trying to run the ros2_action_server.html and I'm runnning into this: Any idea how to solve this issue? |
I think I ran into this at some point and resolved it with #692. Are you on a version of roslibjs with that commit? |
Yes I am. |
I tried a few more things in adjusting the package but it fails to run. I'm on Humble btw. |
I think with these questions going forward, it's best to open a new issue instead of jumping into a closed PR's discussion thread. And if you do, it might be helpful to provide more details on what you've tried and what error(s) you may be getting. It's a bit challenging to help otherwise. Setup should be done as in https://github.com/RobotWebTools/roslibjs?tab=readme-ov-file#usage. Basically just running Then, there are several examples in this folder: https://github.com/RobotWebTools/roslibjs/tree/develop/examples -- specifically, the ones prefixed with When you open those HTML files in a browser, they display instructions for how to start rosbridge in the background and start up certain ROS executables in the background to communicate with the example. |
This comment was marked as off-topic.
This comment was marked as off-topic.
@dineshHarikrishnan please open a new issue. PR's are not a place to discuss such issues. |
Public API Changes
This PR adds new ROS 2 functionality for actions, without touching any of the existing ROS 1 APIs.
This, plus the fact that CI currently is not running on any ROS 2 distros, does beg the question on what we should do with ROS 2... but I'll raise a different issue for this.
Description
This PR builds on the ROSBridge protocol updates in RobotWebTools/rosbridge_suite#886, to add official action support for ROS 2.
Notably, this doesn't rely on any hidden topics/services or extra logic on the
roslibjs
side. Instead,rosbridge_suite
is directly using therlcpy.actions
module which is way cleaner in my opinion.The examples have been updated to also work with ROS 2, though I haven't done anything with the unit tests / CI yet. I think the state of testing is such that we'd need a host of separate PRs to address this. including actually running CI on ROS 2.
Testing Instructions
npm install
/npm run build
)examples/ros2_action_client.html
andexamples/ros2_action_server.html
examples in your web browser and try them out, following the instructionsdemos
repo installed as well: https://github.com/ros2/demos -- it has binaries, so it may already be on your system... but just in case.