Get single parameter #245

rohbotics · 2016-07-29T00:16:25Z

Added an API for getting a single parameter.

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

wjwwood · 2016-07-29T01:00:17Z

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

wjwwood · 2016-07-29T01:06:08Z

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

rohbotics · 2016-07-29T01:28:13Z

@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 · 2016-07-29T15:45:18Z

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 · 2016-07-29T17:06:32Z

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 · 2016-07-29T17:28:02Z

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 · 2016-07-29T17:28:49Z

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

wjwwood · 2016-07-29T17:57:52Z

@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 · 2016-07-29T18:11:21Z

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 · 2016-07-29T18:19:24Z

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

rohbotics · 2016-07-29T18:44:42Z

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

wjwwood · 2016-07-29T20:10:06Z

rclcpp/include/rclcpp/node_impl.hpp

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


If result is false will this not raise?

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.

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).

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

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

wjwwood · 2016-07-29T20:23:36Z

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 · 2016-07-29T21:58:03Z

@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

wjwwood · 2016-07-29T22:06:33Z

rclcpp/src/rclcpp/node.cpp

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


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 · 2016-07-29T22:07:34Z

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

rohbotics · 2016-07-29T22:11:37Z

I fixed the exception message

wjwwood · 2016-07-29T22:17:28Z

rclcpp/src/rclcpp/node.cpp

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


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 · 2016-07-29T22:25:57Z

I started CI for you:

Linux:
OS X:

wjwwood · 2016-07-29T23:00:10Z

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

wjwwood · 2016-07-29T23:23:59Z

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

Linux:

dirk-thomas · 2016-07-30T00:30:11Z

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

rohbotics · 2016-07-30T03:54:32Z

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 · 2016-08-02T02:49:05Z

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

* 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

…os2#245) * Follow style conventions better and throw eagerly * Use constant naming convention in ZstdCompressor Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

rohbotics force-pushed the get_parameter branch from d009874 to cd2e7ba Compare July 29, 2016 01:12

Rohan Agrawal added 3 commits July 28, 2016 18:23

added function to get parameter by exact name

b947019

added ros1 style get_parameter for parameter variant

217789a

added ros1 style get_param for non-variant types

4cf24e1

rohbotics force-pushed the get_parameter branch from cd2e7ba to 4cf24e1 Compare July 29, 2016 01:24

rohbotics closed this Jul 29, 2016

rohbotics reopened this Jul 29, 2016

rohbotics closed this Jul 29, 2016

rohbotics reopened this Jul 29, 2016

Make the get_parameter functions call a private base function

f501837

wjwwood reviewed Jul 29, 2016
View reviewed changes

Rohan Agrawal added 2 commits July 29, 2016 14:41

Parameter Variant requires name in constructor

1e3812b

Cleaned up to no longer need private function

5beb5eb

rohbotics mentioned this pull request Jul 29, 2016

Add tests for getting single parameter from node ros2/system_tests#158

Merged

wjwwood reviewed Jul 29, 2016
View reviewed changes

Made exception message more clear

4ddc224

wjwwood reviewed Jul 29, 2016
View reviewed changes

wjwwood merged commit 902d558 into ros2:master Aug 2, 2016

nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022

Initialize seq_data_type (ros2#245)

95b4147

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get single parameter #245

Get single parameter #245

rohbotics commented Jul 29, 2016 •

edited

Loading

wjwwood commented Jul 29, 2016

wjwwood commented Jul 29, 2016

rohbotics commented Jul 29, 2016

dirk-thomas commented Jul 29, 2016

rohbotics commented Jul 29, 2016

rohbotics commented Jul 29, 2016

rohbotics commented Jul 29, 2016

wjwwood commented Jul 29, 2016

rohbotics commented Jul 29, 2016

tfoote commented Jul 29, 2016

rohbotics commented Jul 29, 2016

wjwwood Jul 29, 2016

rohbotics Jul 29, 2016 •

edited

Loading

wjwwood Jul 29, 2016

wjwwood Jul 29, 2016

rohbotics Jul 29, 2016

wjwwood commented Jul 29, 2016

rohbotics commented Jul 29, 2016

wjwwood Jul 29, 2016

wjwwood commented Jul 29, 2016

rohbotics commented Jul 29, 2016

wjwwood Jul 29, 2016

wjwwood commented Jul 29, 2016

wjwwood commented Jul 29, 2016

wjwwood commented Jul 29, 2016

dirk-thomas commented Jul 30, 2016

rohbotics commented Jul 30, 2016

wjwwood commented Aug 2, 2016

Get single parameter #245

Get single parameter #245

Conversation

rohbotics commented Jul 29, 2016 • edited Loading

wjwwood commented Jul 29, 2016

wjwwood commented Jul 29, 2016

rohbotics commented Jul 29, 2016

dirk-thomas commented Jul 29, 2016

rohbotics commented Jul 29, 2016

rohbotics commented Jul 29, 2016

rohbotics commented Jul 29, 2016

wjwwood commented Jul 29, 2016

rohbotics commented Jul 29, 2016

tfoote commented Jul 29, 2016

rohbotics commented Jul 29, 2016

wjwwood Jul 29, 2016

Choose a reason for hiding this comment

rohbotics Jul 29, 2016 • edited Loading

Choose a reason for hiding this comment

wjwwood Jul 29, 2016

Choose a reason for hiding this comment

wjwwood Jul 29, 2016

Choose a reason for hiding this comment

rohbotics Jul 29, 2016

Choose a reason for hiding this comment

wjwwood commented Jul 29, 2016

rohbotics commented Jul 29, 2016

wjwwood Jul 29, 2016

Choose a reason for hiding this comment

wjwwood commented Jul 29, 2016

rohbotics commented Jul 29, 2016

wjwwood Jul 29, 2016

Choose a reason for hiding this comment

wjwwood commented Jul 29, 2016

wjwwood commented Jul 29, 2016

wjwwood commented Jul 29, 2016

dirk-thomas commented Jul 30, 2016

rohbotics commented Jul 30, 2016

wjwwood commented Aug 2, 2016

rohbotics commented Jul 29, 2016 •

edited

Loading

rohbotics Jul 29, 2016 •

edited

Loading