Skip to content
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

Get single parameter #245

Merged
merged 7 commits into from
Aug 2, 2016
Merged

Get single parameter #245

merged 7 commits into from
Aug 2, 2016

Conversation

rohbotics
Copy link
Contributor

@rohbotics rohbotics commented Jul 29, 2016

Added an API for getting a single parameter.

Note: this PR is based on master, and might conflict with my other PR

@wjwwood
Copy link
Member

wjwwood commented Jul 29, 2016

Should this function have an optional "default value" parameter? Or should we have a separate function that provides that syntax sugar?

@wjwwood
Copy link
Member

wjwwood commented Jul 29, 2016

Sorry, looks like merging the other pr makes this one conflict. You'll need to rebase it.

@rohbotics
Copy link
Contributor Author

@wjwwood Rebased

Do you think the default value should be on all three versions of the function, or just on the one that returns a parameter?

@dirk-thomas
Copy link
Member

Adding more functions (in this case three different signatures) might be helpful for the user but imo it increases the long term maintenance burden since actual functionality and syntactic sugar is mixed (as I mentioned in #233 (comment)).

From my point of view we should have one component with a minimal interface providing and implementing all the core functionality (call it a "micro kernel"). Any additional API for syntactic sugar should be separated.

@rohbotics
Copy link
Contributor Author

I thought that the minimal interface as supposed to be provided by rcl, and that rclcpp is the more user-friendly frontend.

If we want to keep the node class minimal, we could move all parameter functionality to the parameter_service, and have it have all the syntactic sugar.

That way any node that doesn't pull in the parameter_service isn't a parameter server, so doesn't have any internal map for storing parameters, which is currently in the node. We can also move the parameter_events publisher to the parameter_service, (Connects to #54).

The downside to this approach would be that in order to pass a handle around for creating publishers/subscribers and getting/setting parameters, you would need a pointer to both the parameter_service and the node. We could mitigate this by having the node store a smart_ptr to a passed in parameter_service, or have a function that makes a parameter_service internal to the node.

@rohbotics rohbotics closed this Jul 29, 2016
@rohbotics rohbotics reopened this Jul 29, 2016
@rohbotics
Copy link
Contributor Author

Would you say that maintainability would be increased if instead of the 3 signatures being completely independent/freestanding, they were composed on top of each-other?

@rohbotics rohbotics closed this Jul 29, 2016
@rohbotics rohbotics reopened this Jul 29, 2016
@rohbotics
Copy link
Contributor Author

Sorry, I hit the close and comment instead of the comment button

@wjwwood
Copy link
Member

wjwwood commented Jul 29, 2016

@rohbotics I have a different opinion from @dirk-thomas: this is the highest level interface and the appropriate place to have convenience functions. We can choose to back these functions with a minimal interface in the future if we choose. I do agree that you should try to have the functions call each other where possible. It looks like at least two of them are basically duplicates.

@rohbotics
Copy link
Contributor Author

How should I handle the locking between functions if they call each other?
Right now they all lock the same mutex, so if one of them locks it and then calls another, the callee will wait forever on the mutex, as the caller is holding it until it returns.

@tfoote
Copy link
Contributor

tfoote commented Jul 29, 2016

The higher level one does not need to lock if it's just calling the other one that locks.

@rohbotics
Copy link
Contributor Author

I made all the functions lock, then call a common non-locking private function.


rclcpp::parameter::ParameterVariant parameter_variant(parameter);
bool result = get_parameter_(name, parameter_variant);
parameter = parameter_variant.get_value<ParameterT>();
Copy link
Member

Choose a reason for hiding this comment

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

If result is false will this not raise?

Copy link
Contributor Author

@rohbotics rohbotics Jul 29, 2016

Choose a reason for hiding this comment

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

This should not throw if result == false
That enables use like this

double foo = 0.1;
if (!get_parameter("foo", foo) {
  set_parameter("foo", foo);
}

We do throw if get_value fails because of differing types.

Copy link
Member

Choose a reason for hiding this comment

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

But if result is false, then the ParameterVariant will be "not set" right? So get_value will raise I think (that was what I was referring to before).

Copy link
Member

Choose a reason for hiding this comment

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

After talking with @rohbotics I think I might be wrong about get_value throwing because the ParameterVariant is seeded with the given parameter value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The seeding doesn't compile with anything other than a string, so looks like I need to rethink this a little bit.

@wjwwood
Copy link
Member

wjwwood commented Jul 29, 2016

Also, I'd like to clarify that I agree with @dirk-thomas in that we need to restructure the Node class and make a separation between high level interfaces and the foundation of those interfaces. However, I see it as a larger task that goes beyond these couple of new functions, and that it shouldn't be a pre-requisite for opportunistically taking some contributions like this one.

@rohbotics
Copy link
Contributor Author

@wjwwood I cleaned up a little, and we don't need the non-locking private function

I added tests here ros2/system_tests#158 as we discussed

if (get_parameter(name, parameter)) {
return parameter;
} else {
throw std::out_of_range("Parameter: " + name + " does not exist in map");
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: would you change this to "Parameter '" + name + "' not set"? We usually wrap variables output into string inside of quotes in case name is an empty string, for example. Also, the fact that a map is used is an implementation detail.

@wjwwood
Copy link
Member

wjwwood commented Jul 29, 2016

Other than my comment, this lgtm. I still need to review the tests you added (thanks for those btw), and then run CI.

@rohbotics
Copy link
Contributor Author

I fixed the exception message

if (get_parameter(name, parameter)) {
return parameter;
} else {
throw std::out_of_range("Parameter '" + name + "' not set");
Copy link
Member

Choose a reason for hiding this comment

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

Also, out_of_range may not be appropriate here. Instead maybe a custom exception should be used... I think if this were a method on a "ParameterSet" or "ParameterMap" object it might make sense because out_of_range is the functional reason the parameter cannot be returned (the given key resulted in a "key error"), but it's not really the non-functional reason. In the context of a user asking for a parameter, it might make sense to throw a more domain specific exception type.

I've not convinced myself enough to suggest that you should change it right now, but maybe others can give their opinions.

@wjwwood
Copy link
Member

wjwwood commented Jul 29, 2016

I started CI for you:

  • Linux:
    • Build Status
  • OS X:
    • Build Status

@wjwwood
Copy link
Member

wjwwood commented Jul 29, 2016

@rohbotics looks like your new tests have some compiler warnings: http://ci.ros2.org/job/ci_linux/1678/warnings20Result/new/

@wjwwood
Copy link
Member

wjwwood commented Jul 29, 2016

I kicked off a new Linux CI to check the warnings:

  • Linux:
    • Build Status

@dirk-thomas
Copy link
Member

One new warning on Windows: http://ci.ros2.org/job/ci_windows/1609/warnings39Result/new/

@rohbotics
Copy link
Contributor Author

Fixed warning and test on Windows

@wjwwood, looks like on windows you have to use C++11's instead of C99's <stdint.h>, or you need a specific include order

@wjwwood
Copy link
Member

wjwwood commented Aug 2, 2016

@rohbotics Yeah, makes sense. CI looks good, thanks for following up on this.

@wjwwood wjwwood merged commit 902d558 into ros2:master Aug 2, 2016
Karsten1987 pushed a commit that referenced this pull request Dec 7, 2016
* added function to get parameter by exact name

* added ros1 style get_parameter for parameter variant

* added ros1 style get_param for non-variant types

* Make the get_parameter functions call a private base function

* Parameter Variant requires name in constructor

* Cleaned up to no longer need private function

* Made exception message more clear
nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022
DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Aug 5, 2022
…os2#245)

* Follow style conventions better and throw eagerly
* Use constant naming convention in ZstdCompressor

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
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.

4 participants