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
Performance improvement to least-squares based contrained control allocation #3036
base: master
Are you sure you want to change the base?
Conversation
0025f3e
to
1e6bacf
Compare
test flight successful. All algorithms and settings work as expected. I made QR the default which is not as fast as CHOL, does not suffer from numerical issues. CHOL also did not crash my platform, but is known to provide less accurate solutions when |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you trying to improve this solver. I have however several remarks to make this code easier to read and to maintain. It would also be good to avoid duplication of code from the math lib, or even integrate the new one to make it easier to use by other people.
@@ -1,22 +0,0 @@ | |||
{ | |||
"configurations": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no reason to the vscode files
@@ -0,0 +1,3354 @@ | |||
<?xml version="1.0"?> | |||
<!DOCTYPE protocol SYSTEM "messages.dtd"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should be removed, only use it for your own testing. If you need to include a new message, make a PR in pprzlink and update the submodule in paparazzi.
>>> np.array([5*np.sin(th)]).T + np.array([1.9685, -1.9685]) | ||
--> | ||
|
||
<force name="fr_couple1" frame="BODY" unit="LBS"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should consider replace these pairs of forces by moment
like this: https://github.com/paparazzi/paparazzi/blob/master/conf/simulator/jsbsim/aircraft/simple_quad_wind.xml#L234
<telemetry> | ||
|
||
|
||
<process name="Main"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is not needed to include this file in the PR.
@@ -57,7 +57,7 @@ | |||
#endif | |||
|
|||
/* Default powerbrick values */ | |||
#define DefaultVoltageOfAdc(adc) ((3.3f/4096.0f) * 10.3208191126f * adc) | |||
#define DefaultVoltageOfAdc(adc) ((3.3f/4096.0f) * 18.0f * adc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's very different from original value. Was it really wrong before ? Or is it specific to your board ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was not wrong before. A different value should IMHO have been set in airframe file like:
<section name="BAT">
...
<!-- Changed since this airframe has a non default 3DR type powerbrick for Current and Voltage measurement -->
<define name="VoltageOfAdc(_adc)" value="(3.3f/4096.0f) * 18.0f * adc"/>
...
@@ -26,6 +26,10 @@ | |||
#include "firmwares/rotorcraft/stabilization.h" | |||
#include "firmwares/rotorcraft/stabilization/stabilization_attitude_common_int.h" | |||
#include "firmwares/rotorcraft/stabilization/stabilization_attitude_ref_quat_int.h" | |||
#include "math/ActiveSetCtlAlloc/src/common/solveActiveSet.h" | |||
#ifdef USE_CHIBIOS_RTOS | |||
#include <ch.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to avoid including OS specific code here, are you sure it is really needed ?
#include "setupWLS.h" | ||
#include <math.h> | ||
|
||
void setupWLS_A( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please format the code (and the rest of this lib). There is a script for that at the root of the project (fix_code_style.sh
).
|
||
*cond_est = *max_sig / ((min_diag2 > 1e-10) ? min_diag2 : 1e-10); | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing end of line
#include <math.h> | ||
#include <stdint.h> | ||
|
||
void pprz_cholesky_float(num_t **out, num_t **in, num_t* inv_diag, int n) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not using the function that is already in the math lib ? In general, why the different solvers are not at the root of the math folder if they are generic (cholesky, qr, ...) ?
* This is part of the qr_solve library from John Burkardt. | ||
* http://people.sc.fsu.edu/~jburkardt/c_src/qr_solve/qr_solve.html | ||
* | ||
* It is slightly modified to make it compile on simple microprocessors, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this solver really more efficient than the SVD-based solver already in the math lib math/pprz_matrix_decomp_float.h
?
As part of my masters thesis, I implemented optimized least-squares solvers for Weighted Least-Squares control allocation with bound-constraints on the actuators. To achieve significant improvements, the matrix factors are updated instead of re-computed at each iteration of the solver. Additionally, sparsity of the matrices are exploited, and "warm-starting" is implemented (initializing with the solution at the previous sample).
This PR contains
math/ActiveSetControlAlloc
(QR_naive is largely identical to the previous WLS allocator, plus some fixes)stabilization_indi.c
To Do:
I would appreciate some advice of what of this should/should not go into this PR to master!