Skip to content

Result.msg for Service Responses #279

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

Open
wants to merge 3 commits into
base: rolling
Choose a base branch
from
Open

Result.msg for Service Responses #279

wants to merge 3 commits into from

Conversation

DLu
Copy link
Contributor

@DLu DLu commented Apr 30, 2025

This is the definition of Result.msg as it is defined in ros-simulation/simulation_interfaces#1

From the README:

The standard includes a generic message for handling service responses, Result.msg, which will likely not be included in the final version of the standard. Since it is generic, it will either be promoted to a common message or included in the service mechanism itself.

This PR is for discussing its promotion to common_interfaces in std_msgs in some future distro.

The other option is it gets integrated into the service mechanism itself, as it was in ROS 1.

@mjcarroll
Copy link
Member

Added to PMC agenda for May 6th.

# (e.g. SpawnEntity) to provide a return code which is more specific.

uint8 result # Result to be checked on return from service interface call
string error_message # Additional error description when useful.
Copy link
Collaborator

Choose a reason for hiding this comment

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

result and constants looks simulator specific, i do not think those are generic service response message in system layer. btw, do we not have something like error_message description when the service server returns the error? if that is missing, i think that would be useful for system? (user can leave it empty if they do not use it?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

result and constants looks simulator specific

As discussed in the PMC, I copied it over "as-is". It's updated now.

btw, do we not have something like error_message description when the service server returns the error? i

That existed in ROS 1 but cannot exist in ROS 2 because of the DDS spec (someone else can probably back it up with more precise language)

@tfoote
Copy link
Contributor

tfoote commented May 6, 2025

Overall I think that adding this sort of message is a good idea. We'll need to evolve this to be slightly more generic than the current simluation specific descriptions but we've seen similar patterns in other places like nav2 as well

https://github.com/ros-navigation/navigation2/blob/4c8e23cb0ff391dce1f9d2fa00fda2f2cd36f27b/nav2_msgs/srv/LoadMap.srv#L6-L15

With the goal of not reinventing the wheel. As we look to make this slightly more canonical there's a relatively well established set of error codes used in Abseil and gRPC

https://abseil.io/docs/cpp/guides/status-codes
https://github.com/googleapis/googleapis/blob/master/google/rpc/code.proto

As a first suggestion I might suggest that we leverage this existing standard instead of making a new one.

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-pmc-minutes-for-may-6-2025/43576/1

@gavanderhoorn
Copy link

Wanted to add a textual +1: I've used a similar message/structure in many projects, especially when error recovery is (at least partially) delegated to/implemented by machines.

It also aligns with how many RPC systems work, especially in more traditional and established fields.

re: fields: error messages are nice, for humans.

Error codes make things like state machines and behaviour trees -- and the humans implementing/modelling them -- much happier.

(although perhaps with LLMs we could argue error messages could also be used now, but that doesn't mean we can't cater to both)

The current set of constants is of course specific to what was discussed in ros-simulation/simulation_interfaces#1, but that can be changed and should not be a blocker here.

re: constant values: I second @tfoote's suggestion to try and reuse an existing specification -- although perhaps not necessarily limiting candidates to two Google projects.

I would also suggest making result a signed int, allowing for negative numbers, and making it 32bit (at least) to allow sufficient space for defining user/OEM-defined status/error messages, in addition to reserved values defined by the message/spec itself.

Could even be nice to introduce categories akin to HTTP response codes (2xx, 3xx, etc). Grouping can make handling errors even more convenient, and standardised groups should allow reuse of code, or at least of experience/best-practices.

@mjcarroll
Copy link
Member

Could even be nice to introduce categories akin to HTTP response codes (2xx, 3xx, etc). Grouping can make handling errors even more convenient, and standardised groups should allow reuse of code, or at least of experience/best-practices.

I will note that the rpc.proto codes also have equivalent HTTP mappings, which is helpful for translating between ecosystems.

DLu and others added 2 commits May 12, 2025 15:23
Co-authored-by: G.A. vd. Hoorn <[email protected]>
Signed-off-by: David V. Lu!! <[email protected]>
@DLu
Copy link
Contributor Author

DLu commented May 12, 2025

I have taken the suggestions, and rewritten the message with

  • less simulator specifics
  • int32 everywhere instead uint8
  • Multiple "classes" of errors
  • Inspiration from existing standards.

Generally, I was using the code.proto definition as a guideline. However, I didn't include every value. Here's what I left off.

  • UNAUTHENTICATED seemed too similar to PERMISSION_DENIED
  • RESOURCE_EXHAUSTED seemed unclear in this context
  • ABORTED seemed to similar to CANCELLED
  • DATA_LOSS didn't seem to apply here.

Could even be nice to introduce categories akin to HTTP response codes (2xx, 3xx, etc).

I now have two separate blocks for Client and Server errors. I would have liked to make it so that there was a particular bit set for each, but in the negative number space, I feel like that takes more mental load than its worth to just do a range check.

@zakmat
Copy link

zakmat commented May 13, 2025

Will this promotion be a part of the upcoming Kilted release?

@DLu
Copy link
Contributor Author

DLu commented May 13, 2025

Will this promotion be a part of the upcoming Kilted release?

No, this is being planned for a future release.

@wjwwood wjwwood added the help wanted Extra attention is needed label May 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants