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

Clarify docs on 'mapped parameters' in README #66

Merged
merged 2 commits into from
Sep 28, 2021

Conversation

hello-binit
Copy link

This PR rewords the README to clarify that the parameters zeros and dependent_joints don't actually take in dictionaries, but instead follow the <parameter>.<key>:=<value> format. It also points out that ros2 param list won't show these two params, but that they are still supported. This PR closes #65.

If this change to the README looks good, I can copy it to a wiki. Is there a ROS2 equivalent for the joint_state_publisher ROS wiki?


#### Mapped Parameters

These parameters won't show up under '/joint_state_publisher' with `ros2 param list`, because they are declared differently than the other parameters. For a dictionary, the format to use these parameters is `<parameter>.<key>:=<value>`, where a new parameter is defined for each key. See below for examples.
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this isn't totally true, or at least it is not meant to be true. If you just run:

ros2 run joint_state_publisher joint_state_publisher

it is true that there will be no declared zero.<joint> parameters. But that's because we don't know what the joints might be.

Instead, if you run:

ros2 run joint_state_publisher joint_state_publisher --ros-args -p zeros.joint2:=3.0

Then the zeros.joint2 parameter will indeed show up in ros2 param list /joint_state_publisher.

Theoretically, at the end of configure_robot, we could do a declare_parameter('zeros.<jointname>', 0.0) for every possible joint that we see in the incoming URDF. But that would mean that on a large robot we would have hundreds of parameters, most of which will never be used. I'm not sure if that is worth it or not.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed, that phrase wasn't entirely true. To make it unambiguous that these parameters exist, I've added a commit that declares the zeros and dependent_joints parameters as NOT_SET. The node prints warnings when attempting to interact with these parameters directly instead of using <parameter>.<key>. The README removes the not true statement.

This has the benefit of being able to explore the node's parameters using the cli. In the future, these parameters could have descriptions.

I agree that it's likely not worth declaring every parameter from the URDF; the large printout would make it harder to read the rest of the parameters.

If you think this change doesn't make sense, let me know and I can revert it.

Copy link
Collaborator

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

@hello-binit Thanks for the update! The change to the README looks good.

As for the code change, I'm not inclined to add a parameter that is "fake" and doesn't really do anything. So please remove that change for now.

@hello-binit
Copy link
Author

Sure, makes sense. It's removed @clalancette.

@clalancette
Copy link
Collaborator

Thanks for the PR, merging. I'm also going to forward-port this onto the galactic and ros2 branches.

@clalancette clalancette merged commit f8d73b9 into ros:foxy Sep 28, 2021
clalancette pushed a commit that referenced this pull request Sep 28, 2021
* Clarify docs on 'mapped parameters' in README
clalancette pushed a commit that referenced this pull request Sep 28, 2021
* Clarify docs on 'mapped parameters' in README
clalancette added a commit that referenced this pull request Sep 28, 2021
* Clarify docs on 'mapped parameters' in README

Co-authored-by: Binit Shah <bshah@hello-robot.com>
clalancette added a commit that referenced this pull request Sep 28, 2021
* Clarify docs on 'mapped parameters' in README

Co-authored-by: Binit Shah <bshah@hello-robot.com>
@hello-binit hello-binit deleted the patch-1 branch September 28, 2021 17:13
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.

None yet

2 participants