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.

Sign up for GitHub

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

Jump to bottom

feat: add 'parameters.md' in ros-nodes #220

Closed
wants to merge 8 commits into from
Closed

feat: add 'parameters.md' in ros-nodes #220

wants to merge 8 commits into from

Conversation

shulanbushangshu
Copy link
Contributor

@shulanbushangshu shulanbushangshu commented Oct 21, 2022 •
edited by kenji-miyake
Loading

Signed-off-by: jack.song jack.song@autocore.ai

Description

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The Reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

@takayuki5168 takayuki5168 added the type:documentation Creating or refining documentation. label Oct 21, 2022
@github-actions
Copy link

Documentation URL: https://autowarefoundation.github.io/autoware-documentation/pr-220/
Modified URLs:

@takayuki5168
Copy link
Contributor

takayuki5168 commented Oct 21, 2022 •
edited
Loading

Nice PR!

FYI: I explained how we manage parameters in Autoware in this PR. Regarding the content, it seems almost no conflict among our PRs. 👍
https://github.com/autowarefoundation/autoware-documentation/pull/171/files

kenji-miyake
kenji-miyake reviewed Oct 27, 2022
View reviewed changes
docs/contributing/coding-guidelines/ros-nodes/parameters.md Outdated
Comment on lines 5 to 8
Parameters in ROS are associated with individual nodes. Parameters are used to configure nodes at startup (and during runtime), without changing the code. The lifetime of a parameter is tied to the lifetime of the node (though the node could implement some sort of persistence to reload values after restart).<br>
Parameters are addressed by node name, node namespace, parameter name, and parameter namespace. Providing a parameter namespace is optional.<br>
Each parameter consists of a key, a value, and a descriptor. The key is a string and the value is one of the following types: bool, int64, float64, string, byte[], bool[], int64[], float64[] or string[]. By default all descriptors are empty, but can contain parameter descriptions, value ranges, type information, and additional constraints.<br>
For an hands-on tutorial with ROS parameters [see here](https://docs.ros.org/en/humble/Tutorials/Beginner-CLI-Tools/Understanding-ROS2-Parameters/Understanding-ROS2-Parameters.html)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since it's just a copy of https://docs.ros.org/en/humble/Concepts/About-ROS-2-Parameters.html#overview, we can simplify this part by linking it.

kenji-miyake
kenji-miyake reviewed Oct 27, 2022
View reviewed changes
docs/contributing/coding-guidelines/ros-nodes/parameters.md Outdated
- get &emsp;&emsp;&emsp; Get parameter
- list &emsp;&emsp;&emsp;&thinsp; Output a list of available parameters
- load &emsp;&emsp;&thinsp;&thinsp;&thinsp; Load parameter file for a node
- set &emsp;&emsp;&emsp;&thinsp; Set parameter
Copy link
Contributor

Choose a reason for hiding this comment

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

This part can also be replaced by https://docs.ros.org/en/foxy/How-To-Guides/Using-ros2-param.html.

kenji-miyake
kenji-miyake reviewed Oct 27, 2022
View reviewed changes
docs/contributing/coding-guidelines/ros-nodes/parameters.md
1000ms, std::bind(&MinimalParam::timer_callback, this));
}
}
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.
https://docs.ros.org/en/humble/Tutorials/Beginner-Client-Libraries/Using-Parameters-In-A-Class-CPP.html

@kenji-miyake
Copy link
Contributor

@shulanbushangshu Thank you for writing this.
However, I wanted to write how to design parameters, not how to use parameters.
So, please let me mark this as a draft and propose an alternative document.

@kenji-miyake kenji-miyake marked this pull request as draft October 27, 2022 08:01
@cyn-liu cyn-liu mentioned this pull request Nov 8, 2022
Merged
4 tasks
@shulanbushangshu
add-parameters
6d4caf9 
Signed-off-by: jack.song <jack.song@autocore.ai>
@shulanbushangshu
add-parameters
6b18b59 
Signed-off-by: jack.song <jack.song@autocore.ai>
@shulanbushangshu
add-parameters
6cd1545 
Signed-off-by: jack.song <jack.song@autocore.ai>
@shulanbushangshu
add-parameters
c0a1544 
Signed-off-by: jack.song <jack.song@autocore.ai>
@shulanbushangshu
add-parameters
dc24538 
Signed-off-by: jack.song <jack.song@autocore.ai>
@shulanbushangshu
add-parameters
361cefa 
Signed-off-by: jack.song <jack.song@autocore.ai>
@kenji-miyake
Copy link
Contributor

Related: #271 (comment)

@stale
Copy link

stale bot commented Feb 21, 2023

This pull request has been automatically marked as stale because it has not had recent activity.

@stale stale bot added the status:stale Inactive or outdated issues. (auto-assigned) label Feb 21, 2023
@mitsudome-r mitsudome-r self-requested a review March 28, 2023 08:00
@stale stale bot removed the status:stale Inactive or outdated issues. (auto-assigned) label Mar 28, 2023
@mitsudome-r
Copy link
Member

@shulanbushangshu What is the current status of this PR?
FYI, there was some updates to the parameter documentation in #271. Do you still need further changes to the file?

@shulanbushangshu
Copy link
Contributor Author

@mitsudome-r
I have rewritten this file. You can review this changing. If there is no need to change the current content, I will close this PR.

@mitsudome-r
Copy link
Member

@shulanbushangshu Could you update the branch with latest main branch? It makes it easier for me to see the diffs with current documentation.

Also, you might want to integrate some of the guidelines regarding parameters discussed in this thread.
Including:

Forbid arg and define all parameters in .param.yaml file. The parameters that must be written in launch files are allowed.

@shulanbushangshu
Copy link
Contributor Author

@mitsudome-r
This branch is created when there is initially no content. I have reviewed the discussion and the documentation for the 'parameters'.I don't think there's any problem. I will close the pr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kenji-miyake kenji-miyake Awaiting requested review from kenji-miyake

@mitsudome-r mitsudome-r Awaiting requested review from mitsudome-r

Assignees
No one assigned
Labels
type:documentation Creating or refining documentation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shulanbushangshu @takayuki5168 @kenji-miyake @mitsudome-r