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

change parameter separator to forward slash #367

Merged
merged 3 commits into from
Sep 5, 2017
Merged

Conversation

Karsten1987
Copy link
Contributor

@Karsten1987 Karsten1987 commented Sep 1, 2017

changes:

  • change parameter separator from dot (.) to forward slash /
  • exclude lambda condition for visibility
  • use rcutils logging macros for warning

why?

this allows to correctly use the prefix when calling list_parameters

There are currently no unit-tests for rclcpp parameters (AFAICT). This should be ticketed separately.

@Karsten1987 Karsten1987 added the in progress Actively being worked on (Kanban column) label Sep 1, 2017
@Karsten1987 Karsten1987 self-assigned this Sep 1, 2017
@dhood
Copy link
Member

dhood commented Sep 1, 2017

is this change only necessary because of how you're setting the parameters (with a slash separating them instead of a dot)?
we have tests for list_parameters that work with the existing prefix, but only if you specify paramters using the same prefix.
If we make a change here it should be to make the prefix configurable, not switch from one arbitrary one to another IMO

@Karsten1987
Copy link
Contributor Author

Karsten1987 commented Sep 1, 2017

I would advocate for a convention on how to set the parameters (analog to how they are set in ROS1).
When loading a yaml file, e.g.

my_node:
  topics:
     sub: my_node_subscription
     pub: my_node_publication

I would set these parameters:

/my_node/topics/sub (= my_node_subsription)
/my_node/topics/pub (= my_node_publication)

But nevertheless, we could enhance the msg definition for separator field which may or may not default to some character.

@dhood
Copy link
Member

dhood commented Sep 1, 2017

I don't have an issue with it myself; others may have more context if there was a reason for the dots over slashes, but from the comment it seems like a placeholder.

Main point I wanted to make is that there is a convention at the moment (it's just a bit secret..) and list_parameters will only work with that specific one being used symmetrically. So this PR isn't fixing a broken list_parameters, per se, but instead is changing the convention to what ROS 1 users might expect. If we decide to go in that direction we have tests and demos that would need to be updated accordingly

@mikaelarguedas
Copy link
Member

Looks like the place holder was chosen when we didn't want to use slashes anywhere, now that we do through namespaces I'm not sure that's a blocker anymore. I think the placeholder was chosen in a Java style and didnt have a strong rationale behind it

@Karsten1987
Copy link
Contributor Author

updated the related tests.

CI:
Build Status
Build Status
Build Status

@Karsten1987 Karsten1987 added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Sep 2, 2017
Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -197,26 +198,27 @@ NodeParameters::list_parameters(const std::vector<std::string> & prefixes, uint6
std::lock_guard<std::mutex> lock(mutex_);
rcl_interfaces::msg::ListParametersResult result;

// TODO(esteve): define parameter separator, use "." for now
char separator = '/';
Copy link
Member

Choose a reason for hiding this comment

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

Could this be const?

@Karsten1987 Karsten1987 merged commit 2c5ab49 into master Sep 5, 2017
@Karsten1987 Karsten1987 deleted the list_parameters branch September 5, 2017 18:46
@Karsten1987 Karsten1987 removed the in review Waiting for review (Kanban column) label Sep 5, 2017
@mikaelarguedas
Copy link
Member

Follow up on this: It seems like the rationale for not using / is to avoid ambiguity (mentioned on the parameters design document as follow):

"
Address all parameters without ambiguity in the names
For example, one of the challenges of the current system is that there is a naming ambiguity between nodes and parameters /foo/bar/baz could be a node /foo/bar/baz or a private parameter baz on node /foo/bar.
"

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

* [rcl] Add test for copying arguments struct with no arguments

* Override allocate function in test to reveal bug

* [rcl] Only allocate arrays if there are things to copy in rcl_argument_copy()

Also guard against freeing invalid pointers if rcl_argument_copy() fails.

* Remove uncessary guard against NULL pointer
nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022
* Changing security directory lookup to a prefix match rather than exact match.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Changing security directory lookup to a prefix match rather than exact match.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Changing security directory lookup to a prefix match rather than exact match.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Changing security directory lookup to a prefix match rather than exact match.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Adding security_directory module and moving rcl_get_secure_root function to it. Adding tests.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Adding security_directory module and moving rcl_get_secure_root function to it. Adding tests.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Adding security_directory module and moving rcl_get_secure_root function to it. Adding tests.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Adding security_directory module and moving rcl_get_secure_root function to it. Adding tests.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Adding security_directory module and moving rcl_get_secure_root function to it. Adding tests.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Changing security directory prefix matching to be optional. Improving error messages around security directory lookup.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Fixing get_best_matching_directory so that it always fetches the next file inside the loop.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* make pr ready for ros2cli security feature (#1)

* update docs about possibility of rcl_take no taking (ros2#356)

* update rcl_wait doc with respect to subs and possibility of failing takes

* add a note about possible failing takes in rcl_take docs

* 0.6.2

* Set rmw_wait timeout using ros timers too (ros2#357)

* 0.6.3

* Avoid timer period being set to 0 (ros2#359)

* Fix logic that moves goal handles when one expires (ros2#360)

* Fix error from uncrustify v0.68 (ros2#364)

* Ensure that context instance id storage is aligned correctly (ros2#365)

* Ensure that context instance id storage is aligned correctly

* Make alignment compatible with MSVC

* Namespace alignment macro with RCL_

* [rcl] Guard against bad allocation calling rcl_arguments_copy() (ros2#367)

* [rcl] Add test for copying arguments struct with no arguments

* Override allocate function in test to reveal bug

* [rcl] Only allocate arrays if there are things to copy in rcl_argument_copy()

Also guard against freeing invalid pointers if rcl_argument_copy() fails.

* Remove uncessary guard against NULL pointer

* linter, styles, uncrustify fixes

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Update rcl/include/rcl/security_directory.h

Co-Authored-By: AAlon <avishayalon@gmail.com>
Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Adding line break in docstring

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Removing duplicate doc string

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Removing tinydir from the source tree, instead using the ROS package tinydir_vendor.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Removing tinydir

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Reformatting license notice as per linter template.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Update test_security_directory.cpp

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Changing input to putenv to be a global, statically allocated buffer.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* test_security_directory - Using a larger buffer for env string manipulations.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Copy environment variable to allocated string so it is not clobbered by next lookup

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Address review comments

fix security directory exact match comment and unset env vars before tests

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Remove strncpy

Signed-off-by: Emerson Knapp <eknapp@amazon.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