Skip to content

make _on_parameter_event return result correctly #817

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
Merged
merged 3 commits into from
Jul 18, 2022
Merged

make _on_parameter_event return result correctly #817

merged 3 commits into from
Jul 18, 2022

Conversation

@squizz617
Copy link
Contributor

@squizz617 squizz617 commented Aug 20, 2021

Previously, _on_parameter_event always returned successful=True to the
caller (e.g., ros2param set) regardless of whether setting use_sim_time
parameter actually succeeded or not.

  • PoC:
# terminal 1
$ ros2 run examples_rclpy_minimal_publisher publisher_member_function

[INFO] [1629490410.452032755] [minimal_publisher]: Publishing: "Hello World: 0"
[INFO] [1629490410.918999697] [minimal_publisher]: Publishing: "Hello World: 1"
[INFO] [1629490411.419087028] [minimal_publisher]: Publishing: "Hello World: 2"
[INFO] [1629490411.919343319] [minimal_publisher]: Publishing: "Hello World: 3"
[INFO] [1629490412.419345165] [minimal_publisher]: Publishing: "Hello World: 4"
[INFO] [1629490412.919260702] [minimal_publisher]: Publishing: "Hello World: 5"
[ERROR] [1629490413.030775970] [minimal_publisher]: use_sim_time parameter set to something besides a bool
[INFO] [1629490413.419389164] [minimal_publisher]: Publishing: "Hello World: 6"
[INFO] [1629490413.919106545] [minimal_publisher]: Publishing: "Hello World: 7"
# terminal 2
$ ros2 param set /minimal_publisher use_sim_time Trueeeee

Set parameter successful

As demonstrated above, when trying to set use_sim_time param to an invalid
type, the minimal_publisher node complains it cannot. However, ros2 param
prints "Set parameter successful". This commit fixes this issue.

@fujitatomoya
Copy link
Collaborator

fujitatomoya commented Aug 23, 2021

Just FYI, the following is the current behavior.

root@12956542b338:~/ros2_ws/colcon_ws# ros2 run examples_rclpy_minimal_publisher publisher_member_function
[INFO] [1629693600.867826272] [minimal_publisher]: Publishing: "Hello World: 0"
[INFO] [1629693601.359118866] [minimal_publisher]: Publishing: "Hello World: 1"
[INFO] [1629693601.859174180] [minimal_publisher]: Publishing: "Hello World: 2"
[INFO] [1629693602.359177531] [minimal_publisher]: Publishing: "Hello World: 3"
...<snip>

root@12956542b338:~/ros2_ws/colcon_ws# ros2 param set /minimal_publisher use_sim_time Trueeeee
Setting parameter failed: Wrong parameter type, parameter {use_sim_time} is of type {bool}, setting it to {string} is not allowed.

type mismatch can be detected before _parameters_callbacks is called.

fujitatomoya
fujitatomoya approved these changes Aug 23, 2021
View reviewed changes
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm

@fujitatomoya
Copy link
Collaborator

fujitatomoya commented Aug 23, 2021

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@fujitatomoya
Copy link
Collaborator

fujitatomoya commented Feb 3, 2022

as mentioned in #817 (comment), currently with master, it will check the parameter type via _apply_descriptor before calling on_set_parameters_callback. So it actually cannot reach the code which this PR tries to fix.

But i think this PR makes sense.

@fujitatomoya
Copy link
Collaborator

fujitatomoya commented Feb 3, 2022

@squizz617 could you address flake8 errors?

@squizz617
make _on_parameter_event return result correctly
cc11b45 
Previously, `_on_parameter_event` always returned `successful=True` to the
caller (e.g., ros2param set) regardless of whether setting `use_sim_time`
parameter actually succeeded or not.

* PoC:
```sh
# terminal 1
$ ros2 run examples_rclpy_minimal_publisher publisher_member_function

[INFO] [1629490410.452032755] [minimal_publisher]: Publishing: "Hello World: 0"
[INFO] [1629490410.918999697] [minimal_publisher]: Publishing: "Hello World: 1"
[INFO] [1629490411.419087028] [minimal_publisher]: Publishing: "Hello World: 2"
[INFO] [1629490411.919343319] [minimal_publisher]: Publishing: "Hello World: 3"
[INFO] [1629490412.419345165] [minimal_publisher]: Publishing: "Hello World: 4"
[INFO] [1629490412.919260702] [minimal_publisher]: Publishing: "Hello World: 5"
[ERROR] [1629490413.030775970] [minimal_publisher]: use_sim_time parameter set to something besides a bool
[INFO] [1629490413.419389164] [minimal_publisher]: Publishing: "Hello World: 6"
[INFO] [1629490413.919106545] [minimal_publisher]: Publishing: "Hello World: 7"
```

```sh
# terminal 2
$ ros2 param set /minimal_publisher use_sim_time Trueeeee

Set parameter successful
```

As demonstrated above, when trying to set `use_sim_time` param to an invalid
type, the minimal_publisher node complains it cannot. However, ros2 param
prints "Set parameter successful". This commit fixes this issue.

Signed-off-by: Seulbae Kim <squizz617@gmail.com>
@squizz617
use single quote
8ac897f 
Signed-off-by: Seulbae Kim <squizz617@gmail.com>
@squizz617
Copy link
Contributor Author

squizz617 commented Feb 4, 2022

Fixed the flake8 error. Seems ready to be merged.

And the param type check in the master branch does make sense. Thanks!

@fujitatomoya
Copy link
Collaborator

fujitatomoya commented Feb 4, 2022

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@fujitatomoya
Copy link
Collaborator

fujitatomoya commented Feb 4, 2022

@ivanpauno could you review on this just in case before merge.

ivanpauno
ivanpauno approved these changes Feb 9, 2022
View reviewed changes
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM, it would be nice if tests are added before this gets merged

@fujitatomoya
Copy link
Collaborator

fujitatomoya commented Feb 10, 2022

CI is failing because of https://ci.ros2.org/job/ci_linux/16070/console, we need to use https://github.com/ros2/ros2/blob/4a36f319afcc208deae3c373493ef6d9feda1324/ros2.repos#L45.

@fujitatomoya
Copy link
Collaborator

fujitatomoya commented Feb 10, 2022

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member

ivanpauno commented Feb 28, 2022

@fujitatomoya friendly ping, what's the state of this?

@audrow audrow changed the base branch from master to rolling June 28, 2022 14:21
@sloretz
Copy link
Contributor

sloretz commented Jul 18, 2022

CI (repos file build: --packages-up-to rclpy test: --packages-select rclpy)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@sloretz
Copy link
Contributor

sloretz commented Jul 18, 2022

PR job is test_timer, which appears to be flaky and unrelated to this PR. CI LGTM

@sloretz
Copy link
Contributor

sloretz commented Jul 18, 2022

Thank you for the PR!

@sloretz sloretz merged commit c4cf7a0 into ros2:rolling Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ivanpauno ivanpauno ivanpauno approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

Assignees

@fujitatomoya fujitatomoya

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@squizz617 @fujitatomoya @ivanpauno @sloretz