-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
Generate mixers from description of multicopter geometry #8063
Conversation
dd052f8
to
0a099f5
Compare
291e060
to
a541f5c
Compare
@jlecoeur shouldn't |
If toml is available as an ubuntu package and through pip it doesn't need to be included at all. |
In fact, that reminds me that we should drop the gencpp and genmsg submodules. #8089 |
a541f5c
to
cb44ee6
Compare
I suggest to drop quad_v. It is used in 0 configs. It was introduced for TBS_discovery, but the TBS_discovery config uses quad_w : https://github.com/PX4/Firmware/search?utf8=%E2%9C%93&q=quad_w&type=Code |
I added quad_yThis one has two coaxial rotors in the back (picture)
On the 3rd column we see that only the two rear motors contribute to yaw. Also, as only the two front motors contribute to roll, they must provide more thrust for rolling than for pitching where all 4 motors contribute. The script takes it into account and the first column (roll) has larger gains than the 2nd column (pitch). quad_vtailThis one has the two rear motors tilted to increase yaw authority (picture). This kind of platform has lot of roll/yaw coupling and it is difficult to find gains by hand, here is the output of the script:
First thing, we see that the CG is too much forward and the two front motors are more loaded (4th column). Then the two front motors contribute mostly to roll, with small compensations from rear motors (1st column). On yaw (3rd column) we see that the tilted motors are less loaded. When looking at the non-normalized mix, we see that the yaw gains are lower than the roll and pitch gains, this is because the platform has LOT of yaw authority:
For comparison, on non-normalized
Note that to maintain compatibility with current mixers, non-normalized mixes are not used. |
5c7a477
to
3dc1a02
Compare
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 looked through this and added some comments. I like the generic approach.
Can you please rename geom
to geometry
(geoms
-> geometries
) in sources and file names? I prefer to type less too, but readability is more important.
I suggest we merge this after the release to avoid any regressions that this might have.
Since this adds a new config language, we should consider it for params as well (#6115). I think it's a good candidate, as it supports multi-lines.
lib/terrain_estimation | ||
lib/mathlib | ||
lib/mathlib/math/filter | ||
lib/micro-CDR |
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 did you add lib/micro-CDR
?
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.
Oops that is a leftover from a rebase, will fix!
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.
fixed
# Generic Dodecacopter in X coax configuration, bottom half | ||
|
||
[info] | ||
name = "dodeca_bottom_cox" |
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.
name
is redundant with the file name, I'd use the file name to avoid inconsistencies.
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.
fixed
if args.files is not None: | ||
filenames = args.files | ||
else: | ||
filenames = glob.glob(args.dir + '/*.toml') |
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 use os.path.join
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.
fixed
def generate_mixer_multirotor_header(geom_list, use_normalized_mix=False, use_6dof=False): | ||
''' | ||
Generate C header file with same format as multi_tables.py | ||
TODO: rewrite using templates (see generation of uORB headers) |
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.
We can use a jinja template here, like https://github.com/PX4/Firmware/blob/master/src/modules/systemlib/param/templates/px4_parameters.c.jinja
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.
Yes that would be cleaner.
I was not sure whether the script should generate one big file (current behaviour), or one file per geometry (like uORB and params).
I decided to use the same output format as mixer_table.py (though writing to a buffer instead of stdout!), and I left templating for later.
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.
One big file is fine, I'd say. I'm ok with changing to a template in a later step.
@@ -0,0 +1,356 @@ | |||
#!/usr/bin/env python |
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.
Nice script!
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.
🕺
662f6e6
to
19f15df
Compare
This is amazing! Thank you. |
25f2548
to
daa5b56
Compare
Rebased again due to a minor conflict in integrationtests/run_tests.bash |
7335306
to
19d4bd8
Compare
19d4bd8
to
7e954a5
Compare
@MaEtUgR @bkueng your reviews were dismissed by the last rebase but there was no functional code change. |
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.
No changes, renewing approval.
I think this is good to go. What I'd like to potentially do later is move from the more obscure .toml files to something that is more widely available. This is relevant as this will facilitate generating the UI in QGroundControl or the documentation on the website. @bkueng Does this PR affect the generation of mixer documentation for the documentation? If it does, we should not be merging it yet. |
No. Let's get it in then! @jlecoeur can you make sure the toml files get documented? Maybe by adding a sample file that explains all of the variables? |
@jlecoeur Very cool stuff. I'm wanting to do some work on the mixer and wanted to make sure I understood how the mixer scalers are being generated. For the most part it all makes sense but I had some questions about normalization that I'm hoping you can help me with. 1: Can you explain why it's necessary to normalize the B matrix? 2: Why is the normalization of roll and pitch linked and why is it divided by For a completely symmetric geometry such as the quad_x I would think that the normalized matrix would simply be:
But instead it is being computed as:
Is this right? |
Hi @nanthony21
If you want to remove the normalization, please do! I would be happy to help if I can. You can open a new issue to move the discussion over. |
@jlecoeur Thanks, that helps. Are there drawbacks to having it normalized? I wouldn't want to go changing everything if there isn't a good reason to. |
The drawbacks are:
However nb 1 and 3 are also advantages in terms of the generality of PX4. |
This should and is reflected in the multicopter attitude controller. Because the attitude controller might choose a different optimal non-linear "attitude error direction" depending on less yaw autority which it can't do if the output is scaled linearly afterwards, see https://github.com/PX4/Firmware/blob/19a4f0988c42679207b9da101d54bc61432cdfb7/src/modules/mc_att_control/mc_att_control_main.cpp#L811 |
@MaEtUgR I think you are raising an important point but I am not sure that I follow you. I do not see how the attitude gain
Here is what I have in mind in the case of non-normalized mixer:
|
On the contrary, I'd support it. I only wanted to inform that scaling down yaw output linearly is not the optimal solution for multicopter attitude control. That's part of the insight of my master thesis and the attitude controller changes I recently introduced (#8003). This is generally independent of (non-)normalized thrust. You're plan seems in theory super nice. You should even know the spacial geometry and hence the lever lengths with the mixer and have thrust (unit: N) as output. Then it should pass a motor model and get translated to the output corresponding the ESC e.g. [0,1] or rpm. I was working at a lab during my studies which had the complete model in place.
But you should not forget the problems come in practise when you want to support a wide variety of vehicles without a model. I think it needs to be our goal to be backwards compatible in a sense that existing and arbitrary setups still work unrestricted. But enable and encourage usage of model based features. I think for a normal user/short-term prototype it's reasonable to measure mass and frame geometry but usually not moment of inertia and ESC signal to thrust. I think others like e.g. @priseborough would also have interest in units. |
closes #7382
adds script that generates multicopter mixer from the geometry of the multicopter. For reference, see this notebook: https://github.com/jlecoeur/servo_mix_matrix/blob/master/servo_mix_matrix.ipynb
adds folder containing one config file per multicopter geometry
auto-generates mixers by listing the *.toml files in this directory
Add geometries
mixer_multirotor.cpp
Compare output with previous script (i.e
mixer_multirotor_normalized.generated.h
andmixer_multirotor_legacy.generated.h
Fix CI
px4io/px4-dev-ros:pr-jlecoeur
(no space left on device)make px4fmu_firmware
Flight tests
For later, the script is ready for 6dof control. There is an option
--sixdof
. For example see the geometry "quad_x_pusher" (or quadplane if you prefer) that has a 5th rotor to generate forward thrust.@dagar @bresch
edit: for sake of compatibility, the mixer matrices are normalized so that they match the current mixers (see option
--normalized
). However that voids the advantage of modeling dimensions, mass, intertia, and thrust and moment coefficients.This PR is a first step that introduces the script and the config files. Next independent steps will be to enable 6dof, and to remove normalization.