Skip to content

Update README to reflect changes to config parameters. (fixes #23)#24

Merged
clalancette merged 2 commits into
ros2:foxyfrom
joshnewans:ros2-param-docs
Jan 11, 2021
Merged

Update README to reflect changes to config parameters. (fixes #23)#24
clalancette merged 2 commits into
ros2:foxyfrom
joshnewans:ros2-param-docs

Conversation

@joshnewans
Copy link
Copy Markdown

I have updated the README to reflect the new parameters, and the new defaults.

In doing so a few issues arose, some of which I have acted on and some I have left.

  1. I've rearranged the overall order of the parameters to be a bit more natural (axis, scale, scale_turbo; linear then angular). This reflects how it has been written in the config files too (though not in the code itself).
  2. Rather than making the parameter list three times as long with no clarity, I have grouped the parameters which are part of a struct. This was the simplest way I could think of to do it, please let me know if you have a better idea.
  3. In doing the above, I have introduced some ambiguity with the word axis (to refer to motion/coordinate axes as well as joystick axes). I think this is an inevitable consequence of a program that maps from axis to axis, and is clear enough from context.
  4. I think it would perhaps be clearer to order the angular axes as roll,pitch,yaw rather than yaw,pitch,roll. I presume this has arisen from the original one only having x and yaw, and the other axes being added later. But now that we can map to all six axes, it seems confusing to me that the order here is different from the order that you then see in the Twist message. I've opted not to change this yet in case there is a strong convention to favour yaw,pitch,roll that is more important, but if you agree then I'll reverse them.
  5. On the above, is there value in replacing/adding parameters to reference the angular motions as x/y/z? I'm not sure what is clearer/more sensible.

@joshnewans joshnewans changed the title Update README to reflect changes to config parameters. (#23) Update README to reflect changes to config parameters. (fixes #23) Jan 11, 2021
@clalancette
Copy link
Copy Markdown

1. I've rearranged the overall order of the parameters to be a bit more natural (axis, scale, scale_turbo; linear then angular). This reflects how it has been written in the config files too (though not in the code itself).

That works for me.

2. Rather than making the parameter list three times as long with no clarity, I have grouped the parameters which are part of a struct. This was the simplest way I could think of to do it, please let me know if you have a better idea.

Looks good to me as well.

3. In doing the above, I have introduced some ambiguity with the word `axis` (to refer to motion/coordinate axes as well as joystick axes). I think this is an inevitable consequence of a program that maps from axis to axis, and is clear enough from context.

I think it is clear enough.

4. I think it would perhaps be clearer to order the angular axes as `roll,pitch,yaw` rather than `yaw,pitch,roll`. I presume this has arisen from the original one only having `x` and `yaw`, and the other axes being added later. But now that we can map to all six axes, it seems confusing to me that the order here is different from the order that you then see in the `Twist` message. I've opted **not** to change this yet in case there is a strong convention to favour `yaw,pitch,roll` that is more important, but if you agree then I'll reverse them.

I honestly don't have a strong opinion here. ROS 2 code tends to favor RPY over YPR, but that is not universal. In this case, I'd say leave it as-is.

5. On the above, is there value in replacing/adding parameters to reference the angular motions as `x/y/z`? I'm not sure what is clearer/more sensible.

Personally, I don't think so. RPY seems pretty clear, there is a strong backwards compatibility argument, and I'd rather not add aliases unless we have a very strong reason.

Copy link
Copy Markdown

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

Thanks so much for the changes! I'm going to go ahead and merge this as it is an obvious improvement. Please feel free to keep commenting or open a new issues for further problems you find.

@clalancette clalancette merged commit 98750d8 into ros2:foxy Jan 11, 2021
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.

2 participants