Control Allocation Sequential Desaturation unit tests #22612

somebody-once-told-me · 2024-01-07T03:12:29Z

Solved Problem

Unit tests for ControlAllocationSequentialDesaturation. No code changes are made to any existing library, this only adds unit tests. The unit tests serve 2 purposes: 1) help ensure that expected functionality remains the same and 2) documents and demonstrates (through examples) the desaturation algorithm.

somebody-once-told-me · 2024-01-08T23:44:16Z

hmm, looks like the tailsitter SITL test is failing? curious as this only adds a unit test.

somebody-once-told-me · 2024-01-13T17:29:48Z

merged; now all SITL tests are passing.

sfuhrer · 2024-01-15T11:19:17Z

Thanks for your contribution, I'm going through it later.

hmm, looks like the tailsitter SITL test is failing? curious as this only adds a unit test.

It's now passing. These SITL tests are a bit flaky.

Can you rebase your branch on latest main? Something seems to have been messed up with your history, and this PR shows up many commits that don't come from you.

somebody-once-told-me · 2024-01-15T13:14:06Z

@sfuhrer I can try, but I am not sure why those other commits are showing up. Although, the changes are accurately captured, as can be seen in the Files changed tab. If this PR is merged as-is, won't it all be squashed in a single commit anyway (and the weird commit history shown above won't matter)?

somebody-once-told-me · 2024-01-15T13:36:32Z

@sfuhrer the commit history has been cleaned up.

sfuhrer

Thanks for kicking this off, that's very valuable.
I only have some minor things. I leave it up to you to also add some test cases for the "normalized" allocation - could though also be a follow up PR.

src/modules/control_allocator/ControlAllocation/ControlAllocationSequentialDesaturationTest.cpp

sfuhrer · 2024-01-17T09:32:06Z

src/modules/control_allocator/ControlAllocation/ControlAllocationSequentialDesaturationTest.cpp

+
+	const auto &actuator_sp = allocator.getActuatorSetpoint();
+	// In the case of yaw saturation, thrust per motor will be reduced by the hard-coded
+	// magic-number yaw margin of 0.15f.


@MaEtUgR maybe it's a good time to document this magic number in the algorithm as well!

I can only imagine why this is useful. I think @bresch had the full context here.

…saturation

sfuhrer

Just some mini comments, otherwise good to go.

PS: please do not force push a branch that you opened already a pull request for, as otherwise it's not possible for the reviewer to see what has changed since the last review. Just add the review comments as new commits on top, and push the branch like that. Once the PR is approved you can clean up the commits or we just squash-merge to merge it all into one commit.

src/modules/control_allocator/ControlAllocation/ControlAllocationSequentialDesaturationTest.cpp

…ionSequentialDesaturationTest.cpp Co-authored-by: Silvan Fuhrer <silvan@auterion.com>

sfuhrer

You forgot a couple of MOTOR_COUNTS. Lets do them all in ints and then we can get this in!

src/modules/control_allocator/ControlAllocation/ControlAllocationSequentialDesaturationTest.cpp

…for logical float zeros, and make YAW_MOTORS an int

somebody-once-told-me · 2024-01-24T14:06:18Z

@sfuhrer thanks, I fixed those and also added the float specifier suffix to all float zeros, replaced magic numbers with MOTOR_COUNT where applicable, and changed YAW_MOTORS to an int.

sfuhrer

Thanks for the last clean up, let's get it in!

* [control_allocation] add unit tests for ControlAllocationSequentialDesaturation * complete first 2 unit tests * add yaw test * add more unit tests * improve comments * format * address review comments * submodule update * Update src/modules/control_allocator/ControlAllocation/ControlAllocationSequentialDesaturationTest.cpp Co-authored-by: Silvan Fuhrer <silvan@auterion.com> * Update src/modules/control_allocator/ControlAllocation/ControlAllocationSequentialDesaturationTest.cpp Co-authored-by: Silvan Fuhrer <silvan@auterion.com> * Update src/modules/control_allocator/ControlAllocation/ControlAllocationSequentialDesaturationTest.cpp Co-authored-by: Silvan Fuhrer <silvan@auterion.com> * remove float suffix for logical integers, add missing float suffixes for logical float zeros, and make YAW_MOTORS an int --------- Co-authored-by: Master Chief <master-chief@the-void.com> Co-authored-by: Silvan Fuhrer <silvan@auterion.com>

somebody-once-told-me force-pushed the control-allocation-sequential-desaturation-unit-test branch 2 times, most recently from c4c1d88 to 3c5d8ae Compare January 15, 2024 13:35

somebody-once-told-me force-pushed the control-allocation-sequential-desaturation-unit-test branch 4 times, most recently from 670e226 to 025881f Compare January 17, 2024 08:53

sfuhrer reviewed Jan 17, 2024

View reviewed changes

somebody-once-told-me force-pushed the control-allocation-sequential-desaturation-unit-test branch 4 times, most recently from 5f997b4 to 060d4c2 Compare January 19, 2024 02:20

Master Chief added 8 commits January 19, 2024 20:49

[control_allocation] add unit tests for ControlAllocationSequentialDe…

4724ae9

…saturation

complete first 2 unit tests

4cc36dc

add yaw test

adf1503

add more unit tests

efc3389

improve comments

b658194

format

fe75697

address review comments

72de4ef

submodule update

4bce6d9

somebody-once-told-me force-pushed the control-allocation-sequential-desaturation-unit-test branch from 060d4c2 to 4bce6d9 Compare January 20, 2024 01:50

sfuhrer reviewed Jan 23, 2024

View reviewed changes

somebody-once-told-me and others added 3 commits January 23, 2024 21:36

Update src/modules/control_allocator/ControlAllocation/ControlAllocat…

f573311

…ionSequentialDesaturationTest.cpp Co-authored-by: Silvan Fuhrer <silvan@auterion.com>

Update src/modules/control_allocator/ControlAllocation/ControlAllocat…

5cebe94

…ionSequentialDesaturationTest.cpp Co-authored-by: Silvan Fuhrer <silvan@auterion.com>

Update src/modules/control_allocator/ControlAllocation/ControlAllocat…

597e6fa

…ionSequentialDesaturationTest.cpp Co-authored-by: Silvan Fuhrer <silvan@auterion.com>

sfuhrer reviewed Jan 24, 2024

View reviewed changes

remove float suffix for logical integers, add missing float suffixes …

80240c1

…for logical float zeros, and make YAW_MOTORS an int

sfuhrer approved these changes Jan 26, 2024

View reviewed changes

sfuhrer merged commit f40ede6 into PX4:main Jan 26, 2024
88 checks passed

somebody-once-told-me deleted the control-allocation-sequential-desaturation-unit-test branch January 27, 2024 01:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Control Allocation Sequential Desaturation unit tests #22612

Control Allocation Sequential Desaturation unit tests #22612

somebody-once-told-me commented Jan 7, 2024

somebody-once-told-me commented Jan 8, 2024

somebody-once-told-me commented Jan 13, 2024

sfuhrer commented Jan 15, 2024

somebody-once-told-me commented Jan 15, 2024

somebody-once-told-me commented Jan 15, 2024

sfuhrer left a comment

sfuhrer Jan 17, 2024

MaEtUgR Jan 26, 2024

sfuhrer left a comment

sfuhrer left a comment

somebody-once-told-me commented Jan 24, 2024

sfuhrer left a comment

Control Allocation Sequential Desaturation unit tests #22612

Control Allocation Sequential Desaturation unit tests #22612

Conversation

somebody-once-told-me commented Jan 7, 2024

Solved Problem

somebody-once-told-me commented Jan 8, 2024

somebody-once-told-me commented Jan 13, 2024

sfuhrer commented Jan 15, 2024

somebody-once-told-me commented Jan 15, 2024

somebody-once-told-me commented Jan 15, 2024

sfuhrer left a comment

Choose a reason for hiding this comment

sfuhrer Jan 17, 2024

Choose a reason for hiding this comment

MaEtUgR Jan 26, 2024

Choose a reason for hiding this comment

sfuhrer left a comment

Choose a reason for hiding this comment

sfuhrer left a comment

Choose a reason for hiding this comment

somebody-once-told-me commented Jan 24, 2024

sfuhrer left a comment

Choose a reason for hiding this comment