-
Notifications
You must be signed in to change notification settings - Fork 403
Handle rosapi param responses #930
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
base: develop
Are you sure you want to change the base?
Conversation
was that backported to humble? roslibjs only has one release channel so I don't think we should break API here if not until Humble is EOL. |
It was not backported but this should still be compatible with humble |
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.
agree with copilot about not duplicating the code three places, otherwise LGTM. I only skimmed it the first time and didn't notice that you wrote it in a back-compatible way by checking for the presence of the successful
member.
maybe we need to make failedCallback
mandatory in a future version, otherwise if someone promisifies this callback-style function this'll just hang forever?
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.
Pull Request Overview
Updates the Param class to handle the new response format from rosapi parameter services in ROS 2 Jazzy, which now includes a successful
boolean field and optional reason
for failures.
- Added response validation checking
result.successful
before processing parameter operations - Updated error handling to use the service-provided
reason
field when operations fail - Modified documentation to clarify that failed callbacks now handle both service call failures and unsuccessful parameter operations
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@EzraBrooks I simplified it a bit by skipping the "undefined check" ( |
Public API Changes
None
Description
Thanks to RobotWebTools/rosbridge_suite#1001, since ROS 2 Jazzy, rosapi param services return the
successful
bool and fill the reason for failure if the operation does not succeed.This PR adds the logic for handling these additional fields.