Add motor mixing library and demo#81
Conversation
- Changed input type from int32_t raw SBUS to normalized floats - Thrust: 0.0-1.0, Roll/Pitch/Yaw: -1.0 to 1.0 - Implemented matrix-based motor mixing using factor coefficients - Single mixing algorithm works for all frame types - Easy to add new configurations (hex, octo, etc.) - Added proper output normalization to handle saturation - Maintains control ratios when limits are exceeded - Improved initialization: MIXER_Init() must be called before MIXER_AddMotor() - Updated demos to use new float-based API with SBUS normalization - Removed receiver value normalization from mixer (belongs in input layer)
…ase 1/3 scaling) and dynamic normalization options to preserve thrust authority.
| const float scale = 1.0f / 3.0f; | ||
| mixer_in->roll *= scale; | ||
| mixer_in->pitch *= scale; | ||
| mixer_in->yaw *= scale; |
There was a problem hiding this comment.
Shouldn't throttle be normalized here as well? It seems that with sufficiently large throttle this will still cause saturation.
There was a problem hiding this comment.
Thrust has to be here too. Also we could have a configurable factor between the thrust part and the ypr part, letting you chose between maximum speed when not turning vs turning ratio.
| const float c_max = fabsf(mixer_in->roll) + fabsf(mixer_in->pitch) + | ||
| fabsf(mixer_in->yaw); | ||
|
|
||
| const float headroom = 1.0f - mixer_in->thrust; |
There was a problem hiding this comment.
Shouldn't we be worried about going below zero?
Something like: headroom = min(thrust, 1-thrust) might be better.
There was a problem hiding this comment.
Yeah, you are definitely right. I resolved this, please check it out!
Thanks for the review.
| MIXER_Input_Type *mixer_in) { | ||
| switch (mixer->norm) { | ||
| case MIXER_NORM_STATIC: | ||
| const float scale = 1.0f / 3.0f; |
There was a problem hiding this comment.
You use fixed factors here, but motor factors could change, at least you made a function to make them configurable. If we don't want them to be configurable (always perfect 4 motor x configuration) we could just encode it to a const struct.
There was a problem hiding this comment.
I changed hardcoded scale here to configurable Thrust to RPY weight ratio in pre-compile time using Kconfig symbol for it. @nikolaptr suggested that Thrust should have higher priority over Pitch and Roll, and Pitch and Roll should have higher priority over Yaw.
For now, I only implemented T to RPY ratio, but I can adapt it to fully match the @nikolaptr idea.
| mixer_in->roll *= scale; | ||
| mixer_in->pitch *= scale; | ||
| mixer_in->yaw *= scale; |
There was a problem hiding this comment.
Not all axes are made equal when it comes to the multirotor stabilization, pitch and roll are often prioritized over yaw, as they are more useful for stabilization
Often the yaw is discarded entirely in favor of pitch and roll, which are discarded in favor of thrust.
This is good enough for now but let's revisit later
There was a problem hiding this comment.
That makes sense, yaw definitely has the lowest priority in stabilization when you think about it. Problem is that I don't have any metrics to compare if I manage to improve the saturation handling except I only can watch signals on LA. Maybe we can make some primitive HITL for mixer tuning, we will see.
Thanks for the review!
…STATIC_NORM_THRUST_WEIGHT
| } MIXER_Input_Type; | ||
|
|
||
| typedef struct { | ||
| float roll_factor; // -1.0 to 1.0 |
There was a problem hiding this comment.
What is the point of these comments? We've already established that these hold only for 4X, they would be different for different configurations.
There was a problem hiding this comment.
Yeah you're right, these comments are useless
| typedef struct { | ||
| ESC_Inst_Type motor_arr[MAX_MOTOR_INSTANCES]; | ||
| MIXER_Motor_Factors_Type motor_factors[MAX_MOTOR_INSTANCES]; | ||
| float motor_outputs[MAX_MOTOR_INSTANCES]; |
There was a problem hiding this comment.
Why do we buffer these? If it is actually needed can we call them last_motor_outputs or similarly?
| break; | ||
|
|
||
| case MIXER_NORM_DYNAMIC: | ||
| const float c_max = fabsf(mixer_in->roll) + fabsf(mixer_in->pitch) + |
There was a problem hiding this comment.
Why take absolute values here? What is c_max?
There was a problem hiding this comment.
c_max represents the worst-case control magnitude that could affect any single motor.
We use absolute values because each motor has different mixing factors (+-1). For a given motor, the roll/pitch/yaw contributions could all add up in the same direction.
| static void normalize_control(MIXER_Inst_Type *mixer, | ||
| MIXER_Input_Type *mixer_in); | ||
|
|
||
| MIXER_Error_Type MIXER_AddMotor(MIXER_Inst_Type *mixer, ESC_Inst_Type *esc) { |
There was a problem hiding this comment.
I still dislike this API, see previous comments about it.
| * [2] [4] | ||
| * BACK | ||
| * | ||
| * Initialization flow should be done like in this code snippet: |
There was a problem hiding this comment.
See previous comment, requiring the user to follow this procedure is just waiting for bugs to happen, and it is useless boilerplate. Bundle the motor info and the configuration info together and pass them to MIXER_Init() once
There was a problem hiding this comment.
Well, the user needs to follow some procedure. Motors still need to be initialized by position on the frame. You say I need to bundle motor info and config info together, but there is still room for bug, the user still needs to initialize motors in the correct order (front right/left, back right/left).
Don't get me wrong, I don't like this API either, but it's the only thing I could come up with at the time. I'm afraid if I hardcode the quad config now, we will have too many problems in the future supporting different frame configurations that could include configs with more motors.
If I put 4 previously initialized esc structs to the mixer instance struct, I don't have information which esc is tied to which motor on the frame. I don't have any nice idea how to handle this, except to have something like this:
typedef struct {
...
ESC_Inst_Type esc_front_left;
ESC_Inst_Type esc_front_right;
ESC_Inst_Type esc_back_left;
ESC_Inst_Type esc_back_right;
...
} MIXER_Inst_Type;
,where the user will populate specific motors on the frame. But this is stupid and hardcoded to specific frame configuration, and this still leaves room for mistakes.
I need more clarification here, I don't see the ultimate fool-proof way of handling all possible issues.
There was a problem hiding this comment.
You can use a discriminated union, something like:
typedef struct {
ESC_Inst_Type esc_front_left;
ESC_Inst_Type esc_front_right;
ESC_Inst_Type esc_back_left;
ESC_Inst_Type esc_back_right;
} Mixer_Quad_X_Motors;
typedef struct {
enum MIXER_UAV_Cfg_Type;
union {
Mixer_Quad_X_Motors quad_x;
Mixer_Triangle_Motors triangle;
....
};
} Mixer_Motors;…p useless comments
| mixer->initialized = false; | ||
|
|
||
| switch (uav_cfg) { | ||
| case MIXER_UAV_CFG_QUADROTOR_X: |
There was a problem hiding this comment.
I somewhat dislike checking for geometry all over the place in this file. Maybe just do a top-level switch/case here and call static functions to init geometry-specific stuff, i.e:
switch (uav_cfg) {
case MIXER_UAV_CFG_QUADROTOR_X:
setup_quadrotor_x(...);
break;
default:
return MIXER_INVALID_CFG;
}
There was a problem hiding this comment.
Valid. I will consider this in a new update
| return MIXER_INIT_ERROR; | ||
| } | ||
|
|
||
| if (!mixer->initialized) { |
There was a problem hiding this comment.
Do we actually need this field? If user didn't call init we're dealing with a garbage value here anyway...
There was a problem hiding this comment.
MIXER_AddMotor is ugly API I came up with. @DNedic had some ideas to solve this, where user doesn't need to respect the complicated initialization process. I will probably remove MIXER_AddMotor totally and just have one init call
There was a problem hiding this comment.
It's not just about this API, this field is checked in other APIs as well.
| static MIXER_Error_Type apply_motor_outputs(MIXER_Inst_Type *mixer) { | ||
| for (uint8_t i = 0; i < mixer->motor_instances; i++) { | ||
| ESC_Error_Type esc_status = | ||
| ESC_SetSpeed(&mixer->motor_arr[i], mixer->motor_outputs[i]); |
There was a problem hiding this comment.
Hm, I'm not completely sure how I feel about this from an architectural standpoint.
I guess I'm fine with this especially for initial implementation but I'd prefer if mixer didn't "own" ESCs but instead just did input transformation i.e:
MIXER_Execute(mixer, input, output)
From my standpoint this has multiple benefits:
- Mixer code is simplified, e.g.
MIXER_AddMotorAPI is no longer necessary (@DNedic ) - Mixer can easily be unit-tested completely isolated from the hardware
- Mixer outputs can be logged or emitted via telemetry without polluting either mixer or ESC code
There was a problem hiding this comment.
I totally agree. That would make unit tests much easier to implement, and could help with SITL too. I will start implementing this soon
This PR adds motor mixing library for quadcopters and demo that showcases the usage of the library.