Skip to content

Conversation

saikishor
Copy link
Contributor

@saikishor saikishor commented May 4, 2025

Only return true when the parameters are modified and return false for others cases
I've also modified it to use the unique_lock as it is much better than try_lock on the mutex itself.

Related to ros-controls/ros2_controllers#1198 (review)

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.

Apologies for the delay -- I'm not really on these repos much these days.

Given this is a change in behavior, I don't feel like I should have the authority to make that decision. I'm going to leave this up to @pac48 as an original author of the package.

@christophfroehlich
Copy link
Collaborator

This was introduced with #205, but as discussed in the linked PR I think that it should have a different behavior. We could also deprecated the current one and add the proposed method with a different name if you prefer.

@pac48
Copy link
Contributor

pac48 commented May 26, 2025

@sea-bass I think the change in behavior makes sense in this case. I think we should merge this.

@nbbrooks
Copy link
Member

nbbrooks commented Jun 3, 2025

There are a couple other options I think we should consider here.

1. Make no change, rely on user library to check if __stamp changed to determine if the parameters it once held were old

Perhaps I don't understand the use case here and this is not a solution?

2. Add a new function for the alternative true/false return possibility

Keep the original try_get_params behavior (maybe mark it deprecated) and introduce a new function with the desired, new true/false behavior. Perhaps try_get_updated_params?

Add some brief doxygen to each function to delineate the true/false behavior?

try_get_param - true: your parameters are now up to date

  • true: Acquired the lock and either your params were already up-to-date OR your params are now up-to-date.
  • false: Failed to acquire the lock

try_get_updated_params

  • true: Acquired the lock and updated your params to newer values
  • false: Failed to acquire the lock OR there were not newer values (try_get_params would be a confusing name for this behavior IMHO)

@christophfroehlich
Copy link
Collaborator

christophfroehlich commented Jun 7, 2025

  1. Make no change, rely on user library to check if __stamp changed to determine if the parameters it once held were old

Perhaps I don't understand the use case here and this is not a solution?

Isn't this what the is_old method is doing?
https://github.com/PickNikRobotics/generate_parameter_library?tab=readme-ov-file#dynamic-parameters

  1. Add a new function for the alternative true/false return possibility

Keep the original try_get_params behavior (maybe mark it deprecated) and introduce a new function with the desired, new true/false behavior. Perhaps try_get_updated_params?

Add some brief doxygen to each function to delineate the true/false behavior?

try_get_param - true: your parameters are now up to date

* true: Acquired the lock and either your params were already up-to-date OR your params are now up-to-date.

* false: Failed to acquire the lock

try_get_updated_params

* true: Acquired the lock and updated your params to newer values

* false: Failed to acquire the lock OR there were not newer values (`try_get_params` would be a confusing name for this behavior IMHO)

I'm not sure if deprecating the old method is necessary, as it was never advertised in the documentation. But I agree with your arguments and the suboptimal naming for the new behavior. @saikishor do you have time to implement that, or shall I open a new PR?

@saikishor saikishor changed the title Improve try_get_params method Add try_update_params method Jun 7, 2025
@saikishor
Copy link
Contributor Author

@christophfroehlich I can take care of it. I just pushed some changes

Copy link
Collaborator

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

Great. Let us also add both methods to the README..

@nbbrooks nbbrooks merged commit efef248 into PickNikRobotics:main Jun 11, 2025
7 checks passed
@nbbrooks
Copy link
Member

Thank you for the updates.

@christophfroehlich since we added the new method as opposed to changing the ABI, I assume we should get this PR into the next release? I am going to work on that in the morning, sorry it is taking so long.

@saikishor saikishor deleted the improve/try_get_params branch June 11, 2025 05:01
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants