Avoid duplicate temperature compensation in EAHRS

[ohyaiamhere]

As explained in issue #25792, there is an issue where EAHRS performs duplicate temperature compensation for certain sensors.

The enum TempCalibration of the type TempCal is adjusted to see if the sensors is of the MicroStrain7 type (as mentioned in the issue) and set the enum as IsTempCalibrated. Else the enum is set as IsNotTempCalibrated.

In the inertial sensor section, this enum, which was sent by the message ins_data_message_t, is checked, and if there is temperature compensation, a new bool IsTempCalibrated is set to true. This bool then checked at places where temperature compensation due to HAL_INS_TEMPERATURE_CAL_ENABLE occurs and the code inside is executed if the bool is false.

[peterbarker]

Why are the temperature calibration values non-zero for the external device?

[ohyaiamhere]

Why are the temperature calibration values non-zero for the external device?

Hello Peter,

The function I had created earlier was not correctly attaching the TempCal data with the ins_data_message_t

I have modified the commit to deal with this issue.

[peterbarker]

This is only a partial review - just want to make sure I know what you're trying to do here.

So I think what you're trying to do here is cope with the fact that Microstrain can supply IMU data which has already been adjusted for the temperatures in which the microstrain is operating?

... and that thus you do not want to learn any offsets from the microstrain when we are learning?

Thing is that I'm pretty sure there are many dragons with using tcal and externalahrs IMU data.

I'm particularly concerned about what happens if you disable the IMU data from the externalahrs - do we start to apply the learnt coefficients to the wrong IMU now?!

[peterbarker]

OK, so this is much simpler than it was :-)

We'll need a report on what testing has been done for this PR.

[peterbarker]

.... so you still haven't put this back into the structure... and these casts weren't required before when using the structure.... I think you stopped adding something in here, so perhaps just remove this patch as it doesn't seem to be doing anything.

[peterbarker]

Missing the point. Continue to use the structure, cast only what is required when putting data into the structure.

[ohyaiamhere]

typedef struct {
        Vector3f accel;
        Vector3f gyro;
        float temperature;
        TempCal TempCalibration = TempCal::IsNotTempCalibrated;
    } ins_data_message_t;

Due to this modification being added in AP_ExternalAHRS.h the structure cannot be reused in either of the two files. Each time I try to, it throws an error message.

../../libraries/AP_ExternalAHRS/AP_ExternalAHRS_MicroStrain5.cpp: In member function ‘void AP_ExternalAHRS_MicroStrain5::post_imu() const’:
../../libraries/AP_ExternalAHRS/AP_ExternalAHRS_MicroStrain5.cpp:131:9: error: no matching function for call to ‘AP_ExternalAHRS::ins_data_message_t::ins_data_message_t(<brace-enclosed initializer list>)’
  131 |         };

So another option would be to figure out how to set this default value using another method, and changing this line to

TempCal TempCalibration;

I notice this only in the two MicroStrain drivers but not the others because they use direct assignment, as I had used in the patch before this new one. Like the InertialLabs driver below

        state.accel = ins_data.accel;
        state.gyro = ins_data.gyro;

[ohyaiamhere]

OK, so this is much simpler than it was :-)

We'll need a report on what testing has been done for this PR.

I have explained what I have done below:

Implementation logic

The enum, called TempCal is carried in the ins_data_message_t, from the backend of the AP_ExternalAHRS and the drivers of the individual devices, to AP_InertialSensor. where it is received in pkt

The value of the enum is then used to change the value of tcal(i).enable to AP_InertialSensor_TCal::Enable::Disabled for that instance i of the sensor. This is done to make sure that, whenever this packet is received, the autopilot turns off the temperature calibration for that particular instance of sensor. This implementation is cleaner than creating another variable
externally_temperature_calibrated.

Testing

Using units tests with GNU Debugger, the value of the incoming packet was checked to make sure that the correct value of TempCal was being carried for the device which is already calibrated, and the device which isn't.

---

For MicroStrain 5
./Tools/autotest/autotest.py --debug --gdb build.Plane -B AP_InertialSensor::handle_external test.Plane.MicroStrainEAHRS5

---

For MicroStrain 7
./Tools/autotest/autotest.py --debug --gdb build.Plane -B AP_InertialSensor::handle_external test.Plane.MicroStrainEAHRS7

---

Please let me know what you think, and how else can this be tested. I do not have access to hardware myself, so SITL testing is all I am able to do.

EDIT: Changed the assignment of tcal(i).enable to before the recursive call instead of after.

[tolesam]

@ohyaiamhere Can you add it for SBG driver as well please? SBG's INS are always calibrated in temperature.

[ohyaiamhere]

@ohyaiamhere Can you add it for SBG driver as well please? SBG's INS are always calibrated in temperature.

I can create a commit that adds a cached.sensors.ins_data.TempCalibration around line 633 in the file AP_ExternalAHRS_SBG.cpp

    if (updated_ins) {
        cached.sensors.ins_data.accel = state.accel;
        cached.sensors.ins_data.gyro = state.gyro;
        cached.sensors.ins_ms = now_ms;
        AP::ins().handle_external(cached.sensors.ins_data);
    }

where ins_data is updated.

Although I couldn't find a unit test for this sensor and I do not have the hardware to test it myself, I can make another commit in this pull request. Let me know what you think @peterbarker .

[tolesam]

@ohyaiamhere Can you add it for SBG driver as well please? SBG's INS are always calibrated in temperature.

I can create a commit that adds a cached.sensors.ins_data.TempCalibration around line 633 in the file AP_ExternalAHRS_SBG.cpp

    if (updated_ins) {
        cached.sensors.ins_data.accel = state.accel;
        cached.sensors.ins_data.gyro = state.gyro;
        cached.sensors.ins_ms = now_ms;
        AP::ins().handle_external(cached.sensors.ins_data);
    }

where ins_data is updated.

Although I couldn't find a unit test for this sensor and I do not have the hardware to test it myself, I can make another commit in this pull request. Let me know what you think @peterbarker .

That sounds good to me.
You're right for the unit test missing, I have to implement it.

[ohyaiamhere]

@ohyaiamhere Can you add it for SBG driver as well please? SBG's INS are always calibrated in temperature.

I can create a commit that adds a cached.sensors.ins_data.TempCalibration around line 633 in the file AP_ExternalAHRS_SBG.cpp

    if (updated_ins) {
        cached.sensors.ins_data.accel = state.accel;
        cached.sensors.ins_data.gyro = state.gyro;
        cached.sensors.ins_ms = now_ms;
        AP::ins().handle_external(cached.sensors.ins_data);
    }

where ins_data is updated.
Although I couldn't find a unit test for this sensor and I do not have the hardware to test it myself, I can make another commit in this pull request. Let me know what you think @peterbarker .

That sounds good to me. You're right for the unit test missing, I have to implement it.

I have created the commit. If you have the hardware, would it be possible for you to help test it?