PX4 / PX4-Autopilot

PX4 Autopilot Software
https://px4.io
BSD 3-Clause "New" or "Revised" License
8.5k stars 13.51k forks source link

PR #10116 breaks arbitrary bus/device compatibility #11019

Closed dakejahl closed 5 years ago

dakejahl commented 5 years ago

Problem PR #10116 specifically the introduction of MPU_DEVICE_TYPE_MPU9250 into the bus options list breaks compatibility for mpu9250/mpu6500 on any supported bus. This parameter essentially enforces device+bus pairing because the discovery process just runs through the list until it finds a device that responds to it on that bus. The 9250 is listed first, and thus it will always "discover" as a 9250 device type. selection_004 If I put the 6500 first, it works.... however this requires a priori knowledge of the device to be used on a given bus, which goes against plug-and-play architecture.

Solution Use probe() to read the WHO_AM_I register to determine device type. Do this at the discovery stage (start_bus) and save it as a member variable. You can than use this in place of MPU_DEVICE_TYPE_MPU9250 to do all the device specific stuff you need to do.

I've pushed up a branch to fix, but I do not have an ICM20948 so I cannot test if this works with that device. This is yet another reason why we should not slam together hardware specific drivers into a single driver (unless we are very careful about how we do it).

@flochir @DanielePettenuzzo @dagar @bkueng

dagar commented 5 years ago

@DanielePettenuzzo if you have a newish Here GPS on hand could you give this a quick test? https://github.com/PX4/Firmware/pull/11020

dagar commented 5 years ago

Fixed in https://github.com/PX4/Firmware/pull/11020

flochir commented 5 years ago

@dakejahl @DanielePettenuzzo @dagar

Sorry, couldn't properly respond sooner. With #11020, a few issues for ICM20948 appeared.

Magnetometer only mode is broken (tries to use non-existing accelerometer device file) and ICM20948 is not detected reliably, especially after a reboot:

nsh> mpu9250 -X -R 6 start
INFO  [mpu9250] Bus probed: 2
MPU9250_I2C on I2C bus 1 at 0x69 (bus: 100 KHz, max: 400 KHz)
INFO  [mpu9250] accel cutoff set to 30.00 Hz
INFO  [mpu9250] gyro cutoff set to 80.00 Hz
AK8963_I2C on I2C bus 1 at 0x0c (bus: 100 KHz, max: 400 KHz)
nsh> reboot
...
nsh> mpu9250 -X -R 6 start
INFO  [mpu9250] Bus probed: 2
MPU9250_I2C on I2C bus 1 at 0x69 (bus: 100 KHz, max: 400 KHz)
ERROR [mpu9250] driver start failed

This is because the WHOAMI field on ICM20948 (address 0x00) is only available on register bank 0. Bank 0 being selected can maybe be expected after a power cycle, but not anymore once the device was active and e.g. the flight control got rebooted. That (and because the WHOAMI register addresses differ between MPU9250 and ICM20948) is why I changed the mechanism to probe for icm20948/mpu9250/mpu6500 specifically, choosing the device type to probe for from the beginning. This way, a register bank select would be run while probing for icm20948 and the right addresses could be used from the start. Seemed the cleanest, I didn't know about the implications though.

Also the ICM20948 on an externally mounted Here GPS is now (plug and play) started with "mpu9250 -R 14 start" in line 119 of px4fmu-v3 rc.board - in full IMU mode instead of magnetometer only and with the wrong orientation, breaking accelerometer calibration. Also the internal MPU9250 does not get loaded that way, as the call found the ICM.

So I guess the bus not being specified in line 119 of rc.board is a bug, else there's no way to start an optional, external sensor with special parameters in rc.sensors, when one with the same driver is loaded in some rc.board.

Proposal for a fix is in #11173. Tested on Pixhawk 2.1 cube (no flight yet though).

flochir commented 5 years ago

Probably other board configs loading mpu9250 have similar problems with misdetection of an external ICM/MPU. I only changed the one for px4fmu-v3 for now!