search

ros-controls / control_toolbox

This package contains several C++ classes useful in writing controllers.
https://control.ros.org
BSD 3-Clause "New" or "Revised" License
120 stars 97 forks source link

Add standalone version of LPF (#164) #192

Open roncapat opened 6 months ago

roncapat commented 6 months ago

Closes #164. Test passes locally.

The old LowPassFilter class is now LowPassFilterRos. API-breaking since with this patch LowPassFilter is a parent class without ROS parameters.

@christophfroehlich at least for now I wanted to reflect the same difference of Pid and PidROS. But we need to devise a different naming strategy, or this patch will break a lot of perfectly fine code (will require people to swap LowPassFilter with LowPassFilterRos in their projects).

codecov-commenter commented 6 months ago

Codecov Report

Attention: Patch coverage is 67.34694% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 50.38%. Comparing base (759d954) to head (e3fde82).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## ros2-master #192 +/- ## =============================================== + Coverage 50.10% 50.38% +0.28% =============================================== Files 10 11 +1 Lines 497 518 +21 Branches 166 170 +4 =============================================== + Hits 249 261 +12 - Misses 217 225 +8 - Partials 31 32 +1 ``` | [Flag](https://app.codecov.io/gh/ros-controls/control_toolbox/pull/192/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/ros-controls/control_toolbox/pull/192/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls) | `50.38% <67.34%> (+0.28%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/ros-controls/control_toolbox/pull/192?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls) | Coverage Δ | | |---|---|---| | [include/control\_filters/low\_pass\_filter.hpp](https://app.codecov.io/gh/ros-controls/control_toolbox/pull/192?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls#diff-aW5jbHVkZS9jb250cm9sX2ZpbHRlcnMvbG93X3Bhc3NfZmlsdGVyLmhwcA==) | `80.43% <100.00%> (+5.05%)` | :arrow_up: | | [src/control\_filters/low\_pass\_filter.cpp](https://app.codecov.io/gh/ros-controls/control_toolbox/pull/192?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls#diff-c3JjL2NvbnRyb2xfZmlsdGVycy9sb3dfcGFzc19maWx0ZXIuY3Bw) | `100.00% <100.00%> (ø)` | | | [include/control\_filters/low\_pass\_filter\_ros.hpp](https://app.codecov.io/gh/ros-controls/control_toolbox/pull/192?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls#diff-aW5jbHVkZS9jb250cm9sX2ZpbHRlcnMvbG93X3Bhc3NfZmlsdGVyX3Jvcy5ocHA=) | `60.00% <60.00%> (ø)` | |
christophfroehlich commented 5 months ago

Sorry for the late reply. I see two options:

What do you think?

roncapat commented 5 months ago

Sorry for the late reply. I see two options:

  • Use a consistent nomenclature and break API with Jazzy (we would have to branch this repo then)
  • Deprecate the old LowPassFilter with the hint to replace it with LowPassFilterRos and call the base filter LowPassFilterBase or similar. This could be then rewritten in a future release to converge back to the consistent nomenclature.

What do you think?

Yeah I believe we should slowly make nomenclature consistent, I like better solution 2.

BTW, why are there no 'humble', 'iron', 'rolling' branches and so on in this repo?

roncapat commented 5 months ago

I will update the PR as soon as possible implementing what you proposed as option 2.

christophfroehlich commented 5 months ago

BTW, why are there no 'humble', 'iron', 'rolling' branches and so on in this repo?

because the ros2-master branch is released to all distros, there was no need for branching yet