AP_Compass: avoid saving device IDs if not calibrated

[tridge]

this fixes an issue with the following sequence:

• new board (or board with FORMAT_VERSION reset) starts up with only internal compasses
• internal compasses are detected and devids saved
• an external compass is added and the board is rebooted
• the external compass will not be the first compass
• user then calibrates and flies, but has internal as primary

this can lead to a very bad experience for new users. At least one vehicle has crashed due to this sequence.

The fix is to not save device IDs during the Compass::init() if we have never been calibrated. This means that when an external compass is added it will come up as the first compass.

@peterbarker I'd like to discuss how we could add a CI test for this. I think we would need to use AP_Param/tools/eedump_apparam to look for saved IDs for key 147 (compass)

edit: updated to move DroneCAN probing for compasses earlier in the detection list

[IamPete1]

Could this be fixed with documentation? Or in mission planner before the calibration?

We could pre-arm if using a internal compass when there is a calibrated external.

There was extreme hesitance about messing with compass stuff in #23706 and this is a much more major re-work.

[tridge]

We could pre-arm if using a internal compass when there is a calibrated external.

it is perfectly valid to use the internal as primary and on some vehicles that is actually the right thing to do

Could this be fixed with documentation? Or in mission planner before the calibration?

the compass ordering is already documented. It didn't save this vehicle, and I have noticed that quite a few logs I review have the internal as first compass. Now we know why.

I think this was just a bug. If when you unwrap your shiny new flight controller and plug it in on USB to see the blinky lights (which I bet nearly everyone does) then it locks in your internal as the first compass. That is not desired behaviour.

[peterbarker]

@peterbarker I'd like to discuss how we could add a CI test for this. I think we would need to use AP_Param/tools/eedump_apparam to look for saved IDs for key 147 (compass)

I think we can actually simulate the user story outlined above reasonably well, actually.

We should be able to

• start up a vehicle without a defaults file, and wipe its parameters. The FlyEachFrame test does this, but also ensuring the binary is doing the multicast UDP SITL state thing
• reboot the simulated vehicle
• start the increasingly-misnamed sitl_periph_gps binary, ensure we see it somehow in the main firmware
• reboot the vehicle
• do a large-vehicle-mag-cal
• reboot the vehicle
• check the results are what we expect, at least in terms of compass IDs and whatnot.

The toughest bit in there is working out how to start the sitl_periph_gps thing...

[tridge]

@peterbarker it doesn't have to actually be a DroneCAN compass. The original issue was with I2C internal and external compasses.
So we just need to startup without an external I2C compass, then startup with the compass and do a cal, and check ordering.
I also think we will need a need a new frame target which sets SIM_MAG_SAVE_IDS=0 by default, maybe quad--no-mag-save ?

[andyp1per]

Seems like this is two different changes that should be in two different PR's

[tridge]

Seems like this is two different changes that should be in two different PR's

the GPS changes are actually already in master, I've rebased and now it doesn't show them in this PR. Sorry for the confusion

[peterbarker]

Suggested change

	for (uint8_t i=0; i<COMPASS_MAX_INSTANCES; i++) {
	for (uint8_t i=0; i<ARRAY_SIZE(_state._priv_instance); i++) {

[davidbuzz]

Kinda feels like this should also be backported to as many releases as possible?