search

ArduPilot / ardupilot

ArduPlane, ArduCopter, ArduRover, ArduSub source
http://ardupilot.org/
GNU General Public License v3.0
10.81k stars 17.27k forks source link

Compass: changing COMPASS_USEx parameter in flight should cause compass to not be used #10785

Open rmackay9 opened 5 years ago

rmackay9 commented 5 years ago

As part of this PR's review we discovered that if a user set the COMPASS_USE parameter to 0 in flight then instead of the EKF switching to another compass, it will actually get stuck using the compass. We should fix this so that if COMPASS_USEx is set to zero, the EKF switches to the next compass in line (or doesn't use a compass if there are not alternatives)

peterbarker commented 5 years ago

On Mon, 11 Mar 2019, Randy Mackay wrote:

As part of this PR's review we discovered that if a user set the COMPASS_USE parameter to 0 in flight then instead of the EKF switching to another compass, it will actually get stuck using the compass. We should fix this so that if COMPASS_USEx is set to zero, the EKF switches to the next compass in line (or doesn't use a compass if there are not alternatives)

I was musing on how MAG_ENABLE works here, too. If you turn this off in flight we'll stop calling compass.read() but we don't exactly undo what "ahrs.set_compass(...)" does!

Any objections if I move MAG_ENABLE into being COMPASS_ENA? That gives the EKF a fighting chance at stopping using the compass. All vehicles have a MAG_ENABLE ATM.

rmackay9 commented 5 years ago

@peterbarker, that sounds OK although @OXINARF and I have discussed this before and we thought that perhaps we should just completely remove the MAG_ENABLE parameter. Essentially setting all compass_usex parameters to 0 is the same as disabling them all... maybe? If not then I fully support moving mag-enable into the compass library so that it's shared by all vehicles.

proficnc commented 5 years ago

Removing it makes sense. Duplicate parameters for the same task can cause unnecessary confusion

peterbarker commented 5 years ago

On Thu, 14 Mar 2019, Philip wrote:

Removing it makes sense. Duplicate parameters for the same task can cause unnecessary confusion

I'm not sure. By the time you've got parameters for 9 compasses cough, it might be annoying to have to disable them all.

Further, when we move to finding compasses at runtime (e.g. plugging in a CANBUS combo device) we have to make a decision on whether to start using it or not - and you won't necessarily have parameters saying, "no" when you plug in it.

I think there's still room for a "really, just don't" parameter in this case.

rmackay9 commented 5 years ago

@peterbarker, that works for me.