-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Incremental Nonlinear Dynamic Inversion (INDI) position controller for the Crazyflie #568
Conversation
Hi and thanks for your pull request! Happy to finally get INDI in the position control as well! I will review the code in detail later this week and try it for myself. I just see a little something already, as the INDI controller is now defined as standard in your pull request. I will put a remark at the line of code. I will give the rest of the review later this week |
src/modules/src/stabilizer.c
Outdated
@@ -184,7 +184,7 @@ void stabilizerInit(StateEstimatorType estimator) | |||
|
|||
sensorsInit(); | |||
stateEstimatorInit(estimator); | |||
controllerInit(ControllerTypeAny); |
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.
Could you please return this to ControllerTypeAny? The controller type will be set later in the stabilizer.c code
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.
Done :)
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.
thanks! more comments are now coming up
@@ -168,7 +168,7 @@ PROJ_OBJ += crtp_commander_generic.o crtp_localization_service.o | |||
PROJ_OBJ += attitude_pid_controller.o sensfusion6.o stabilizer.o | |||
PROJ_OBJ += position_estimator_altitude.o position_controller_pid.o | |||
PROJ_OBJ += estimator.o estimator_complementary.o | |||
PROJ_OBJ += controller.o controller_pid.o controller_mellinger.o controller_indi.o |
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.
Could you add the position_controller_indi.o to line 169 instead, so that all position controllers are in the Makefile together?
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.
Done.
#define STABILIZATION_INDI_G1_P 0.0034724f | ||
#define STABILIZATION_INDI_G1_Q 0.0052575f | ||
#define STABILIZATION_INDI_G1_R -0.0015942f | ||
#define STABILIZATION_INDI_G2_R -0.11281f | ||
#define STABILIZATION_INDI_REF_ERR_P 3.57f | ||
#define STABILIZATION_INDI_REF_ERR_Q 3.57f | ||
#define STABILIZATION_INDI_REF_ERR_R 3.57f |
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 is quite a difference. How did you tune this? I guess this are the values for the INDI attitude controller right?
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.
The attitude INDI was quite uncontrollable (very slow and not very stable), that's why we have decided to re-estimate some of the parameters.
The gains (STABILIZATION_INDI_REF_ERR_P/Q/R
) were tuned manually. The control effectiveness parameters (G1, G2) of the INDI were tuned according to the paper "Adaptive Incremental Nonlinear Dynamic Inversion for Attitude Control of Micro Air Vehicles" (Section A. Parameter Estimation). I have uploaded two Matlab scripts (param_estimation.m
, import_logdata.m
) and the flight data (data_1901.csv
) I used to estimate these parameters. They are located in the /tools/param_est
folder.
@@ -149,7 +155,7 @@ void controllerINDI(control_t *control, setpoint_t *setpoint, | |||
} | |||
} | |||
|
|||
if (RATE_DO_EXECUTE(POSITION_RATE, tick)) { | |||
if (RATE_DO_EXECUTE(POSITION_RATE, tick) && !outerLoopActive) { |
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.
So I'm not exactly sure about this. So you made an outerloop boolean to give people the choice to use either PID or INDI for the position control? Just trying to to think if this is actually that necessary like this, as this enables people to change controllers on the fly, which results in both controllers being made and that might be overkilled. Could this also be handled with a define or a make flag? So that at build people can decide if they want position INDI or PID?
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.
The main reason why I introduced this variable is to differentiate between two main use cases I needed:
- Attitude INDI only
- Position and Attitude INDI
I can remove position PID from attitude INDI completely and at first structure the INDI controllers (into different files) as you suggested at the bottom. Thus, there will remain only two options:
- Attitude INDI only
- Position and Attitude INDI
which are controlled by the outerLoopActive
.
Later, (if needed) one can add position PID.
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.
Hmm yeah good point. Just leave as is for now. I don't think it is a big issue on a second thought and I actually would like to try out indi/pid with my position controller script that I just pushed to the python library.
src/modules/src/controller_indi.c
Outdated
|
||
static float r_roll; | ||
static float r_pitch; | ||
static float r_yaw; | ||
static float accelz; | ||
|
||
static vector_t refOuterINDI; // Reference values from outer loop INDI | ||
static float outerLoopActive; // if 1, outer loop INDI is activated |
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.
OuterloopActive is a boolean right? You should define this as a boolean and initialize it as = false. (don't forget to change this at the parameters as well)
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've changed it as you suggested, however I am not sure if there is a boolean type for parameters (that's why I defined it as PARAM_UINT8
).
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.
Indeed, there is no boolean for parameters, but uint8 is just fine. Thanks for changing it
src/modules/src/controller_indi.c
Outdated
rateDesired.yaw = yaw_kp*(attitudeDesired.yaw - state->attitude.yaw); | ||
//rateDesired.yaw = yaw_kp*(attitudeDesired.yaw - state->attitude.yaw); | ||
attYawError = attitudeDesired.yaw - state->attitude.yaw; | ||
if (attYawError > 180.0f) { |
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.
In PID controller file
attitudeDesired.yaw = capAngle(attitudeDesired.yaw); |
Anyway, if we get more controllers we probably need to think about making a separate file with controller helper functions, but that is something for the future.
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.
Done.
} | ||
|
||
|
||
PARAM_GROUP_START(INDI_Outer) |
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.
Could you call the parameter group instead: posCtrlIndi, to keep it similar with
PARAM_GROUP_START(posCtlPid) |
Do you also have parameters for velocity? if not you can also just call it 'ctrlIndi'
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 am not sure if I understand this. The PARAM
/LOG
groups of the position INDI do not only include the param/log variables for position and velocity, but also for acceleration, thrust, gains etc.. That's why I have put all these variables in one group that belongs to the position INDI, the same way it is done in the attitude INDI file. I renamed it from INDI_Outer
to posCtrlIndi.
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.
That is true, but we will think about the name change later. For now it is important that all the control parameters can be found at the same place as they are listed in alphabetical order in the parameter list in the cfclient.
Thanks for changing it
|
||
|
||
|
||
LOG_GROUP_START(INDI_Outer) |
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.
posCtrlIndi
Separate log group for velocity (velCtrlIndi)
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 see the answer to the previous comment.
}; | ||
|
||
// Compute transformation matrix from body frame (index B) into NED frame (index O) | ||
float M_OB[3][3] = {0}; |
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.
In general I would prefer if matrix acceleration of the stm is used, so using the arm_matrix_instance_f32 and such like in kalman_core.c. But this is not a huge part of the code. Let us know if you want to change this or rather save this for another pull request
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.
If possible, I would leave it for the next pull request.
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.
Oke no problem
So I added some more comments. It is mostly about coherence with the rest of the firmware, to make sure it is similar to how the PID and Mellinger is implemented. Also, since now there are 2 separate PID controllers, it might be good to separate the attitude INDI from the controller_indi.c in attitude_controller_indi.c. In this way it is similar to how it is handled for PID.c. So that you get the following structure of calling functions:
I hope that is a bit clear? |
…ed, param/log group renamed
The changes are sufficient for now. Thank you for implementing them. I will try out the indi controller with outerloop on and off in the lighthouse system one of these days. If that is going well, I can merge this pull request :) |
Cool! I forgot to mention something: the position ( |
Ah good you mentioned it. This will be a bit of a problem for compatibility... It would be best that as soon as you get the set point in your controller, you invert the y- and z-axis. The z-axis is defined to be upwards towards the sky in the crazyflie. |
Ah I see. So you give the setpoints as a manually parameter and not through the setpoint_t struct as all the others. This is quite important, so please look at the pid controller as reference:
Sorry to be strict on this, but it is necessary to use the same structure and manner as the other implemented controllers. That will keep the code manageable and will make it more future proof if more controllers are added. So this way, from the python script one can easily just choose an controller and give the set points and more people will use your controller. Let me know if it this will be possible for you to change or if you need some help! I can also make some changes to the code. |
I have noticed that the reference values are usually provided by the |
Ah ok. Since we would definitly want this in the code I will help you out. I will look at changing to code to be suitable for merging. In order to do that, I will need push access to this branch of your repo. Could you double check if I have so, you are able to select this in the pull request I think (https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork) |
alright perfect. I will see what I can do this week. I'll update here |
https://github.com/volmen3/crazyflie-firmware into volmen3-SA_INDI_outer_pos_pr
So just an update. I was able to verify that the parameter way indeed worked for the controller. I've implemented the setpoint way but it is not quite the behavior that I want so I haven't pushed it yet. So it still needs some more investigation, but that will be for next week:) happy easter! |
Thank you for the update. Wish you happy holidays as well! :) |
Fwew that took a bit more work than expected ':) I looked over the part in controller_indi.c that disabled the use for setpoints (the passthrough of the setpoints were in a modeDisable if statement). But I tested it and it worked. The attitude controller tuning seems indeed better but the position tuning can be a bit better. Right now the PID controller is faster still. But at least it is all sufficient to merge it all now. Some things that I would still want to see in the future is:
This is not necessary for this pull request, but I'll make an ticket/issue in github to remind ourselves in the future. In general I think that the entire controller structure in the crazyflie needs revamping. I've also been working on a controller tuning script in this repo (bitcraze/crazyflie-lib-python#146). Would be great if we could also add a tuner for the INDI eventually :) Anyway, look at the changes I made and say if you are okay with it. I removed some variables that were not necessary. |
Hey! Thank you very much for helping with the modifications! From my side all the changes are fine (especially if you think they suit well the overall handling of the reference values of the Crazyflie). By the way, what is the exact procedure to control the Crazyflie through the setpoint struct on the example of this new INDI position controller? Should one use the |
You can try the autonomousSequence.py in the crazyflie-lib-python repo in examples. In run_sequence you can set the controller with:
On the top of the script you can set the positions where the crazyflie will fly to. I set it to first (0,0,0.5,0) to make it just take off for a half a meter just to try it out. |
Thanks for providing the example. I have tried it and it worked! |
alright! I will squash and merge this pull request and make an issue for the wanted changes for the INDI controller :) Thanks again! |
Thank you! :) |
Hi @volmen3, we have some questions about the INDI controller on the forum. Could you take a look at it and answer if you have questions? Thanks! |
Hi @volmen3. We are currently busy with adding descriptions params and logs variables, for which INDI we still need some help. Also we want to access how to setup controllers in the firmware in the future and would like to hear your experience with it as well. Could you contact us by email: contact_at_bitcraze.io so that we can include you in the discussions? Thanks! |
ParamTab: UI fixes
Hello, Dear Bitcraze community, this is a pull request for another version of the position controller, known as Incremental Nonlinear Dynamic Inversion (INDI). This controller works as the outer loop of the already existing INDI attitude controller (inner loop). It was implemented in the framework of my semester thesis under the supervision of @EwoudSmeur and according to the following publication:
Ewoud J. J. Smeur, Guido C. H. E. de Croon, Qiping Chu "Cascaded Incremental Nonlinear Dynamic Inversion Control for MAV Disturbance Rejection"
Below are some notes regarding the implementation.
Files that have been modified:
Makefile
src/modules/src/stabilizer.c
(changed controller type)src/modules/interface/controller_indi.h
src/modules/src/controller_indi.c
(corrected some of the mistakes, implemented call of the position controller)Files that have been added:
src/modules/interface/position_controller_indi.h
src/modules/src/position_controller_indi.c
Unfortunately, I could not find a clear information/documentation about different modes and their influence on the behavior of the Crazyflie. So I implemented the control inputs (reference position) to be set in the "Parameters" section of the cfclient. If the current handling of the control inputs does not correspond to some of the internal guidelines, feel free to adjust it.
The INDI attitude controller works as before just right after the start of the Crazyflie, to activate the outer loop (position controller), the variable
outerLoopActive
(ctrlINDI
group) has to be manually set to1
. When the outer loop is active, the position controller is still in the waiting mode (this is done to avoid random oscillations of the thrust which happen if the position estimates are not accurate, e.g due to homogeneous pattern of the surface CF is placed on). So to activate the controller thearm
variable (INDI_Outer
group) has to be set to1
. Now, whenouterLoopActive
andarm
are set to1
, the position can be commanded through the following variables (INDI_Outer
group):pos_set_x
,pos_set_y
,pos_set_z
. After the landing, thearm
variable has to be set back to0
. To return to the mode, where only the INDI attitude controller is active, setouterLoopActive
to0
.