IMU configuration

[Fabien-B]

Hi @fvantienen, @gautierhattenberger
I think we should discuss about the IMU/sensors configurations.
This message is also supported by @FlorianSan 😃

the problems

• the naming:
  When IMU_BODY_TO_IMU_[PHI/THETA/PSI] are defined, IMU does not refers to the sensor, but more to the reference frame of all sensors. Therefore, I think it could be named BODY_TO_BOARD.

• changing meaning:
  Before the following line, the body_to_sensor field seems to contain a board_to_sensor rotation matrix. (can you confirm?)
  It makes it harder to understand the code.
  

  paparazzi/sw/airborne/modules/imu/imu.c

  Lines 386 to 387 in 9c1e21d

  	int32_rmat_comp(&body_to_sensor, body_to_imu_rmat, &imu.gyros[i].body_to_sensor);
  	RMAT_COPY(imu.gyros[i].body_to_sensor, body_to_sensor);

• over-complicated retro-compatibility:
  There is more than 130 lignes of macro preprocessing for the configuration in imu.c. Don't you think it's too much?
  Can we drop the old way of defining the configuration and focus on the new way?

• As much as I like when it's unambiguous, the configuration for a sensor is awfully long:
  <define name="ACCEL_CALIB" value="{{.abi_id=24, .calibrated={.neutral=true, .scale=true, .rotation=true},.neutral={-10,-47,-130}, .scale={{36821,8142,5741},{60003,13319,9358}}, .body_to_sensor={{0,-16383,0,16383,0,0,0,0,16382}}}}"/>
  Most of this line is generated by the calibration tool so it's ok for me, but adding the body_to_sensor field is tedious.
  Maybe this field should be separated from the rest, especially because:

  • it's not something that will change a lot,
  • I suspect the name is wrong at this time (cf 2nd point).
  • Writing a matrix is tedious. Could it be eulers angles ? Or quaternions ?

the solutions

What can we do about that is up to discussions!

A possible solution is to write the rotations in a separated file that could be handled either by the airframe generator, or by the calibration tool.
This file would be an optional argument of calibrate.py. If present, the body_to_sensor part of the structure would also be generated, making the copy-paste easy again.
The rotation file could look like this:

# body to board rotation
board:
    phi: 45
    theta: 0
    psi: 0

# board to sensor rotations
sensors:
    - name: "accel"
      abi_id: 9
      # default orientation
    - name: "mag"
      abi_id: 3
      phi: 0
      theta: 0
      psi: 90
    - name: "external mag"
      abi_id: 3
      phi: 0
      theta: -90
      psi: 90

Using a file like this, the body_to_imu_rmat could entirely be removed from the code, leaving just the body_to_sensor fields, without the naming issue.

What do you think ?

[gautierhattenberger]

The problem I see is that we'll have to manage a bunch of extra files, not explicitly related to airframe, although it is the case in reality.
A part of the problem is that defining structures (and matrices) in the airframe file is a pain. This could be made easier with something like:

<define name="SOME_STRUCT">
  <field name=".abi_id" value="42"/>
  <field name=".calibrated" value="{.neutral=true, .scale=true, .rotation=true}"/>
  <field name=".body_to_sensor">
    <field name=".phi" value="0."/>
    <field name=".theta" value="0."/>
    <field name=".psi" value="90."/>
  </field> <!-- because recursive definition is so much fun -->
</define>

and

<define name="SOME_MATRIX">
  <row name="roll" value="{1., 2., 3., 4.}"/> <!-- note that 'name' is useless and could be removed -->
  <row name="pitch" value="1., 2., 3., 4." type="float[]"/> <!-- alternate format -->
  <field value="{1., 2., 3., 4.}"/> <!-- another option with reuse of 'field' without 'name' to indicate a matrix -->
</define>

About the issue on rotation matrix, I agree the naming is not correct and defining matrices (for rotations) in config file is not nice. It could have been in Euler angles format with initialisation of the matrix at runtime rather than direct static initialisation.

[Fabien-B]

The file could be related to the airframe file if it is mentioned in it, then it's semantically very similar to the include directive.

<include href="rotations.yaml" processor="imu_rotation.py"/>

I am not in favor of the solutions you propose as this is still hardly readable, and will still require specific handling by the OCaml generator.
Managing complex data specific to a module as always been a pain, and it will always be if we stick to the XML only.
Allowing other file types is just a step further on the idea of defining an airframe with multiple files.

Maybe at a longer term we need to allow modules to register "preprocessors" with standardised "hooks" to generate code to be integrated in the airframe/flightplan/settings.

I am in favor of resolving this specific case with a user friendly solution, even if this is a bit "hacky", and start thinking about a more general solution to handle complex specific data in a separate discussion.

[fvantienen]

• For the naming I approve this can be changed and should be changed. Though board is also ambiguous, since it happens for all sensors even the off-board sensors. I think calling it IMU frame is maybe better?
• It contains a body_to_sensor matrix and not a board_to_sensor matrix after the init function and when the sensor has set a default. So this init function combines the default imu_to_sensor with the body_to_imu which results in a single matrix containing the body_to_sensor. This way the matrix can be used directly when getting the raw values to translate them to body frame with a single matrix multiplication.
• For me the old way can be fully removed and was also how I proposed the PR, but then for compatibility @gautierhattenberger added back the old defines.
• I agree
  • Yes that is tedious, but my idea was to change the generator to create this struct in the airframe.h. That way it would be easier to understand and add the rotation matrix.
  • It is true that the naming is incorrect there, since it is actually a imu_to_sensor matrix but this was chosen to create another struct and reuse the same struct.
  • Yes true, but with the matrix you can do more stuff and sometimes I prefer it as it is easier to understand as there is no order or direction in which the operations are performed. A generator would solve this problem and maybe gives both options.

I prefer to keep it in the airframe and have something like this (can also be different in name and maybe like all have the name field but just ad an example which is really understandable:

<calibration type="accel" id="20>
  <neutral x="-10" y="100" z="5"/>
  <scale x="1.015643" y="1.08946" z="0.98747"/> <!-- with relation to the defaults, such that you can easily see your sensor errors -->
  <rotation roll="90" pitch="0" yaw="5" type="deg"/> <!-- The order in which you enter them is the way they are rotated -->
</calibration>
...
<calibration type="accel" id="20>
  <rotation roll="1,0,0" pitch="0,1,0" yaw="0,0,1" type="matrix"/>
</calibration>

[gautierhattenberger]

The general policy about backward compatibility is that only major release should break it. So I don't mind breaking and cleaning things, however I would like to first make a stable release (v6.4, should have been done already anyway), then start a 7.x series with the proposed changes. And the proposed changes / specs / objectives should be clarified a bit more in my opinion, as we have several options and it is probably a bad idea to implement them all.

• adding a pre-processor to href seems fairly easy to integrate, it can simplify the addition of external complicated things, but I don't think it should be a mandatory element at the moment, you should be able to do a few things from original XML as well
• extending XML definition for calibration node would help a lot for clarity, but needs a little bit of work for parsing and code generation (and no, OCaml is not that awful)
• defining matrices is not only useful for calibration (e.g. efficiency matrices for INDI), so improving, even a little, would probably help
• it was discussed during IMAV that we could split the airframe into multiple files (like settings)
• more elements could be integrated to the modules XML

In the meantime, changing a few names can be done to clarify the different rotation. I'm also in favour of defining the rotation matrices by angles (and compute the rotation matrix at runtime). It is human-readable and gives more guarantees that the matrix is a proper rotation matrix, without too much approximation due to number of digits.

[fvantienen]

Yes that can be done in the meantime, but changing it to angles is not always enough for us. We had some bad IMU's with inverted axis, so preferably we also keep the matrix options.

[fvantienen]

For the depreciation can we at keep a list of all the items that needs to be removed/changed in the next release somewhere?

[gautierhattenberger]

I think the best is to open pull requests and issues and tag them for next release.