[JTC] Fix state offset tests by christophfroehlich · Pull Request #797 · ros-controls/ros2_controllers

christophfroehlich · 2023-10-10T21:11:20Z

I tried to write tests for #794 and copied the initialization of joint_pos_ from test_hw_states_has_offset_first_controller_start/test_hw_states_has_offset_later_controller_start: This did not work because ActivateTrajectoryController always has set it to the constant values from the utils-header -> I fixed that.

Tests failed now, and thinking about the test criteria itself, considering #189, I rewrote the tests -> succeed now.

add approach to change controller without "jumps" from the command to state values:

when started HW interface has to set command_interfaces to N aN

controller checks if command interfaces are NaN --> then is a controller started for the first time, so read states

if command interface have a value --> so a controller was running already (supports switching between controllers)

@destogl You have added this part, before it got changed a bit by #340 by @AndyZe and @mechwiz

Why should we set state_desired_ to the value of read_state_from_command_interfaces? If there is no active trajectory (start_with_holding is false), this value will only be published on the state-topic. If there is a new trajectory, it will be overwritten by the sampled trajectory.
Setting state_current to the command read from the hardware interface is fine: It is only used for setting the set_hold_position if start_with_holding is set. It will be overwritten by the first call of update.

codecov · 2023-10-10T21:19:11Z

Codecov Report

Merging #797 (387afc7) into master (232154d) will decrease coverage by 0.49%.
Report is 1 commits behind head on master.
The diff coverage is 28.57%.

❗ Current head 387afc7 differs from pull request most recent head efcf0d1. Consider uploading reports for the commit efcf0d1 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #797      +/-   ##
==========================================
- Coverage   45.40%   44.92%   -0.49%     
==========================================
  Files          40       40              
  Lines        3649     3642       -7     
  Branches     1723     1716       -7     
==========================================
- Hits         1657     1636      -21     
- Misses        810      834      +24     
+ Partials     1182     1172      -10

Flag	Coverage Δ
unittests	`44.92% <28.57%> (-0.49%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...jectory_controller/joint_trajectory_controller.hpp	`0.00% <ø> (ø)`
...ory_controller/src/joint_trajectory_controller.cpp	`47.72% <28.57%> (+0.45%)`	⬆️

... and 3 files with indirect coverage changes

bmagyar

let's wait for @destogl to reply too

destogl

Just a small proposal for optimization since we are discussing those functions.

Answers to comments

Why should we set state_desired_ to the value of read_state_from_command_interfaces? If there is no active trajectory (start_with_holding is false), this value will only be published on the state-topic. If there is a new trajectory, it will be overwritten by the sampled trajectory.

Can it be that we change this when fixing the “hold on start” feature? I think the reason was that we have immediate output to the command interface even if there is no reference. If we can achieve that by using start_with_holding this is also fine with me. Nevertheless, I would not add additional if, even if we set the value “unnecessary”. Or do you think that this could be confusing?

Setting state_current to the command read from the hardware interface is fine: It is only used for setting the set_hold_position if start_with_holding is set. It will be overwritten by the first call of update.

OK, you are right. Then we probably should not set state_desired in that case.

joint_trajectory_controller/src/joint_trajectory_controller.cpp

joint_trajectory_controller/include/joint_trajectory_controller/joint_trajectory_controller.hpp

christophfroehlich · 2023-11-01T21:06:54Z

Why should we set state_desired_ to the value of read_state_from_command_interfaces? If there is no active trajectory (start_with_holding is false), this value will only be published on the state-topic. If there is a new trajectory, it will be overwritten by the sampled trajectory.

Can it be that we change this when fixing the “hold on start” feature? I think the reason was that we have immediate output to the command interface even if there is no reference. If we can achieve that by using start_with_holding this is also fine with me. Nevertheless, I would not add additional if, even if we set the value “unnecessary”. Or do you think that this could be confusing?

I'll think that trough again if this is achieved with the hold-on-start in a fresh moment.

Optional override is already inside an if, for me it is clearer to have it in the else branch instead (but that's only my personal preference).

christophfroehlich · 2023-11-11T10:14:36Z

Why should we set state_desired_ to the value of read_state_from_command_interfaces? If there is no active trajectory (start_with_holding is false), this value will only be published on the state-topic. If there is a new trajectory, it will be overwritten by the sampled trajectory.

Can it be that we change this when fixing the “hold on start” feature? I think the reason was that we have immediate output to the command interface even if there is no reference. If we can achieve that by using start_with_holding this is also fine with me. Nevertheless, I would not add additional if, even if we set the value “unnecessary”. Or do you think that this could be confusing?

The current way with this PR is:

If we have start_with_holding=false, no update of the command interface until the first trajectory is received. See has_active_trajectory() in update method.
start_with_holding=true: There will be an immediate update of the command interfaces, with position set from state_current_, which is
- set to the values of the command interfaces, if they are not NaN or
- set to the values read from the state interfaces otherwise.

Notes:

I asked in [JTC] Need for start_with_holding option? #816 if we need start_with_holding parameter at all.
with [JTC] Add option to hold position on last command #794 there would be no change, because last_commanded_state_ is initialized identically.

@destogl What do you think?

mergify · 2023-11-15T11:21:26Z

This pull request is in conflict. Could you fix it @christophfroehlich?

Co-authored-by: Dr. Denis <denis@stoglrobotics.de>

destogl

This looks great! Thanks!

joint_trajectory_controller/src/joint_trajectory_controller.cpp

* Simplify initialization of states * Rename read_state_from_hardware method * Don't set state_desired in on_activate --------- Co-authored-by: Dr. Denis <denis@stoglrobotics.de> (cherry picked from commit b13ae5b) # Conflicts: # joint_trajectory_controller/test/test_trajectory_controller_utils.hpp

* Simplify initialization of states * Rename read_state_from_hardware method * Don't set state_desired in on_activate --------- Co-authored-by: Dr. Denis <denis@stoglrobotics.de>

Squashed commit of the following: commit 0a9f4b0 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu Nov 16 23:17:20 2023 +0000 Bump ros-tooling/action-ros-ci from 0.3.4 to 0.3.5 (ros-controls#833) commit 3fc0f50 Author: Christoph Fröhlich <christophfroehlich@users.noreply.github.com> Date: Wed Nov 15 20:03:40 2023 +0100 [JTC] Activate update of dynamic parameters (ros-controls#761) commit b13ae5b Author: Christoph Fröhlich <christophfroehlich@users.noreply.github.com> Date: Wed Nov 15 19:14:09 2023 +0100 [JTC] Fix tests when state offset is used (ros-controls#797) * Simplify initialization of states * Rename read_state_from_hardware method * Don't set state_desired in on_activate --------- Co-authored-by: Dr. Denis <denis@stoglrobotics.de> commit f1a5397 Author: Christoph Fröhlich <christophfroehlich@users.noreply.github.com> Date: Wed Nov 15 12:18:38 2023 +0100 [JTC] Remove deprecation warnings, set `allow_nonzero_velocity_at_trajectory_end` default false (ros-controls#834) commit 232154d Author: Christoph Fröhlich <christophfroehlich@users.noreply.github.com> Date: Sat Nov 11 15:30:19 2023 +0100 [CI] Update coverage workflows and some cleanup (ros-controls#819) * Add CI coverage jobs for humble and iron * Add all packages to source build * Update repos files * Build ros-control packages from source * use humble job names Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com> --------- Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com> commit 0196eeb Author: Tony Baltovski <tony.baltovski@gmail.com> Date: Thu Nov 9 17:15:45 2023 -0500 [diff_drive_controller] Fixed typos in diff_drive_controller_parameter.yaml. (ros-controls#822) commit 71ec842 Author: Christoph Fröhlich <christophfroehlich@users.noreply.github.com> Date: Tue Nov 7 13:47:23 2023 +0100 [CI] Re-add humble workflow files and add iron to readme (ros-controls#818) commit 9d4ea6b Author: Bence Magyar <bence.magyar.robotics@gmail.com> Date: Tue Nov 7 12:01:23 2023 +0000 [diff_drive_controller] Remove non-stamped Twist option (ros-controls#812) commit 5f78fe4 Author: Bence Magyar <bence.magyar.robotics@gmail.com> Date: Tue Nov 7 12:01:02 2023 +0000 Adjust tests after passing URDF to controllers (ros-controls#817) commit 2c6d7a6 Author: Bence Magyar <bence.magyar.robotics@gmail.com> Date: Mon Nov 6 18:29:10 2023 +0000 Add iron branch (ros-controls#814) commit f81a82c Author: Christoph Fröhlich <christophfroehlich@users.noreply.github.com> Date: Fri Nov 3 21:18:31 2023 +0100 [CI] Codecov (ros-controls#807) commit c6ecab7 Author: Bence Magyar <bence.magyar.robotics@gmail.com> Date: Wed Nov 1 21:38:16 2023 +0000 Use pre-commit fork of clang-format instead of local (ros-controls#813) commit 6e46053 Author: Bence Magyar <bence.magyar.robotics@gmail.com> Date: Tue Oct 31 21:34:10 2023 +0000 3.17.0 commit 822552f Author: Bence Magyar <bence.magyar.robotics@gmail.com> Date: Tue Oct 31 21:33:40 2023 +0000 Update changelogs commit 8a90b51 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue Oct 31 20:27:04 2023 +0000 Bump ros-tooling/action-ros-ci from 0.3.3 to 0.3.4 (ros-controls#810) commit b65314d Author: Christoph Fröhlich <christophfroehlich@users.noreply.github.com> Date: Thu Oct 26 12:09:27 2023 +0200 Cleanup comments and unnecessary checks (ros-controls#803) commit b36b9d2 Author: Christoph Fröhlich <christophfroehlich@users.noreply.github.com> Date: Tue Oct 24 20:17:28 2023 +0200 [CI] Fix coverage build and codecov stats (ros-controls#804) commit 22356e0 Author: Dr. Denis <denis@stoglrobotics.de> Date: Mon Oct 16 18:49:47 2023 +0200 [TestNodes] Optimize output about setup of JTC publisher (ros-controls#792) commit ac291ab Author: Christoph Fröhlich <christophfroehlich@users.noreply.github.com> Date: Mon Oct 16 13:00:53 2023 +0200 Steering controllers library: fix open loop mode (ros-controls#793) * set last*velocity variables for open loop odometry * Make function arguments const * Update function in header file too commit c831b6c Author: Christoph Fröhlich <christophfroehlich@users.noreply.github.com> Date: Mon Oct 16 12:58:44 2023 +0200 Update requirements of state interfaces (ros-controls#798)

* Simplify initialization of states * Rename read_state_from_hardware method * Don't set state_desired in on_activate --------- Co-authored-by: Dr. Denis <denis@stoglrobotics.de>

christophfroehlich requested review from bmagyar and destogl October 11, 2023 07:20

christophfroehlich marked this pull request as ready for review October 15, 2023 22:42

github-actions bot requested review from duringhof, harderthan, malapatiravi, progtologist and rosterloh October 15, 2023 22:43

bmagyar approved these changes Oct 31, 2023

View reviewed changes

destogl reviewed Nov 1, 2023

View reviewed changes

joint_trajectory_controller/src/joint_trajectory_controller.cpp Outdated Show resolved Hide resolved

joint_trajectory_controller/include/joint_trajectory_controller/joint_trajectory_controller.hpp Outdated Show resolved Hide resolved

christophfroehlich added the backport-iron label Nov 11, 2023

christophfroehlich and others added 9 commits November 15, 2023 17:12

Simplify initialization of states

38d2798

Update *states_has_offset* tests' criteria

751237d

Fix comment

c73a7af

Update comments and debug output

ef6deca

Fix test criteria and initialization of state + command interfaces

eb34ead

Cleanup debug code

55a2844

Cleanup one test

f1eaa49

Rename read_state_from_hardware method

34ec02e

Co-authored-by: Dr. Denis <denis@stoglrobotics.de>

Don't set state_desired in on_activate

5a111e3

christophfroehlich force-pushed the jtc/fix_state_offset_tests branch from 8937354 to 5a111e3 Compare November 15, 2023 17:13

Fix whitespace

387afc7

destogl approved these changes Nov 15, 2023

View reviewed changes

joint_trajectory_controller/src/joint_trajectory_controller.cpp Outdated Show resolved Hide resolved

Remove added empty line.

efcf0d1

destogl merged commit b13ae5b into ros-controls:master Nov 15, 2023

mergify bot mentioned this pull request Nov 15, 2023

[JTC] Fix state offset tests (backport #797) #837

Closed

christophfroehlich deleted the jtc/fix_state_offset_tests branch November 15, 2023 18:40

christophfroehlich mentioned this pull request Nov 15, 2023

[JTC] Backport recent changes #801

Merged

RobertWilbrandt mentioned this pull request Nov 21, 2023

Renamed normalize_joint_error_ to joints_angle_wraparound_ UniversalRobots/Universal_Robots_ROS2_Driver#876

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JTC] Fix state offset tests#797

[JTC] Fix state offset tests#797
destogl merged 11 commits intoros-controls:masterfrom
christophfroehlich:jtc/fix_state_offset_tests

christophfroehlich commented Oct 10, 2023 •

edited

Loading

Uh oh!

codecov bot commented Oct 10, 2023 •

edited

Loading

Uh oh!

bmagyar left a comment

Uh oh!

destogl left a comment

Uh oh!

Uh oh!

Uh oh!

christophfroehlich commented Nov 1, 2023

Uh oh!

christophfroehlich commented Nov 11, 2023

Uh oh!

mergify bot commented Nov 15, 2023

Uh oh!

destogl left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

christophfroehlich commented Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bmagyar left a comment

Choose a reason for hiding this comment

Uh oh!

destogl left a comment

Choose a reason for hiding this comment

Answers to comments

Uh oh!

Uh oh!

Uh oh!

christophfroehlich commented Nov 1, 2023

Uh oh!

christophfroehlich commented Nov 11, 2023

Uh oh!

mergify bot commented Nov 15, 2023

Uh oh!

destogl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

christophfroehlich commented Oct 10, 2023 •

edited

Loading

codecov bot commented Oct 10, 2023 •

edited

Loading