Skip to content

Conversation

@ArthurVal
Copy link
Contributor

@ArthurVal ArthurVal commented Jan 8, 2025

It adds the unit-tests for the LinearFeedbackController class.

Tests (TestSuite = LinearFeedbackControllerTest):

  • LinearFeedbackController()
    • Ctor
  • load(const ControllerParameters&) -> bool:
    • LoadEmptyParams - Empty params
      • Unexpected exception thrown (urdf parser)
    • LoadNoURDF - No urdf
      • Unexpected exception thrown (urdf parser)
    • LoadNegativeDuration - Negative duration
      • load doesn't return false
    • Load - Valid ControllerParameters
  • set_initial_state(VectorXd const&,VectorXd const&) -> bool:
    • SetInitialStateEmpty - Empty vectors
      • set_initial_state doesn't return false
    • SetInitialStateSizeMismatch - Mismatch vector size (args)
      • Internal Eigen asserts
    • SetInitialStateSpecialDouble - NaN/inf
      • Does not return false
    • SetInitialState - Valid References
  • compute_control(const TimePoint&, const Sensor&, const Control&, bool) -> const VectorXd&:
    • ComputeControl: Valid with/without Gravity compensation
      • Eigen assert when computing with gravity compensation
  • Other ... ?

IMPORTANT: All tests that are testing against non-nominal inputs have been DISABLED

@ArthurVal
Copy link
Contributor Author

ArthurVal commented Jan 9, 2025

NOTE: Looking at the code of the LinearFeedBackController, since it just contains both PD and LF controllers and only switch between then based on the time, I'll have to wait/rebase onto the LFController unit-tests in order to use (and therefore not re-create) the tools developed here.

@ArthurVal
Copy link
Contributor Author

Also, testing this is extremely redundant with the tests of both PDController and LFController, since functions (expect compute_control) only forward arguments to the underlying controller functions.
This may be an indication that it is not needed ?

@ArthurVal ArthurVal added the enhancement New feature or request label Jan 9, 2025
@ArthurVal
Copy link
Contributor Author

NOTE: Looking at the code of the LinearFeedBackController, since it just contains both PD and LF controllers and only switch between then based on the time, I'll have to wait/rebase onto the LFController unit-tests in order to use (and therefore not re-create) the tools developed here.

Rebase done into #46 (commit e0ffc68)

@ArthurVal
Copy link
Contributor Author

Rebase done into #46 (commit 9837315)

@ArthurVal
Copy link
Contributor Author

Rebased into humble-devel (includes #62)

Copy link
Collaborator

@MaximilienNaveau MaximilienNaveau left a comment

Choose a reason for hiding this comment

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

Unit-tests passing I approve!

<< (gravity_compensation ? "TRUE"sv : "FALSE"sv));

// First call always calls PDController
EXPECT_EQ(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way to force formatter to do something better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you use a loop inside a test, SCOPED_TRACE() is one way to provide a trace for user to what was the looped variable value when a test failed (see gtest).

I can also forward the string at the end each EXPECT_* call (EXPECT_EQ(...) << gravity_compensation;), but it would be cumbersome and very repetitive.

At the beginning, I used EXPECT_PRED*() in order to let GTest format the inputs parameters automatically when an expectation failed (the same one that I used to check for double vectors equality with rounding errors), but I chosed the revert to the simpler and more explicit way to ease the review process (with the direct .compute_control() method calls).

In fact, it could have looked something like this:

EXPECT_PRED4(
  ComputeControlOn(
    ctrl
    EqualsNear(5e-6, ((1.0 - ratio) * expected.pd) + (ratio * expected.lf)))
   ),
   when, sensor, control, gravity_compensation
 );

And it would have output something along the lines of:

ComputeControlOn(
  ctrl
  EqualsNear(5e-6, ((1.0 - ratio) * expected.pd) + (ratio * expected.lf)))
)(when, sensor, control, gravity_compensation) is false, where
when is ...
sensor is ...
control is ...
gravity_compensation is ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you misunderstood me. I meant to force clang format to format the text in a more readable way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, for this I don't know unfortunately, especially since we don't have any control over the format config file.

The only way I can think of is by encapsulating the code between clang-format off/on and do the formatting ourselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or just abbreviate the variables more in order to have a single line expression.

@ArthurVal ArthurVal requested a review from Kotochleb February 6, 2025 10:03
@MaximilienNaveau MaximilienNaveau merged commit 05d4121 into loco-3d:humble-devel Feb 6, 2025
2 checks passed
@ArthurVal ArthurVal deleted the add/linear_feedback_controller_unittests branch February 6, 2025 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants