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

Control Allocator: Metric #24199

Merged
merged 7 commits into from
Jan 20, 2025
Merged

Control Allocator: Metric #24199

merged 7 commits into from
Jan 20, 2025

Conversation

Pedro-Roque
Copy link
Member

@Pedro-Roque Pedro-Roque commented Jan 10, 2025

Solved Problem

Adds metric control allocation (ie non-normalized) to make possible model-based control schemes.

Solution

Changelog Entry

For release notes:

Feature: Metric Control Allocation
New parameter: XYZ_Z
Documentation: Need to clarify control allocation page

Test coverage

  • Unit/integration test: available
  • Tested in SITL, will provide logs

Context

Depends on #24196

@Pedro-Roque Pedro-Roque changed the title Pr metric allocator library Control Allocator: Overhaul Jan 10, 2025
Copy link

github-actions bot commented Jan 10, 2025

🔎 FLASH Analysis

px4_fmu-v5x [Total VM Diff: 40 byte (0 %)]
    FILE SIZE        VM SIZE    
--------------  -------------- 
+0.0%     +40  +0.0%     +40    .text
  +0.4%     +32  +0.4%     +32    ../../src/modules/control_allocator/ControlAllocator.cpp
  +0.4%     +12  +0.4%     +12    ../../src/lib/control_allocation/control_allocation/ControlAllocationPseudoInverse.cpp
  -0.1%      -4  -0.1%      -4    ../../src/modules/ekf2/EKF/yaw_estimator/EKFGSF_yaw.cpp
+0.0%     +38  [ = ]       0    .debug_abbrev
  -0.8%     -18  [ = ]       0    ../../src/lib/control_allocation/control_allocation/ControlAllocationPseudoInverse.cpp
   +11%     +56  [ = ]       0    ../../src/lib/version/version.c
-0.0%      -8  [ = ]       0    .debug_aranges
  -5.0%      -8  [ = ]       0    ../../src/lib/version/version.c
+0.0%      +8  [ = ]       0    .debug_frame
+0.0%     +31  [ = ]       0    .debug_info
  +0.1%     +35  [ = ]       0    ../../src/lib/control_allocation/control_allocation/ControlAllocationPseudoInverse.cpp
  -0.2%      -4  [ = ]       0    ../../src/lib/version/version.c
-0.0%     -37  [ = ]       0    .debug_line
  -0.1%      -7  [ = ]       0    ../../src/lib/control_allocation/control_allocation/ControlAllocationPseudoInverse.cpp
  -1.3%     -25  [ = ]       0    ../../src/lib/version/version.c
  -0.4%      -5  [ = ]       0    task/task_cancelpt.c
+0.0%     +72  [ = ]       0    .debug_loc
  +0.7%     +72  [ = ]       0    ../../src/lib/control_allocation/control_allocation/ControlAllocationPseudoInverse.cpp
-0.0%      -6  [ = ]       0    .debug_ranges
  -2.6%      -8  [ = ]       0    ../../src/lib/version/version.c
  +3.1%      +2  [ = ]       0    task/task_cancelpt.c
+0.0%     +98  [ = ]       0    .debug_str
  +0.9%     +87  [ = ]       0    ../../src/lib/control_allocation/control_allocation/ControlAllocationPseudoInverse.cpp
  +0.0%     +11  [ = ]       0    ../../src/modules/control_allocator/ControlAllocator.cpp
-0.3%     -40  [ = ]       0    [Unmapped]
+0.0%    +196  +0.0%     +40    TOTAL

px4_fmu-v6x [Total VM Diff: 40 byte (0 %)]
    FILE SIZE        VM SIZE    
--------------  -------------- 
+0.0%     +40  +0.0%     +40    .text
  +0.4%     +32  +0.4%     +32    ../../src/modules/control_allocator/ControlAllocator.cpp
  +0.4%     +12  +0.4%     +12    ../../src/lib/control_allocation/control_allocation/ControlAllocationPseudoInverse.cpp
  -0.1%      -4  -0.1%      -4    ../../src/modules/ekf2/EKF/yaw_estimator/EKFGSF_yaw.cpp
+0.0%     +38  [ = ]       0    .debug_abbrev
  -0.8%     -18  [ = ]       0    ../../src/lib/control_allocation/control_allocation/ControlAllocationPseudoInverse.cpp
   +11%     +56  [ = ]       0    ../../src/lib/version/version.c
-0.0%      -8  [ = ]       0    .debug_aranges
  -5.0%      -8  [ = ]       0    ../../src/lib/version/version.c
+0.0%      +8  [ = ]       0    .debug_frame
+0.0%     +31  [ = ]       0    .debug_info
  +0.1%     +35  [ = ]       0    ../../src/lib/control_allocation/control_allocation/ControlAllocationPseudoInverse.cpp
  -0.2%      -4  [ = ]       0    ../../src/lib/version/version.c
-0.0%     -29  [ = ]       0    .debug_line
  -0.1%      -7  [ = ]       0    ../../src/lib/control_allocation/control_allocation/ControlAllocationPseudoInverse.cpp
  -1.3%     -25  [ = ]       0    ../../src/lib/version/version.c
  +0.3%      +3  [ = ]       0    task/task_cancelpt.c
+0.0%     +72  [ = ]       0    .debug_loc
  +0.7%     +72  [ = ]       0    ../../src/lib/control_allocation/control_allocation/ControlAllocationPseudoInverse.cpp
-0.0%     -10  [ = ]       0    .debug_ranges
  -2.6%      -8  [ = ]       0    ../../src/lib/version/version.c
  -3.0%      -2  [ = ]       0    task/task_cancelpt.c
+0.0%     +98  [ = ]       0    .debug_str
  +0.9%     +87  [ = ]       0    ../../src/lib/control_allocation/control_allocation/ControlAllocationPseudoInverse.cpp
  +0.0%     +11  [ = ]       0    ../../src/modules/control_allocator/ControlAllocator.cpp
-0.1%     -40  [ = ]       0    [Unmapped]
+0.0%    +200  +0.0%     +40    TOTAL

Updated: 2025-01-17T20:21:13

@Jaeyoung-Lim Jaeyoung-Lim requested a review from sfuhrer January 13, 2025 17:07
@Pedro-Roque Pedro-Roque force-pushed the pr-metric-allocator-library branch from f60507b to b257fe9 Compare January 13, 2025 19:59
@Pedro-Roque Pedro-Roque changed the title Control Allocator: Overhaul Control Allocator: Metric Jan 13, 2025
@Pedro-Roque Pedro-Roque marked this pull request as ready for review January 13, 2025 20:17
bool update_normalization_scale)
{
ControlAllocation::setEffectivenessMatrix(effectiveness, actuator_trim, linearization_point, num_actuators,
update_normalization_scale);
Copy link
Member

Choose a reason for hiding this comment

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

Is update_normalization_scale ever used?

Copy link
Member Author

@Pedro-Roque Pedro-Roque Jan 14, 2025

Choose a reason for hiding this comment

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

Not by metric allocator. It is used here in ControlAllocationPseudoInverse.cpp lines 53 and 62.

Copy link
Member Author

@Pedro-Roque Pedro-Roque Jan 14, 2025

Choose a reason for hiding this comment

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

The issue is that if this is removed, than the code in

_control_allocation[i]->setEffectivenessMatrix(config.effectiveness_matrices[i], config.trim[i],
config.linearization_point[i], total_num_actuators, reason == EffectivenessUpdateReason::CONFIGURATION_UPDATE);
will break if the metric allocator is used there.

I can, however, remove it to avoid confusion and assume that if the user wishes to use it with control_allocator, there will be trouble. Perhaps that's the correct way?

@Pedro-Roque
Copy link
Member Author

@Jaeyoung-Lim I set the parameter to false to make it clear that we don't care about normalization in metric allocator.

@Pedro-Roque
Copy link
Member Author

@sfuhrer would be happy to take inputs so we can merge this soon

@sfuhrer
Copy link
Contributor

sfuhrer commented Jan 15, 2025

@Pedro-Roque yes sure, happy to have a look. Could you first rebase on latest main, as the PRs this was based on are since in main? And at the same time you could squash your commits. That makes reviewing bit easier. Thanks!

@Pedro-Roque Pedro-Roque force-pushed the pr-metric-allocator-library branch from efcca10 to 0f7f340 Compare January 15, 2025 11:39
@Pedro-Roque
Copy link
Member Author

@sfuhrer done

@DronecodeBot
Copy link

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/px4-sync-q-a-jan-15-2025/43274/1

Copy link
Contributor

@sfuhrer sfuhrer left a comment

Choose a reason for hiding this comment

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

Would be nice to untangle the matrix normalization from the actual allocation algorithm inside the ControlAllocationPseudoInverse. Then the metric allocation wouldn't even specific methods, but it would only be a flag (normalize or not) prior to passing it to the allocator.
If that cannot be done now then I'm okay to get this in, but would rename the files.

@@ -34,6 +34,8 @@
px4_add_library(ControlAllocation
ControlAllocation.cpp
ControlAllocation.hpp
ControlAllocationMetric.cpp
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you consider calling it ControlAllocationPseudoInverseMetric? That would make it more clear which parts of the existing, normalized, pipeline you're replacing.

Next step would then be to move all the normalization and scaling of the allocation matrix out of the PseudoInverse. The algorithms in the end don't care whether the allocation matrix is normalized or not, do they? Would ControlAllocationSequentialDesaturation work out of the box with a metric allocation matrix?

Copy link
Member Author

Choose a reason for hiding this comment

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

To address this I've created an internal variable to activate metric allocation. Then we keep the same structure and if non-metric is being used, the allocator behaves as previously expected.

I'd need to look in detail towards the sequential desaturation algorithm, but since I don't use it, I don't have a good answer as of now. In principle, yes, this should be possible.

@Pedro-Roque
Copy link
Member Author

Would be nice to untangle the matrix normalization from the actual allocation algorithm inside the ControlAllocationPseudoInverse. Then the metric allocation wouldn't even specific methods, but it would only be a flag (normalize or not) prior to passing it to the allocator. If that cannot be done now then I'm okay to get this in, but would rename the files.

Thanks for the input @sfuhrer ! I've done exactly that for now.

The same interface could be added to the sequential desaturation, but since I don't work with that allocator, it's hard for me to test. But the principle would be the same...

@@ -46,6 +46,7 @@
#pragma once

#include "ControlAllocation.hpp"
#include <px4_platform_common/log.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this isn't necessary, as you don't enable it due to

``` // adding #include <px4_platform_common/log.h> + PX4_WARN leads to failed linking on test

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed the requested line. Let's get it merged? 😃

@sfuhrer sfuhrer merged commit b09340c into main Jan 20, 2025
60 of 61 checks passed
@sfuhrer sfuhrer deleted the pr-metric-allocator-library branch January 20, 2025 12:30
Pedro-Roque added a commit that referenced this pull request Jan 20, 2025
…on) (#24199)

* add: metric allocation

* add: actual files

* rft: moved metric allocation to pseudo-inverse via flag with public method

* del: removed metric allocation test and added test in pseudo-inverse testing

* rft: deleted extra newline at the end of pseudo inverse test file

* feat: removed unnecessary log include
JoelJ18 pushed a commit to microstrain-robotics/PX4-Autopilot that referenced this pull request Jan 27, 2025
…on) (PX4#24199)

* add: metric allocation

* add: actual files

* rft: moved metric allocation to pseudo-inverse via flag with public method

* del: removed metric allocation test and added test in pseudo-inverse testing

* rft: deleted extra newline at the end of pseudo inverse test file

* feat: removed unnecessary log include
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.

4 participants