Fix temperature readout for Invensense IMUs #9054

mhkabir · 2018-03-10T21:23:27Z

This fixes the temperature readout for the MPU/ICM IMUs. The sensitivity parameters were originally incorrect. This is needed for enabling the Pixhawk 2.1 IMU heater and holding a temperature set-point.

mhkabir · 2018-03-11T04:10:06Z

TODO : other drivers also seem to have incorrect parameters.

LorenzMeier · 2018-03-11T21:36:35Z

Generally a great initiative, but we need to take into consideration what the side effect will be to boards that are already calibrated.

ksschwabe

@mhkabir : I think your temp sensitivity changes aren't correct. Please see my individual line comments.

ksschwabe · 2018-03-13T09:58:25Z

src/drivers/imu/mpu9250/mpu9250.cpp

+	//
+	// Sensitivity (in degC/LSB) is from the Invensense datasheet for the specific sensor
+	// Zero offset (in degC) varies from part to part, and this value is a good default
+	_last_temperature = (report.temp) / 340.0f + 21.0f;


Where do these numbers come from? Is the temp sensitivity not 333.87LSB/degC?

Yep, you're right. Thanks!

ksschwabe · 2018-03-13T09:59:53Z

src/drivers/imu/mpu6000/mpu6000.cpp

 	if (is_icm_device()) { // if it is an ICM20608
 		_last_temperature = (report.temp) / 326.8f + 25.0f;

 	} else { // If it is an MPU6000
-		_last_temperature = (report.temp) / 361.0f + 35.0f;
+		_last_temperature = (report.temp) / 340.0f + 36.0f;


Is the temp offset not 35 degC for the MPU6000?

Thanks, you are indeed right. I believe I had been looking at an older version of the datasheet. Will fix.

Do you know whether this sensitivity is related to a particular MPU6000 silicon revision (as in we will have to handle the temp conversion based on the silicon revision)? Or is it that there was a typo in the previous data sheets and they corrected it in the later version.

I am also not sure if I have the latest datasheets. These were the ones that I downloaded from the Invensense website, so as long as those are the most recent, then it should be alright.

ksschwabe · 2018-03-13T10:01:02Z

src/drivers/imu/mpu6000/mpu6000.cpp

+	// We calculate sensor temperature as :
+	// temperature_real = temperature_raw * temperature_sensitivity + zero_offset
+	//
+	// Sensitivity (in degC/LSB) is from the Invensense datasheet for the specific sensor


Should the units of sensitivity not be LSB/degC?

In the datasheet, yes. However, I defined it here as 1/(LSB/degC). Will make it clearer. Thanks.

mhkabir · 2018-03-13T15:17:27Z

@ksschwabe All addressed! Thanks for the corrections!

mhkabir · 2018-03-15T03:13:13Z

@LorenzMeier @dagar Options for merging this? People with current tempcal will need to recalibrate. Wish we had some form of parameter versioning so that we could reset the TC_* parameters from older versions and force a recalibration.

dagar · 2018-03-15T05:47:47Z

This is a tough one, especially because of how annoying it will be to redo. I suppose we could explain the correction to apply per param with release notes. I'm not sure that anyone would actually do that, but somehow it still seems better than force resetting these valuable parameters.

dagar · 2018-03-15T17:40:20Z

This isn't great, but I have 2 ideas for making the transition acceptable.

python script that reads your ulog, and based on PX4 version and existing TC parameters will shift the curve and provide updated TC parameters. We can note this in the release.
We could add a new param directly in mpu6000 and mpu9250 to handle this change. Anyone with affected TC parameters could fall back to using the existing incorrect temperature calculation, but get a mavlink error message (and logged) every startup. Doing a fresh TC would set the new parameter, use the correct temperature, and give no warning. We could drop it after one full release cycle (v1.8.0 -> v1.9.0).

Typically I don't think we should be trying to handle every migration, but in this case performing TC yet again is time consuming and becomes an expensive mistake.

Thoughts?

priseborough · 2018-03-15T20:39:54Z

Is the effect on temperature an offset only or offset plus scale factor? If only an offset then only the *_TREF parameters for that sensor need to be adjusted by the amount of the temperature shift.

mhkabir · 2018-03-16T21:29:28Z

It's both scale factor and offset. How do we identify which IMU model each calibration is for though? Device ID?

…

On Mar 15, 2018 4:40 PM, "Paul Riseborough" ***@***.***> wrote: Is the effect on temperature an offset only or offset plus scale factor? If only an offset then only the *_TREF parameters for that sensor need to be adjusted by the amount of the temperature shift. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9054 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFTNrCeYj0hIRT_Rdvt-dQDnLz7lMB_4ks5tetGfgaJpZM4Slcsx> .

mhkabir · 2018-05-14T16:23:21Z

@dagar Any updates here?

dagar · 2018-05-14T22:03:13Z

At an absolute minimum we'd need to ensure incorrect temperature calibrations aren't used. Changing the device id might be an option.

mcsauder · 2018-05-15T01:06:27Z

@bkueng, @priseborough,
I am interested in gathering your feedback on @mhkabir's merge request as well, (as you are primary authors/maintainers in the thermal calibration routines). This PR, if merged, does necessitate a recalibration, however the bad data currently being propagated by the incorrect values is actively impacting current users as well. Any additional thoughts on how this could proceed?

Thanks!

-Mark

priseborough · 2018-05-15T01:57:59Z

My view in the short term is that we give it a new device ID and and provide users with a python script they can run that reads in the polynomial coefficients and recalculates them so that the correction is the same as suggested by @dagar

However something to consider is that if the user is calibrating the sensor, then it makes more sense to apply the calibration to the data before the manufacturers compensation is applied. This is because the compensation curve provided by the manufacturer has steps in the slope which makes curve fitting harder and is likely responsible for the higher order polynomial required.

I would like to see a comparison of the fit achieved with and without the manufacturers compensation.

proficnc · 2018-05-15T02:43:56Z

Device ID and Bus?
I agree with Paul, dump the manufacturer calibration and do it all in PX4

mhkabir · 2018-05-15T02:57:58Z

Do the IMUs even have a manufacturer compensation?

mcsauder · 2018-05-15T04:03:03Z

The manufacturer's don't provide compensation, they only provide the conversion for the ADC counts LSB to deg C... Unfortunately, the initial implementation of the driver was incorrect because the data sheet was incorrect, (@mhkabir wasn't the only one affected by data sheet revisions. :) ).

In this case, I know the bug fix has a negative impact, but it feels like the bug should still simply be squashed and that it somehow feels wrong to correct the driver bug by giving it the device a new ID.

Is there any way to quantify how many users this would impact and if there is a way to coordinate a fix with them?

dagar · 2018-05-15T13:24:14Z

Do the IMUs even have a manufacturer compensation?

Not that I've seen in our currently supported IMUs, but it wouldn't surprise me. I believe Paul's referring to the ms5611 (applies to the ms5525 as well). #9267

mcsauder · 2018-05-15T18:48:41Z

As far as I can see reading through the temp calibration code, each routine is querying the respective (correct) temperature sensor for the sensor being calibrated, so although the mpu9250 accel/gyro would require recal, for factory calibrated baro/dynamic pressure sensors this pull request would have no impact.

(Are there any imu's or magnetometers similar to the mpu9250/mpu6500 that have a factory cal?)

mcsauder · 2018-05-16T05:01:25Z

5 days enough to resolve this issue? Let's set a goal. :)

LorenzMeier · 2018-05-16T06:57:52Z

@mcsauder That's fair - we are better off setting expectations and timelines.

mcsauder · 2018-05-19T04:08:18Z

Hi Everyone,

I think I have a proposal. The 2.0.0 release appears to be coming soon, and the implication for Major version increments in the Major.Minor.Patch version methodology is the suggestion that Major version increments may/will cause backward compatibility breakage... (e.g. Python2 to Python3.)

Can we introduce this fix into the 2.0.0 release understanding that it causes breakage, (and perhaps other changes will cause breakage), and everyone can understand the implications?

Also, are there other backward compatibility issues that could be slated for 2.0.0?

Let me know your thoughts.

-Mark

mcsauder · 2019-01-12T18:28:33Z

@mhkabir, @dagar, do you think we should revisit this PR and get this bugfix slated into an upcoming release?

dakejahl · 2019-01-12T18:40:00Z

It is fixed https://github.com/PX4/Firmware/blob/master/src/drivers/imu/mpu9250/mpu9250.cpp#L1364

mcsauder · 2019-01-12T19:02:08Z

thanks @dakejahl !

mcsauder · 2019-01-14T03:11:53Z

@dakejahl, it looks like the mpu6000 fix from this PR hasn't been applied yet. (https://github.com/PX4/Firmware/blob/29a0950753854223d144de0b7335bbcbe40ea1d4/src/drivers/imu/mpu6000/mpu6000.cpp#L1804)

@mhkabir, would you be interested in rebasing/updating this PR? Let me know if I can help.

mhkabir · 2019-01-28T12:12:49Z

On master.

ksschwabe reviewed Mar 13, 2018

View reviewed changes

mhkabir force-pushed the pr-invensense-temp branch from b0a4b57 to 7f0becb Compare March 13, 2018 15:16

dagar requested review from dagar, priseborough and LorenzMeier March 15, 2018 17:40

dagar added the bug label Mar 15, 2018

dagar requested a review from thomasgubler March 15, 2018 17:40

dagar added this to the Release v1.8.0 milestone Mar 15, 2018

M-Skelton mentioned this pull request Apr 8, 2018

Discussion: Look into improved curve fits for ms5611 Baro temp calibration #9266

Closed

dagar removed this from the Release v1.8.0 milestone May 4, 2018

mhkabir mentioned this pull request May 14, 2018

mpu9250 improvements #9459

Closed

dagar added the devcall label May 14, 2018

dagar removed the devcall label May 23, 2018

dagar added this to the Release v2.0.0 milestone May 23, 2018

mhkabir added 2 commits June 5, 2018 09:42

mpu6000 : fix temperature output to match datasheet parameters

e5a00e9

mpu9250 : fix temperature output to match datasheet parameters

2b241de

mhkabir force-pushed the pr-invensense-temp branch from 7f0becb to 2b241de Compare June 5, 2018 04:12

weekly-digest bot mentioned this pull request Jan 19, 2019

Weekly Digest (12 January, 2019 - 19 January, 2019) #11244

Closed

mcsauder mentioned this pull request Jan 27, 2019

Correct the mpu6000 temperature scalar to match the datasheet #11314

Merged

mhkabir closed this Jan 28, 2019

LorenzMeier deleted the pr-invensense-temp branch January 18, 2021 14:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix temperature readout for Invensense IMUs #9054

Fix temperature readout for Invensense IMUs #9054

mhkabir commented Mar 10, 2018 •

edited by AuterionWrikeBot

Loading

mhkabir commented Mar 11, 2018

LorenzMeier commented Mar 11, 2018

ksschwabe left a comment

ksschwabe Mar 13, 2018

mhkabir Mar 13, 2018

ksschwabe Mar 13, 2018

mhkabir Mar 13, 2018

ksschwabe Mar 13, 2018

ksschwabe Mar 13, 2018

mhkabir Mar 13, 2018

mhkabir commented Mar 13, 2018

mhkabir commented Mar 15, 2018

dagar commented Mar 15, 2018 •

edited

Loading

dagar commented Mar 15, 2018

priseborough commented Mar 15, 2018

mhkabir commented Mar 16, 2018 via email

mhkabir commented May 14, 2018

dagar commented May 14, 2018

mcsauder commented May 15, 2018

priseborough commented May 15, 2018

proficnc commented May 15, 2018

mhkabir commented May 15, 2018

mcsauder commented May 15, 2018 •

edited

Loading

dagar commented May 15, 2018

mcsauder commented May 15, 2018

mcsauder commented May 16, 2018 •

edited

Loading

LorenzMeier commented May 16, 2018

mcsauder commented May 19, 2018 •

edited

Loading

mcsauder commented Jan 12, 2019

dakejahl commented Jan 12, 2019

mcsauder commented Jan 12, 2019

mcsauder commented Jan 14, 2019 •

edited

Loading

mhkabir commented Jan 28, 2019

Fix temperature readout for Invensense IMUs #9054

Fix temperature readout for Invensense IMUs #9054

Conversation

mhkabir commented Mar 10, 2018 • edited by AuterionWrikeBot Loading

mhkabir commented Mar 11, 2018

LorenzMeier commented Mar 11, 2018

ksschwabe left a comment

Choose a reason for hiding this comment

ksschwabe Mar 13, 2018

Choose a reason for hiding this comment

mhkabir Mar 13, 2018

Choose a reason for hiding this comment

ksschwabe Mar 13, 2018

Choose a reason for hiding this comment

mhkabir Mar 13, 2018

Choose a reason for hiding this comment

ksschwabe Mar 13, 2018

Choose a reason for hiding this comment

ksschwabe Mar 13, 2018

Choose a reason for hiding this comment

mhkabir Mar 13, 2018

Choose a reason for hiding this comment

mhkabir commented Mar 13, 2018

mhkabir commented Mar 15, 2018

dagar commented Mar 15, 2018 • edited Loading

dagar commented Mar 15, 2018

priseborough commented Mar 15, 2018

mhkabir commented Mar 16, 2018 via email

mhkabir commented May 14, 2018

dagar commented May 14, 2018

mcsauder commented May 15, 2018

priseborough commented May 15, 2018

proficnc commented May 15, 2018

mhkabir commented May 15, 2018

mcsauder commented May 15, 2018 • edited Loading

dagar commented May 15, 2018

mcsauder commented May 15, 2018

mcsauder commented May 16, 2018 • edited Loading

LorenzMeier commented May 16, 2018

mcsauder commented May 19, 2018 • edited Loading

mcsauder commented Jan 12, 2019

dakejahl commented Jan 12, 2019

mcsauder commented Jan 12, 2019

mcsauder commented Jan 14, 2019 • edited Loading

mhkabir commented Jan 28, 2019

mhkabir commented Mar 10, 2018 •

edited by AuterionWrikeBot

Loading

dagar commented Mar 15, 2018 •

edited

Loading

mcsauder commented May 15, 2018 •

edited

Loading

mcsauder commented May 16, 2018 •

edited

Loading

mcsauder commented May 19, 2018 •

edited

Loading

mcsauder commented Jan 14, 2019 •

edited

Loading