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

Add ros2 action client capability #813

Closed
wants to merge 37 commits into from
Closed

Conversation

sathak93
Copy link

@sathak93 sathak93 commented Oct 16, 2022

Public API Changes

3.3.10 Send Goal

{ "op": "send_goal",
  (optional) "id": <string>,
  "action_name": <string>,
  "action_type": <string>,
  (optional) "feedback": <bool>,
  (optional) "goal_msg": <list<json>>
}

This command creates an action client with the specified action name and type if not already created; sends the goal msg to the action server and returns the feedback and result of the goal.

  • action_name – the name of the action to which the goal will be sent (ex. /navigate_to_pose)
  • action_type – the type of the action (ex. /nav2_msgs/action/NavigateToPose)
  • feedback – an optional boolean specifies whether to receive the feedback for this goal or not. defaults to true)
  • goal_msg – if the Goal has no args, then goal msg does not have to be
    provided, though an empty list is equally acceptable. goal msg should be a list of json objects representing the arguments to the action
  • id – an optional id to distinguish this goal call

3.3.11 Cancel Goal

{ "op": "cancel_goal",
  (optional) "id": <string>,
  "action_name": <string>
}

This command cancels all the executing and pending goals of the specified action and return the result of cancel call.

  • action_name – the name of the action (ex. /navigate_to_pose)
  • id – an optional id to distinguish this cancel goal call.

3.3.12 Destroy Client

{ "op": "destroy_client",
  (optional) "id": <string>,
  "action_name": <string>
}

This command destroys the action client if the action client was created on earlier send_goal calls.

  • action_name – the name of the action (ex. /navigate_to_pose)
  • id – an optional id to distinguish this cancel goal call.

Description
This pr adds ros2 action client capability using the direct approach. now clients can create action clients, send goal to get result and feedback, cancel goal, destroy client.

this address #697

@sathak93
Copy link
Author

@nakai-omer you are using actionclient in roslibjs which only works for ros1 as it simply subscribes to feedback and result and publish goal topics. for ROS 2 action client is not added yet . you can use the code in the folder ros2action on my fork https://github.com/sathak93/roslibjs/tree/ros2actionclient. once this PR is finalized and added to rosbridge_server . pr for client side implementation can be added.

@nakai-omer
Copy link

@sathak93 Got it thanks a lot!

@amalnanavati
Copy link

@sathak93 Your rosbridge_suite code and roslibjs code was a lifesaver! Looking forward to this being incorporated into the official releases. 🙌

@ahoenerBE
Copy link

ahoenerBE commented Jun 6, 2023

Is this PR still active? It's been a while and seems like it'd be a good addition

@sathak93
Copy link
Author

@ahoenerBE and @amalnanavati it is still in good condition i think. rosbridge doesn't have action capability in ros 2 only. Most of the current users of rosbridge are using ROS 1 i think thats why they doesn't know they miss out important feature in ros2 . This PR will increase code base. if it is okay with maintainers then we can review and merge

I believe that the best way to add action support is to do that on the client side. Roslibjs for instance provides an ActionClient and a SimpleActionServer, not sure if they work for ros2 actions though. Overall I would prefer to keep the rosbridge backend lean and maintanable.

@mjforan
Copy link

mjforan commented Aug 25, 2023

I need this capability as well.

@defunctzombie Can we get another review of this?

@sea-bass
Copy link
Contributor

sea-bass commented Oct 6, 2023

+1 -- we are looking to add ROS 2 Action Server support soon and would love for this PR to be in first as well.

@ahoenerBE
Copy link

ahoenerBE commented Oct 6, 2023

Reading #697 and seeing that actions and services are implemented on top of topics I am not clear on what capability this adds that could not be accomplished but it does increase the maintenance surface area.

I believe that the best way to add action support is to do that on the client side. Roslibjs for instance provides an ActionClient and a SimpleActionServer, not sure if they work for ros2 actions though. Overall I would prefer to keep the rosbridge backend lean and maintanable.

I appreciate and understand the desire to keep the codebase maintainable, as too many changes can create unneeded bloat, but I don't understand the pushback for supporting a core feature of ROS2. Sure, it's publishers and service calls under the hood, but that way of thinking requires each library in different languages to write some sort of adapter for ROS2 actions themselves.
It's creating much more bloat in 10 other projects to save a bit of code in the core one.

And for that matter, if actions are ever changed, then each of those projects will have to update their implementation to restore functionality, instead of the base rosbridge being updated and having the spec maintain functionality for all libraries that interact with it automatically.

I think it makes much more sense to add something that can natively interact with the ROS2 action server, and provide the standard for other libraries to accept, rather than making each different library do their own thing

@sea-bass
Copy link
Contributor

sea-bass commented Oct 6, 2023

Most of the current users of rosbridge are using ROS 1

I think it stems from this. In ROS 1 you could interact with actions using their underlying topics, but as far as I know that is not the case in ROS 2 and we do need to use the action interface.

@amalnanavati
Copy link

Yup, ROS2 implements actions using hidden services/topics, and the service/message types are not easily introspectable (relevant design doc). Both those factors prevented me from being able to interact with ROS2 actions using topic/service calls in roslibjs, which is why I think we need functionality on the rosbridge level directly.

@okahilak
Copy link

okahilak commented Oct 9, 2023

+1 to supporting ROS2 actions in rosbridge and getting this patch forward. Not being able to use actions in the front-end is currently a major blocker in our project, and as @amalnanavati pointed out, actions are not easily doable in ROS2 using topic/service calls.

@sea-bass
Copy link
Contributor

BTW I am going to work on action server functionality fairly soon (in the next few weeks), so it would be fantastic if this got merged in. I'm happy to also help write unit tests for this action client functionality as it's something we'll likely want at some point too.

Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

Looks good, though I didn't test.

Would be great if you also got to work on some unit tests (I just ported all the pub/sub ones to ROS 2 in #882 if you want a reference)... but I'm also happy to help do this after this one is merged.

BTW I am only reviewing as an interested party, sadly I don't have merging power here.

ROSBRIDGE_PROTOCOL.md Outdated Show resolved Hide resolved
ROSBRIDGE_PROTOCOL.md Outdated Show resolved Hide resolved
from rosbridge_library.internal.message_conversion import extract_values


class ActionClientRequests(Capability):
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem like it's named consistently as a "capability" compared to the others.

I think maybe this should be called SendActionGoal or something?

@defunctzombie
Copy link
Contributor

@sea-bass 👋 Good seeing you at roscon. Since Foxglove doesn't leverage rosbridge anymore, our support of this package has waned. You mentioned that PickNik relies on rosbridge for some products so I am open to adding you as a maintainer in Github to let you all carry the torch forward.

@sea-bass
Copy link
Contributor

@sea-bass 👋 Good seeing you at roscon. Since Foxglove doesn't leverage rosbridge anymore, our support of this package has waned. You mentioned that PickNik relies on rosbridge for some products so I am open to adding you as a maintainer in Github to let you all carry the torch forward.

This would be great, thank you for the offer -- could we also add my colleague @EzraBrooks, who is also a contributor to this repo?

@ahoenerBE
Copy link

Since Foxglove doesn't leverage rosbridge anymore, our support of this package has waned.

@sea-bass @defunctzombie Is the recommended bridge node now the foxglove ws-protocol if support here is waning? I want to make sure our projects are using something that will be updated in the future

@sea-bass
Copy link
Contributor

sea-bass commented Oct 24, 2023

I'd defer to Foxglove's answer, but from what I heard at ROSCon Foxglove uses a different protocol which means you can definitely use it on your end -- it is open-source -- but you'll need to modify/rewrite your own client libraries.

At least for the foreseeable future, PickNik plans to continue using this protocol (just the ROS 2 branch though), and we're happy to help maintain.

@ahoenerBE
Copy link

I'd defer to Foxglove's answer, but from what I heard at ROSCon Foxglove uses a different protocol which means you can definitely use it on your end -- it is open-source -- but you'll need to modify/rewrite your own client libraries.

At least for the foreseeable future, PickNik plans to continue using this protocol (just the ROS 2 branch though), and we're happy to help maintain.

@sea-bass That's good to know. I'm maintaining rosreact, which is a react hooks wrapper around roslibJS and it'd be unfortunate to have to rewrite it all for a new comms protocol. I'm also open to helping support, if people are wanting to keep this package alive.

@amacneil
Copy link
Member

@sea-bass @EzraBrooks I sent you both invites to this github org.

As for the ROS-Foxglove Bridge, we designed it to avoid performance and other issues we were seeing with rosbridge. It's open source, but we don't recommend it for general purpose (i.e. non-Foxglove related) use.

@sea-bass
Copy link
Contributor

sea-bass commented Oct 31, 2023

@sathak93 I didn't want to make you write a ton of unit tests, so wanted to share that I'm taking pieces from the great work you've done here and implementing them a little different + with tests + also adding action servers in #886

If you have any inputs please let me know.

Also, my plan is to try do the roslibjs changes next, and I saw you have a nice branch there. Do you mind if I try work with that as well once I get there?

Thank you!

@sea-bass
Copy link
Contributor

Closing in favor of #886, which supports both action servers and clients and is now merged!

Thanks again for your reference client implementation, @sathak93 -- it was very useful to my implementation and you should 100% get credit too.

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

10 participants