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

[JTC] Add Parameter to Toggle State Setting on Activation #1231

Conversation

KentaKato
Copy link
Contributor

This pull request introduces a new parameter in the JointTrajectoryController that enables toggling whether the last command interface value should be set as both the current_state and last_commanded_state upon activation.

Motivation

Consider a scenario where there is a transition from a JTC to another controller and back to a JTC. Due to the implementation specifics of the intermediary controller, it is possible that the command interface values from the first JTC remain. This can lead to dangerous situations where, upon reactivation, the second JTC inadvertently uses these stale command values to command the hardware.

To mitigate this risk, I have added an option within the JTC that ensures command values are reliably reset to reflect the current state upon activation, enhancing system safety.

@KentaKato
Copy link
Contributor Author

Hi, I wanted to kindly ask if there’s a chance for this pull request to be reviewed. Any feedback would be greatly appreciated. Thank you!

Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

Thank you!

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@destogl
Copy link
Member

destogl commented Aug 29, 2024

Wait, I am not sure about this. Can you detail the scenario more. Are you using open loop?

Copy link

codecov bot commented Aug 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.36%. Comparing base (97c1e24) to head (6988e10).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1231      +/-   ##
==========================================
+ Coverage   80.34%   80.36%   +0.01%     
==========================================
  Files         105      105              
  Lines        9384     9388       +4     
  Branches      826      825       -1     
==========================================
+ Hits         7540     7545       +5     
  Misses       1570     1570              
+ Partials      274      273       -1     
Flag Coverage Δ
unittests 80.36% <100.00%> (+0.01%) ⬆️

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

Files with missing lines Coverage Δ
...ory_controller/src/joint_trajectory_controller.cpp 84.03% <100.00%> (+0.19%) ⬆️

... and 1 file with indirect coverage changes

@KentaKato
Copy link
Contributor Author

@destogl
Thank you for your review!


We are not using open-loop control. We are creating a closed-loop controller called joint_impedance_controller. This controller only has the effort command_interface and does not have the pos/vel/acc command_interfaces.

  • JTC
    • command_interface: pos/vel/acc/effort
  • joint_impedance_controller
    • command_interface: effort

Scenario:

  1. Activate JTC.
  2. Switch to joint_impedance_controller and change the arm's position.
    • At this point, the position command_interface is not updated from step 1.
  3. Switch to JTC again.
    • The position command_interface from the final position in step 1 remains. With the current implementation, this position gets set to state_current_ and last_command_state_.

…ctivation to release_notes. Updated the parameters.yaml description to match the same wording.
@KentaKato
Copy link
Contributor Author

@christophfroehlich
Thank you for your review!
I’ve added the note as you suggested. I appreciate your feedback!

@bmagyar
Copy link
Member

bmagyar commented Sep 11, 2024

@christophfroehlich , are we good?

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

LGTM

@KentaKato
Copy link
Contributor Author

@bmagyar @christophfroehlich
friendly ping!

@christophfroehlich christophfroehlich merged commit f96d2fc into ros-controls:master Oct 26, 2024
20 of 21 checks passed
@christophfroehlich
Copy link
Contributor

@Mergifyio backport iron humble

Copy link
Contributor

mergify bot commented Oct 26, 2024

backport iron humble

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Oct 26, 2024
* [JTC] Add param to setting last command interface value as state on activation

* [JTC] add a note about set_last_command_interface_value_as_state_on_activation to release_notes. Updated the parameters.yaml description to match the same wording.

---------

Co-authored-by: Bence Magyar <[email protected]>
Co-authored-by: Christoph Fröhlich <[email protected]>
(cherry picked from commit f96d2fc)
mergify bot pushed a commit that referenced this pull request Oct 26, 2024
* [JTC] Add param to setting last command interface value as state on activation

* [JTC] add a note about set_last_command_interface_value_as_state_on_activation to release_notes. Updated the parameters.yaml description to match the same wording.

---------

Co-authored-by: Bence Magyar <[email protected]>
Co-authored-by: Christoph Fröhlich <[email protected]>
(cherry picked from commit f96d2fc)

# Conflicts:
#	joint_trajectory_controller/src/joint_trajectory_controller_parameters.yaml
christophfroehlich pushed a commit that referenced this pull request Oct 26, 2024
)

* [JTC] Add param to setting last command interface value as state on activation

* [JTC] add a note about set_last_command_interface_value_as_state_on_activation to release_notes. Updated the parameters.yaml description to match the same wording.

---------

Co-authored-by: Bence Magyar <[email protected]>
Co-authored-by: Christoph Fröhlich <[email protected]>
(cherry picked from commit f96d2fc)

Co-authored-by: Kenta Kato <[email protected]>
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.

5 participants