Skip to content

Conversation

ViktorCVS
Copy link
Contributor

@ViktorCVS ViktorCVS commented Sep 15, 2025

Overview

Add a first-order derivative filter and selectable discretization methods for the I and D paths, keeping previous behavior as the default.

What was added/changed in this PR

  • Derivative filter (D path): added derivative_filter_time (seconds) in gains.__map_dof_names.
    Continuous form: C_D(s) = D * s / (1 + s * derivative_filter_time). Default 0.0 preserves the prior (unfiltered) behavior; non-negative validation.

  • Discretization methods: added per-DOF options

    • integration_method: forward_euler (default), backward_euler, trapezoidal (Tustin).
    • derivative_method: forward_euler (default), backward_euler, trapezoidal (Tustin).
      Defaults match the previous implicit scheme, so existing setups remain unchanged unless these parameters are set.

About tests

Builds cleanly and passes pre-commit and colcon test.

About change in tests

Added PidControllerTest.all_parameters_set_configure_success_full_gains, which loads test_pid_controller_full_gains, asserts successful on_configure(...), and verifies parsing/mapping of all standard fields plus the new derivative_filter_time, integration_method, and derivative_method values.

Related PR's

Final notes

I'm very open to any recommendations to improve this code.

Copy link

codecov bot commented Sep 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.26%. Comparing base (9ce90ba) to head (30b8e73).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1909   +/-   ##
=======================================
  Coverage   85.25%   85.26%           
=======================================
  Files         143      143           
  Lines       13794    13801    +7     
  Branches     1197     1198    +1     
=======================================
+ Hits        11760    11767    +7     
  Misses       1636     1636           
  Partials      398      398           
Flag Coverage Δ
unittests 85.26% <100.00%> (+<0.01%) ⬆️

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

Files with missing lines Coverage Δ
pid_controller/test/test_pid_controller.cpp 100.00% <100.00%> (ø)
pid_controller/test/test_pid_controller.hpp 86.66% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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 this feature is not yet included in the PID class, right?

@ViktorCVS
Copy link
Contributor Author

Contributor

still not, I'll add a warning in the PR description

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.

2 participants