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

temperature_compensation module #12601

Merged
merged 1 commit into from
Jan 21, 2020
Merged

Conversation

dagar
Copy link
Member

@dagar dagar commented Aug 1, 2019

This is a new module for temperature compensation that's currently part of the sensors and events modules.

Moving this to a separate module means it can be trivially disabled when not used (saving ~ 16 kB of flash), and is architecturally consistent with removing the sensors hub as a bottle neck for running multiple estimators.

@dagar
Copy link
Member Author

dagar commented Aug 1, 2019

Flash savings if disabled from a build (eg things get desperate in px4_fmu-v2).

Screenshot from 2019-08-01 16-37-13

@dagar dagar force-pushed the pr-temperature_compensation_clean_up branch from 9b7a485 to 6bb7d2e Compare August 1, 2019 20:48
@@ -730,7 +659,6 @@ void VotedSensorsUpdate::gyro_poll(struct sensor_combined_s &raw)
if (_gyro.last_best_vote != best_index) {
_gyro.last_best_vote = (uint8_t)best_index;
_corrections.selected_gyro_instance = (uint8_t)best_index;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

The selected_gyro_instance needs to be published.

Copy link
Contributor

@dakejahl dakejahl Aug 13, 2019

Choose a reason for hiding this comment

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

Hmm... I think we need to split these topics
I'd like to rename sensor_correction to sensor_thermal_corrections and move

	uint8_t selected_gyro_instance;
	uint8_t selected_accel_instance;
	uint8_t selected_baro_instance;

into their own topic (active_sensors? select_sensors?). Does this make sense?

The *_mapping is just for applying the thermal corrections to the correct senor...right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe sensor_correction could move entirely to temperature_compensation and the selected instance (and mapping) could go away entirely.

Then in the sensors module sensor_selection could also contain the ORB instance number.

@julianoes
Copy link
Contributor

@dagar please request reviews when it's ready.

@dagar
Copy link
Member Author

dagar commented Sep 30, 2019

This is working now (at least on the bench). @dakejahl could you take another pass?

nsh> temperature_compensation status
INFO  [temperature_compensation] Temperature Compensation:
INFO  [temperature_compensation]  gyro: enabled: 1
INFO  [temperature_compensation]   using device ID 3672842 for topic instance 0
INFO  [temperature_compensation]   using device ID 2360330 for topic instance 1
INFO  [temperature_compensation]  accel: enabled: 1
INFO  [temperature_compensation]   using device ID 3607306 for topic instance 0
INFO  [temperature_compensation]   using device ID 1442826 for topic instance 1
INFO  [temperature_compensation]  baro: enabled: 1
INFO  [temperature_compensation]   using device ID 3998482 for topic instance 0
nsh> sensors status
INFO  [sensors] gyro status:
INFO  [ecl/validation] validator: best: 0, prev best: 0, failsafe: NO (0 events)
INFO  [ecl/validation] sensor #0, prio: 100, state: OK
INFO  [ecl/validation]  val:  -0.0046, lp:  -0.0010 mean dev:  -0.0000 RMS:   0.0028 conf:   1.0000
INFO  [ecl/validation]  val:   0.0035, lp:   0.0012 mean dev:  -0.0001 RMS:   0.0015 conf:   1.0000
INFO  [ecl/validation]  val:   0.0020, lp:   0.0022 mean dev:   0.0001 RMS:   0.0010 conf:   1.0000
INFO  [ecl/validation] sensor #1, prio: 100, state: OK
INFO  [ecl/validation]  val:   0.0020, lp:   0.0004 mean dev:  -0.0001 RMS:   0.0031 conf:   1.0000
INFO  [ecl/validation]  val:  -0.0020, lp:  -0.0033 mean dev:   0.0000 RMS:   0.0014 conf:   1.0000
INFO  [ecl/validation]  val:   0.0044, lp:   0.0041 mean dev:  -0.0000 RMS:   0.0011 conf:   1.0000
INFO  [sensors] accel status:
INFO  [ecl/validation] validator: best: 0, prev best: 0, failsafe: NO (0 events)
INFO  [ecl/validation] sensor #0, prio: 100, state: OK
INFO  [ecl/validation]  val:   0.0293, lp:   0.0279 mean dev:   0.0001 RMS:   0.0109 conf:   1.0000
INFO  [ecl/validation]  val:  -0.2623, lp:  -0.3164 mean dev:   0.0009 RMS:   0.0500 conf:   1.0000
INFO  [ecl/validation]  val:  -9.8288, lp:  -9.8239 mean dev:   0.0001 RMS:   0.0145 conf:   1.0000
INFO  [ecl/validation] sensor #1, prio: 100, state: OK
INFO  [ecl/validation]  val:   0.0361, lp:   0.0468 mean dev:  -0.0003 RMS:   0.0110 conf:   1.0000
INFO  [ecl/validation]  val:  -0.1933, lp:  -0.2559 mean dev:  -0.0002 RMS:   0.0477 conf:   1.0000
INFO  [ecl/validation]  val:  -9.7500, lp:  -9.7518 mean dev:   0.0003 RMS:   0.0172 conf:   1.0000
INFO  [sensors] mag status:
INFO  [ecl/validation] validator: best: 0, prev best: 0, failsafe: NO (0 events)
INFO  [ecl/validation] sensor #0, prio: 255, state: OK
INFO  [ecl/validation]  val:  -0.3583, lp:  -0.3579 mean dev:  -0.0000 RMS:   0.0032 conf:   1.0000
INFO  [ecl/validation]  val:  -0.4282, lp:  -0.4330 mean dev:  -0.0000 RMS:   0.0027 conf:   1.0000
INFO  [ecl/validation]  val:   0.4922, lp:   0.4932 mean dev:   0.0000 RMS:   0.0042 conf:   1.0000
INFO  [sensors] baro status:
INFO  [ecl/validation] validator: best: 0, prev best: 0, failsafe: NO (0 events)
INFO  [ecl/validation] sensor #0, prio: 75, state: OK
INFO  [ecl/validation]  val: 1014.4800, lp: 1014.5020 mean dev:   0.0070 RMS:   0.0593 conf:   1.0000
INFO  [ecl/validation]  val:  31.5700, lp:  31.5495 mean dev:   0.0225 RMS:   0.0265 conf:   1.0000
INFO  [ecl/validation]  val:   0.0000, lp:   0.0000 mean dev:   0.0000 RMS:   0.0000 conf:   1.0000
INFO  [sensors] Airspeed status:
INFO  [ecl/validation]  no data

INFO  [vehicle_acceleration] selected sensor: 3607306 (sensor_accel:0)
INFO  [vehicle_acceleration] offsets: -0.03656 -0.18259 0.10048
INFO  [vehicle_acceleration] scale: 0.99911 0.91098 1.00166
INFO  [vehicle_acceleration] bias: -0.00459 0.00511 -0.04384
vehicle_acceleration: cycle time: 30642 events, 519315us elapsed, 16.95us avg, min 14us max 396us 5.832us rms
vehicle_acceleration: interval: 30647 events, 4005.48us avg, min 1301us max 6783us 78.510us rms
vehicle_acceleration: sensor latency: 30651 events, 3809259us elapsed, 124.28us avg, min 115us max 2887us 26.389us rms

INFO  [vehicle_angular_velocity] selected sensor: 3672842 (sensor_gyro_control:1)
INFO  [vehicle_angular_velocity] offsets: 0.00266 0.02651 -0.02043
INFO  [vehicle_angular_velocity] scale: 1.00000 1.00000 1.00000
INFO  [vehicle_angular_velocity] bias: -0.00109 0.00118 0.00240
vehicle_angular_velocity: cycle time: 121605 events, 2317850us elapsed, 19.06us avg, min 17us max 49us 3.796us rms
vehicle_angular_velocity: interval: 121624 events, 1010.20us avg, min 677us max 1694us 352.201us rms
vehicle_angular_velocity: sensor latency: 121642 events, 2899439us elapsed, 23.84us avg, min 23us max 51us 2.222us rms
nsh> listener sensor_correction

TOPIC: sensor_correction
 sensor_correction_s
        timestamp: 139657561  (18.717096 seconds ago)
        gyro_offset_0: [0.0027, 0.0265, -0.0204]
        gyro_scale_0: [1.0000, 1.0000, 1.0000]
        gyro_offset_1: [0.0256, -0.0035, 0.0039]
        gyro_scale_1: [1.0000, 1.0000, 1.0000]
        gyro_offset_2: [0.0000, 0.0000, 0.0000]
        gyro_scale_2: [1.0000, 1.0000, 1.0000]
        accel_offset_0: [-0.0370, -0.1821, 0.1019]
        accel_scale_0: [0.9991, 0.9109, 1.0017]
        accel_offset_1: [0.0832, 0.1596, -0.0156]
        accel_scale_1: [0.9985, 1.0005, 0.9899]
        accel_offset_2: [0.0000, 0.0000, 0.0000]
        accel_scale_2: [1.0000, 1.0000, 1.0000]
        baro_offset_0: -2.2326
        baro_scale_0: 1.0000
        baro_offset_1: 0.0000
        baro_scale_1: 1.0000
        baro_offset_2: 0.0000
        baro_scale_2: 1.0000
        gyro_device_id: [3672842, 2360330, 0]
        accel_device_id: [3607306, 1442826, 0]
        baro_device_id: [3998482, 0, 0]
        gyro_mapping: [1, 0, 0]
        accel_mapping: [1, 0, 0]
        baro_mapping: [0, 0, 0]

@dagar
Copy link
Member Author

dagar commented Sep 30, 2019

I'd still like to split sensor_correction into several different simpler messages, but I'll save that for another pass.

@dagar dagar force-pushed the pr-temperature_compensation_clean_up branch 3 times, most recently from 3afb456 to 053ab20 Compare September 30, 2019 13:31
Copy link
Contributor

@dakejahl dakejahl left a comment

Choose a reason for hiding this comment

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

Looks good!

@dagar
Copy link
Member Author

dagar commented Sep 30, 2019

TODO:

  • check that onboard calibration still works (what was handled in send_event)
  • skip starting temperature_compensation module entirely if not enabled (check parameters)

@TSC21 TSC21 requested a review from bkueng October 15, 2019 14:37
@dagar dagar marked this pull request as ready for review October 16, 2019 16:46
@dagar dagar force-pushed the pr-temperature_compensation_clean_up branch from 053ab20 to ba3eed5 Compare October 16, 2019 16:57
Copy link
Contributor

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

I haven't fully grasped this yet. Need to look at it again with fresh eyes.

}

// TODO: do something with vehicle commands
// TODO: what is this modules purpose?
Copy link
Contributor

Choose a reason for hiding this comment

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

🙈


for (int correction_index = 0; correction_index < MAX_SENSOR_COUNT; correction_index++) {
if (corrections.accel_device_id[correction_index] == _selected_sensor_device_id) {
_sensor_correction_index = correction_index;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand this. Is _sensor_correction_index valid for the remainder? And if so, we only ever correct one of the sensors?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is _sensor_correction_index valid for the remainder

No, unless there's a bug I am not seeing. SensorSelectionUpdate() is called everytime thru the loop in Run() and it will perform the check for data.device_id == selection.accel_device_id and if this fails it calls SensorCorrectionsUpdate which will update _sensor_correction_index. It's kinda tangled and messy and I don't particularly like it, but as far as I am aware this "works" (if it aint broke don't fix it?).

_offset = Vector3f{corrections.gyro_offset_2};
_scale = Vector3f{corrections.gyro_scale_2};

} else {
_offset = Vector3f{0.0f, 0.0f, 0.0f};
_scale = Vector3f{1.0f, 1.0f, 1.0f};
}
}

return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why false?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be made to return void .. which means Start() could also return void seeing as how it's return value is unused.

@BazookaJoe1900
Copy link
Member

note that #13165 is related. and if what said on #13165 is true, it can be fixed with this pr

Copy link
Contributor

@dakejahl dakejahl left a comment

Choose a reason for hiding this comment

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

@dagar Have you tested this? (the onboard calibration routine).


for (int correction_index = 0; correction_index < MAX_SENSOR_COUNT; correction_index++) {
if (corrections.accel_device_id[correction_index] == _selected_sensor_device_id) {
_sensor_correction_index = correction_index;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is _sensor_correction_index valid for the remainder

No, unless there's a bug I am not seeing. SensorSelectionUpdate() is called everytime thru the loop in Run() and it will perform the check for data.device_id == selection.accel_device_id and if this fails it calls SensorCorrectionsUpdate which will update _sensor_correction_index. It's kinda tangled and messy and I don't particularly like it, but as far as I am aware this "works" (if it aint broke don't fix it?).

_offset = Vector3f{corrections.gyro_offset_2};
_scale = Vector3f{corrections.gyro_scale_2};

} else {
_offset = Vector3f{0.0f, 0.0f, 0.0f};
_scale = Vector3f{1.0f, 1.0f, 1.0f};
}
}

return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be made to return void .. which means Start() could also return void seeing as how it's return value is unused.

@dagar dagar force-pushed the pr-temperature_compensation_clean_up branch from 5c21cd5 to 74cd307 Compare November 24, 2019 21:05
@dagar
Copy link
Member Author

dagar commented Nov 24, 2019

Let's finish this.

@dagar dagar force-pushed the pr-temperature_compensation_clean_up branch from 74cd307 to 3eb5de8 Compare November 24, 2019 22:05
@dagar
Copy link
Member Author

dagar commented Nov 24, 2019

TODO:

  • only run if enabled (TC_{A, B, G}_ENABLE or online calibrate
  • review vehicle_angular_velocity and vehicle_acceleration logic (recent lockstep bug)
  • test online calibrate

@dagar dagar force-pushed the pr-temperature_compensation_clean_up branch from 3eb5de8 to b8bfd24 Compare November 25, 2019 15:10
@dakejahl
Copy link
Contributor

dakejahl commented Dec 12, 2019

Tested successfully on the bench. Veriified online calibration (freezer + heat gun) as well as the continual updating of corrections as the sensors undergo temperature changes (heat gun).

We should remove TC_*_ENABLE parameters and use a single TC_ENABLE parameter instead. Right now the TemperatureCompensationModule always publishes corrections for all 3 sensors, however if a sensor does not have an associated calibration the offsets are 0 and the scales are 1, thus having no effect. Also, right now the subscribers of sensor_correction do not check the TC_*_ENABLE flags. Alternatively we could have the subscribers apply the updates on sensor_correction_updated as well as TC_*_ENABLE.

Thoughts?

@dakejahl
Copy link
Contributor

Selection_050
The "published correction" was my debug printout to verify

@dagar dagar force-pushed the pr-temperature_compensation_clean_up branch from bf7c223 to d0d1c0c Compare January 21, 2020 01:47
@dagar dagar changed the title [WIP]: temperature_compensation module temperature_compensation module Jan 21, 2020
@dagar
Copy link
Member Author

dagar commented Jan 21, 2020

Rebased on master and testing.

@dagar
Copy link
Member Author

dagar commented Jan 21, 2020

For systems not using temperature compensation this actually saves about 2 kB of memory (tested on fmu-v4).

nsh> free
             total       used       free    largest
Umem:       229568     178928      50640      47392  <- master
Umem:       229568     176736      52832      46288  <- PR

This also saves a small amount of cpu in the sensors module (maybe 0.2%).

@dagar
Copy link
Member Author

dagar commented Jan 21, 2020

I've verified sensor_correction on a few calibrated systems before and after, and this looks safe to merge.

Big thank you to @dakejahl for doing the heavy lifting here. This now makes the entire temperature compensation functionality modular and optional, so we can disable it where needed (eg fmu-v2) and save a fairly big chunk of flash and a little bit of memory. Having this out of the way will also help in refactoring the sensors module, which is one of the remaining pieces in the way of running multiple estimators.

@dagar dagar merged commit dc05cea into master Jan 21, 2020
@dagar dagar deleted the pr-temperature_compensation_clean_up branch January 21, 2020 02:42
@julianoes
Copy link
Contributor

Nice @dagar!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants