Skip to content
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

fix thermal calibration if a sensor does not have temperature sensor #15671

Merged
merged 2 commits into from
Sep 5, 2020

Conversation

NicolasM0
Copy link
Contributor

Describe problem solved by this pull request
If a sensor does not have temperature measurement, the onboard thermal calibration of other sensors will never finish.

This PR also change the message displayed as an error by thermal compensation module if a sensor does not have a thermal compensation. It is now the normal use case for some autopilots (eg: CUAV V5+).

Describe your solution
Sensors with no temperature measure are skipped.
The compensation not found error is now displayed as an info.

Test data / coverage
Tested on V5+ with no temperature data on gyro 2

Additional context
related to :
#15644
#15582

@NicolasM0
Copy link
Contributor Author

@dagar

@@ -67,7 +67,7 @@ void TemperatureCompensationModule::parameters_update()
int temp = _temperature_compensation.set_sensor_id_gyro(report.device_id, uorb_index);

if (temp < 0) {
PX4_ERR("%s init: failed to find device ID %u for instance %i", "gyro", report.device_id, uorb_index);
PX4_INFO("%s init: failed to find device ID %u for instance %i", "gyro", report.device_id, uorb_index);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could make this message a little more clear while we're here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No temperature calibration available for gyro device id (N)?

PX4_WARN("Result Accel %d does not have a valid temperature sensor", sensor_index);

uint32_t param = 0;
set_parameter("TC_A%d_ID", sensor_index, &param);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, but worries me slightly. We know about the cases that don't have temperature at all (now reporting NAN), but I'm not absolutely certain it's impossible for other sensors to never publish NAN (eg sporadic read error, etc).

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, better to be sure on that point. I think I can reverse the logic : initialize with no temperature sensors and change that when a temperature measure is received. Calibration will exit immediately if the first temperature measure of all sensors is nan ( I hope it is not the case juste afetthe boot ...)

Copy link
Member

@dagar dagar Sep 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calibration will exit immediately if the first temperature measure of all sensors is nan ( I hope it is not the case juste afetthe boot ...)

Briefly thinking about this I'd say it's unlikely, but also not impossible.

The structure of many of the newer IMU drivers is that the temperature is read AFTER the FIFO. This doesn't apply to the sensors like the ICM20602 that have the temperature in the FIFO. https://github.com/PX4/Firmware/blob/7e5e8259f3bc644185569cb2292fd5ebd6bb532f/src/drivers/imu/invensense/icm20689/ICM20689.cpp#L265-L269

That means technically the very first sensor_{accel, gyro} publications with have a temperature of NAN. Now, these drivers start much earlier than the temperature_compensation module and run at a much higher priority, but some of them do have fairly long initial reset periods (30-100 ms) before that first publication.

Let's move forward with this change and keep an eye on it. I wouldn't be opposed to restructuring the drivers slightly so that we go out of our way to never publish NAN as a temperature unless there's no sensor or something is actually busted.

@dagar dagar merged commit a979845 into PX4:master Sep 5, 2020
@NicolasM0 NicolasM0 deleted the pr_thermal_calibration branch September 16, 2020 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants