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] Rename open_loop_control parameter and slightly change its scope #1525

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

Thieso
Copy link

@Thieso Thieso commented Feb 4, 2025

Resolves #1505

@Thieso Thieso changed the title [JTC]#1505 [JTC] Rename open_loop_control parameter and slightly change its scope #1505 Feb 4, 2025
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.

Thanks for your PR. However, we must not change the default behavior of existing setups. Your change would go into the stable jazzy release as well.

  1. please keep the old open_loop parameter and add a deprecation warning if it is set to the non-default value (true)
  2. if open_loop is set to true, we still should deactivate the use_closed_loop_pid_adapter_ and override the setting of interpolate_from_desired_state
  3. Mark deprecated code clearly, so we can remove that in the release after next.
  4. Please add a migration note and release note here.

@christophfroehlich christophfroehlich changed the title [JTC] Rename open_loop_control parameter and slightly change its scope #1505 [JTC] Rename open_loop_control parameter and slightly change its scope Feb 4, 2025
Thieso and others added 3 commits February 7, 2025 23:03
…rameters.yaml

Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
@Thieso
Copy link
Author

Thieso commented Feb 7, 2025

Thanks for the comments. This should be better now.

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.

Thanks for the follow-up, but please use pre-commit the next time (fixed that for you).

Copy link

codecov bot commented Feb 11, 2025

Codecov Report

Attention: Patch coverage is 66.66667% with 3 lines in your changes missing coverage. Please review.

Project coverage is 84.25%. Comparing base (ae6efeb) to head (5ec4197).

Files with missing lines Patch % Lines
...ory_controller/src/joint_trajectory_controller.cpp 0.00% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1525      +/-   ##
==========================================
- Coverage   84.27%   84.25%   -0.03%     
==========================================
  Files         123      123              
  Lines       11359    11360       +1     
  Branches      961      963       +2     
==========================================
- Hits         9573     9571       -2     
- Misses       1470     1471       +1     
- Partials      316      318       +2     
Flag Coverage Δ
unittests 84.25% <66.66%> (-0.03%) ⬇️

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

Files with missing lines Coverage Δ
...ory_controller/test/test_trajectory_controller.cpp 99.74% <100.00%> (-0.01%) ⬇️
...ntroller/test/test_trajectory_controller_utils.hpp 83.87% <ø> (-0.06%) ⬇️
...ory_controller/src/joint_trajectory_controller.cpp 84.50% <0.00%> (-0.39%) ⬇️

... and 2 files with indirect coverage changes

@christophfroehlich
Copy link
Contributor

@Thieso as you are currently working on the open-loop mode, could you have a look on #780 please and approve it if it makes sense for you? Then, please change also all occurances of open-loop in the code comments in this PR here ;)

@Thieso
Copy link
Author

Thieso commented Feb 13, 2025

@Thieso as you are currently working on the open-loop mode, could you have a look on #780 please and approve it if it makes sense for you? Then, please change also all occurances of open-loop in the code comments in this PR here ;)

Hi, I took a look and tested it on my example and it actually fixes another bug which I was not yet able to find. So it looks good to me but what exactly do you mean by replacing open-loop occurances?

@christophfroehlich
Copy link
Contributor

can you leave a review in the other pr please?
i just mean that in the comments of some tests there is written open-loop, and after your changes this does not make sense any more

Copy link
Contributor

mergify bot commented Feb 14, 2025

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

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.

[JTC] Parameter open_loop_control should be the default
2 participants