Skip to content

Separate declare and get params #1

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Separate declare and get params #1

wants to merge 19 commits into from

Conversation

saikishor
Copy link
Owner

No description provided.

@saikishor saikishor mentioned this pull request Oct 30, 2023
Closed
christophfroehlich
christophfroehlich reviewed Oct 31, 2023
View reviewed 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.

Looks clean, and it should be compatible with existing code?!

...ameter_library_py/generate_parameter_library_py/jinja_templates/cpp/parameter_library_header

private:
void update_interal_params(Params updated_params) {
std::lock_guard<std::mutex> lock(mutex_);
params_ = updated_params;
}

Params get_internal_params_copy() const{
std::lock_guard<std::mutex> lock(mutex_);
Copy link
Collaborator

Choose a reason for hiding this comment

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

so we need the mutex here?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think as we are accessing the the params_ variable which could be modified by the callback at the same time

@saikishor @christophfroehlich
apply formatting suggestions
52a14cc 
Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
@saikishor
Copy link
Owner Author

Looks clean, and it should be compatible with existing code?!

@christophfroehlich I ran the tests after compiling with this library, in principle they are passing on the ros2_control and ros2_controllers side. I tried to leave it backward compatible. That's why I left it to get the parameters on construction by default

@saikishor
revert the changes
2281dd2
@saikishor
add latest_stamp variable to track the changes of the parameters
46c0270
@saikishor
update get_params method to get the declared params locally
3ee6864
@saikishor
separate dynamic parameters declaration and parameter value retrieval
5bf0775
@saikishor
update parameters in update cb only when there is a validation present
dc4d5a0
@saikishor
always have normal parameter as they might be useful for map of dynam…
2a7a2ec 
…ic parameters
@saikishor
remove the unused update_interal_params method and internal params va…
1648e53 
…riable
@saikishor
deprecate the refresh_dynamic_parameters method
4e557b3
@saikishor
remove timestamp update from the declare_params method
dc8500d
@saikishor
throw exception if the params are not loaded yet and get_params is in…
7651f1c 
…voked
@saikishor
don't perform validation in the get_params method
a89d00f
@saikishor
use Params object instead of get_params method in the update callback
acfbd73
@saikishor
use internal methods to update the param state for update callback
aa207d6
@saikishor
optimize get_params to use internal variable with some logic
9ecb96f
@saikishor
retrieve the main dynamic map parameter parent params and pass to set…
62383cb 
…_parameter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christophfroehlich christophfroehlich christophfroehlich left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@saikishor @christophfroehlich