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

Add standalone version of LPF (#164) #192

Closed
wants to merge 3 commits into from

Conversation

roncapat
Copy link
Contributor

@roncapat roncapat commented Mar 2, 2024

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).

@roncapat roncapat marked this pull request as draft March 2, 2024 20:30
@codecov-commenter
Copy link

codecov-commenter commented Mar 2, 2024

Codecov Report

Attention: Patch coverage is 64.58333% with 17 lines in your changes missing coverage. Please review.

Project coverage is 50.69%. Comparing base (e05102c) to head (7aa2f97).

Files with missing lines Patch % Lines
include/control_filters/low_pass_filter_ros.hpp 57.50% 14 Missing and 3 partials ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           ros2-master     #192      +/-   ##
===============================================
+ Coverage        50.41%   50.69%   +0.27%     
===============================================
  Files                9       10       +1     
  Lines              486      507      +21     
  Branches            62       64       +2     
===============================================
+ Hits               245      257      +12     
- Misses             215      223       +8     
- Partials            26       27       +1     
Flag Coverage Δ
unittests 50.69% <64.58%> (+0.27%) ⬆️

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

Files with missing lines Coverage Δ
include/control_filters/low_pass_filter.hpp 78.26% <100.00%> (+5.95%) ⬆️
include/control_filters/low_pass_filter_ros.hpp 57.50% <57.50%> (ø)

@roncapat roncapat marked this pull request as ready for review March 18, 2024 13:19
@christophfroehlich
Copy link
Contributor

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?

@roncapat
Copy link
Contributor Author

roncapat commented Apr 4, 2024

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
Copy link
Contributor Author

roncapat commented Apr 4, 2024

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

@christophfroehlich
Copy link
Contributor

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

@christophfroehlich
Copy link
Contributor

@roncapat do you still plan to work on this PR?

@roncapat
Copy link
Contributor Author

roncapat commented Nov 4, 2024

@christophfroehlich sorry but at least for now I'm not able to finish this one due of lack of time. Of course more than happy if someone else can pick this up.

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.

LowPassFilter accepts parameters only from node
3 participants