Skip to content

Conversation

bjsowa
Copy link
Member

@bjsowa bjsowa commented Aug 29, 2025

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.

@EzraBrooks
Copy link
Contributor

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.

@bjsowa
Copy link
Member Author

bjsowa commented Aug 29, 2025

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

Copilot

This comment was marked as outdated.

Copy link
Contributor

@EzraBrooks EzraBrooks left a 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?

@bjsowa bjsowa requested a review from Copilot September 9, 2025 01:43
Copy link

@Copilot Copilot AI left a 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.

@bjsowa
Copy link
Member Author

bjsowa commented Sep 9, 2025

@EzraBrooks I simplified it a bit by skipping the "undefined check" (result.successful === false will return false if successful is undefined) and also changed it so that the normal callback is still called if result is unsuccessful and failedCallback is not provided to keep the old behavior. Let me know what you think

@bjsowa bjsowa requested a review from EzraBrooks September 9, 2025 01:54
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.

2 participants