Migration to busDevice infrastructure: acc/gyro/mag/osd #2290

digitalentity · 2017-10-08T12:46:03Z

Refactoring of busDevice code, migration of a acc/gyro sensors to busDevice

digitalentity · 2017-10-08T13:30:44Z

This is going to be a big PR 🤔

martinbudden · 2017-10-09T13:18:54Z

src/main/drivers/accgyro/accgyro_bus_mpu6000.c

+
+    spiSetSpeed(MPU6000_SPI_INSTANCE, SPI_CLOCK_INITIALIZATON);
+
+    if (!mpu6000InitDone) {


I really don't like these initialisation guards. It implies we have lost control of the initialisation sequence. mpu6000AccAndGyroInit should be called once and once only - that renders the guard unnecessary.

Also, as an aside, none of the gyro drivers should have any static data - all required data should be in the gyroDev_t structure.

Agreed, I'm going to remove it eventually. This is merely a proof of concept code.

martinbudden · 2017-10-09T13:20:06Z

src/main/drivers/accgyro/accgyro.h

    sensorGyroUpdateFuncPtr updateFn;
    extiCallbackRec_t exti;
-    busDevice_t bus;
+    busDevice_t * dev;


Why is this a pointer? And why have you renamed it to dev - that is just confusing.

Because busDevice_t object may be shared between gyro and acc (and compass).

Yes, it can be shared. But it is small and not often shared. I think it is better to duplicate.

@martinbudden the reason to share the object is that we do device detection (and most of the configuration) only in gyro driver. Acc driver needs to know if gyro device was detected and initialized - shared scratchpad field of the busDevice_t is used for that.

The way this is currently handled is:

acc init occurs after gyro init

acc init copies gyro busDevice_t data.

It's straightforward, has and no potential null pointer errors. Note also this works for dual gyro systems in Betaflight.

Sometimes a pragmatic approach is better than a "pure" approach.

I never likes the part of initialization that was copying around data between gyro_t and acc_t. High-level code needs to know nothing about how devices operate. All hardware sharing issues should be solved at low level.

High-level code needs to do two things: 1) call accWhateverDetect() and 2) call acc->init() on detected device.

If we were sticking to pragmatic approach - we would still be flying Baseflight code 😄

martinbudden · 2017-10-09T13:22:41Z

src/main/drivers/accgyro/accgyro_bus_mpu6000.c

+
+    if (!mpu6000InitDone) {
+        // Device Reset
+        busWrite(gyro->dev, MPU_RA_PWR_MGMT_1, BIT_H_RESET);


Should be busWrite(&gyro->bus,...) see earlier comment. busWrite writes to a bus, not to a dev.

Then I'll need to refactor this. It's actually writing to a specific dev on the bus.

martinbudden · 2017-10-09T13:27:18Z

src/main/drivers/bus.c


+#define BUSDEV_MAX_DEVICES 8
+
+static busDevice_t busDevPool[BUSDEV_MAX_DEVICES];


So you are adding dynamic allocation of busDevices. Why? It complicates things and is a potential source of error.

I need a busDevice for each active sensor. Initially I did allocate a static busDevice_t object for each supported sensor, but that was a waste of memory.

Yes, it does waste a bit of memory. But busDevice_t is fairly small, and it's not like we have hundreds of sensors. I'm prepared to pay a bit of RAM for increased reliability.

Also, you statically allocate for 8 sensors anyway.

Agreed. I'll look into reverting back to static allocation.

martinbudden · 2017-10-09T13:28:38Z

src/main/drivers/accgyro/accgyro_mpu.c

+void mpuIntExtiInit(gyroDev_t *gyro)
 {
-    if (!gyro->mpuIntExtiConfig) {
+    if (!gyro->dev->irqPin) {


It's not clear to me that the irqPin should be part of the busDevice_t structure. I think it should probably be part of the gyroDev_t structure.

martinbudden · 2017-10-09T13:31:29Z

src/main/drivers/bus.h

+            uint8_t address;        // I2C bus device address
        } i2c;
    } busdev;
+    IO_t irqPin;                    // Device IRQ pin. Bus system will only assign IO_t object to this var. Initialization is up to device driver


I think the device driver should probably own the irqPin.

It's an abstraction of a generic device on a bus. Most devices have an IRQ pin. We might not use it, but we have it. Allocating and handling it in one place is more logical than to allow each of the drivers handle it on it's own.

I'm even thinking about handling incoming interrupts in the busDevice_t layer in driver-independent maner.

But it's not handled in one place. Your comment specifically says Bus system will only assign IO_t object to this var. Initialization is up to device driver

As a general rule of thumb, the thing that does the initialisation should have ownership (I agree there are sometimes exceptions).

@martinbudden that's temporary. The code is far from completion. Eventually IRQ handling code will be migrated to busDevice_t and only callbacks & flags will be exposed to device driver.

martinbudden · 2017-10-09T13:34:28Z

See some specific comments in the code. However my biggest issue with this is the dynamic allocation of buses. Dynamic allocation adds the potential for errors. What are the advantages that you seek?

martinbudden · 2017-10-09T14:15:51Z

All hardware sharing issues should be solved at low level

I'm not sure about that. The gyro device should not need to know that it shares a bus with the acc device or vice versa. It's actually the higher level code that knows the two devices are packaged into one chip.

Everything should be as simple as possible. An accelerometer device driver that doesn't know anything about a gyro is simpler than one that does know. Of course somebody needs to know, but I argue that knowledge is correctly placed in the sensor code, not the device driver code.

The pragmatic bit I am talking about is the copying of data rather than using pointers. However, whether the bus is copied or whether a pointer is used, the knowledge still belongs in the sensor layer.

digitalentity · 2017-10-09T14:18:57Z

The gyro_t device has no idea it's shared. All sharing is handled at chip driver level. And so that a chip driver is aware that it was already initialized (driver itself is stateless) - a shared memory in busDevice_t is required so chip driver can "keep notes there".

digitalentity · 2017-11-10T10:47:56Z

I'm thinking about separating drivers for native AK8963 over I2C and AK8963 built-in into MPU9250.

What we have currently:
For MPU6500 + AK8963 as a separate chip: MPU6500 driver handles gyro/accel, AK8963 drvier handles mag
For MPU9250: MPU6500 driver handles gyro/accel, AK8963 driver uses MPU6500 API to read the compass via internal MPU's I2C bus. So, AK8963 essentially implements part of MPU6500 driver.

What I intend to do:
For MPU6500 + AK8963 as a separate chip: MPU6500 driver handles gyro/accel, AK8963 drvier handles mag
For MPU9250: A new MPU9250 will handle acc/gyro/mag. Internal MPU9250's compass will be detected as MPU9250 (not as AK8963 as before).

Thoughs?

digitalentity · 2017-11-15T11:27:41Z

Testing of this PR is needed on as much targets as possible. Shouldn't affect flight performance, only sensor detection and reading.

Some targets (ones having MPU9250) will likely require changing the accelerometer type.

stronnag · 2017-11-16T19:47:39Z

I flashed this on my F3 OMNIBUS. The baro (BMP280) is no longer detected. Going back to development or de_agl_estimation and it's OK again:

de_busdev_migration

# status
System Uptime: 17 seconds
Current Time: 0000-01-01T00:00:00.000+01:00
Voltage: 36 * 0.1V (1S battery - OK)
CPU Clock=72MHz, GYRO=MPU6000, ACC=MPU6000, MAG=HMC5883
STM32 system clocks:
  SYSCLK = 72 MHz
  HCLK   = 72 MHz
  PCLK1  = 36 MHz
  PCLK2  = 72 MHz
Sensor status: GYRO=OK, ACC=OK, MAG=OK, BARO=UNAVAILABLE, RANGEFINDER=NONE, OPFLOW=NONE, GPS=OK
SD card: Manufacturer 0x1c, 7878656kB, 10/2008, v1.0, 'USD  '
Filesystem: Ready
Stack size: 6144, Stack address: 0x10002000
I2C Errors: 1, config size: 3428, max available config: 4096
ADC channel usage:
   BATTERY : configured = ADC 1, used = ADC 1
      RSSI : configured = ADC 3, used = none
   CURRENT : configured = ADC 2, used = ADC 2
  AIRSPEED : configured = none, used = none
System load: 4, cycle time: 1012, PID rate: 988, RX rate: 49, System rate: 9

# bootlog
Time Evt            Description  Parameters
  48:  0          CONFIG_LOADED 
  48:  1       SYSTEM_INIT_DONE 
 550: 19   TIMER_CHANNEL_MAPPED  (1, 1, 0, 2)
 550: 19   TIMER_CHANNEL_MAPPED  (2, 2, 0, 2)
 550: 19   TIMER_CHANNEL_MAPPED  (3, 3, 0, 2)
 550: 19   TIMER_CHANNEL_MAPPED  (4, 4, 0, 2)
 550: 18  TIMER_CHANNEL_SKIPPED  (5, 4, 0, 3)
 550: 18  TIMER_CHANNEL_SKIPPED  (6, 4, 0, 3)
 550:  2          PWM_INIT_DONE 
 561:  3       EXTRA_BOOT_DELAY 
1214:  9         GYRO_DETECTION  (6, 0, 0, 0)
1517: 10          ACC_DETECTION  (7, 0, 0, 0)
1567: 11         BARO_DETECTION  (0, 0, 0, 0)
1567: 20        PITOT_DETECTION  (0, 0, 0, 0)
1597: 12          MAG_DETECTION  (2, 0, 0, 0)
3264: 13  RANGEFINDER_DETECTION  (0, 0, 0, 0)
3264:  4       SENSOR_INIT_DONE 
3968:  5          GPS_INIT_DONE 
3968:  7    TELEMETRY_INIT_DONE 
4974:  8           SYSTEM_READY 

# get baro
baro_hardware = BMP280
Allowed values: NONE, AUTO, BMP085, MS5611, BMP280, MS5607, FAKE

de_agl_estimation

# status
System Uptime: 7 seconds
Current Time: 0000-01-01T00:00:00.000+01:00
Voltage: 36 * 0.1V (1S battery - OK)
CPU Clock=72MHz, GYRO=MPU6000, ACC=MPU6000, BARO=BMP280, MAG=HMC5883
STM32 system clocks:
  SYSCLK = 72 MHz
  HCLK   = 72 MHz
  PCLK1  = 36 MHz
  PCLK2  = 72 MHz
Sensor status: GYRO=OK, ACC=OK, MAG=OK, BARO=OK, RANGEFINDER=NONE, OPFLOW=NONE, GPS=OK
SD card: Manufacturer 0x1c, 7878656kB, 10/2008, v1.0, 'USD  '
Filesystem: Ready
Stack size: 6144, Stack address: 0x10002000
I2C Errors: 1, config size: 3428, max available config: 4096
ADC channel usage:
   BATTERY : configured = ADC 1, used = ADC 1
      RSSI : configured = ADC 3, used = none
   CURRENT : configured = ADC 2, used = ADC 2
  AIRSPEED : configured = none, used = none
System load: 5, cycle time: 1010, PID rate: 990, RX rate: 49, System rate: 9

I said I would test de_agl_estimation tomorrow anyway, so that settles it.

digitalentity · 2017-11-17T04:01:24Z

This is odd. BMP280 is on SPI bus which didn't change. I'll verify on my OMNIBUS F3 board.

digitalentity · 2017-11-17T05:04:19Z

@stronnag bug confirmed. Working on a fix

digitalentity · 2017-11-17T06:40:47Z

Bug fixed. Caused by incorrect hw definition for BMP085/BMP180 which was overriding BMP280

digitalentity · 2017-11-17T10:45:20Z

Should be functional on OMNIBUS and other targets now

stronnag · 2017-11-18T13:05:43Z

Just flashed an SPRACINGF3EVO, after set acc_hardware = auto the acc is recognised as expected:

# status
System Uptime: 8 seconds
Current Time: 2017-11-18T14:01:40.324+01:00
Voltage: 10 * 0.1V (0S battery - NOT PRESENT)
CPU Clock=72MHz, GYRO=MPU9250, ACC=MPU9250, BARO=BMP280, MAG=HMC5883

stronnag · 2017-11-18T16:23:23Z

flight tested on Omnibus F3 and SPRacingF3Evo, locally merged with development. Nav modes (WP, RTH, PH, turn off TX failsafe RTH, free flight).
Also flown on SPRacingF3 (no nav modes).

stronnag · 2017-11-19T15:12:02Z

Tested on Matek F405, locally merged with development. Nav modes (WP, RTH, PH, free flight).

So back to the SPRF3, which was marked as 'flown' rather than 'tested', for a reason. Yesterday (genuine SPRF3 board) I was not content, often it would not arm without multiple power cycles and when it did arm, horizon mode appeared to lose its idea of level. Today I tried a Dodo board (same firmware), when it wouldn't arm I plugged in mwp to see 'Not level' reported (when it obviously was) -- and the AH indicating inverted. Flashed back to development, which was perfectly OK. So there appears to be an ACC initialisation / stability issue on SPRF3.

Final assessment:

Omnibus, SPRF3EVO, Matek F405: Content, no regression
SPRF3 : Not content, issue with ACC.

digitalentity · 2017-11-20T10:53:40Z

Ok. Thanks for testing @stronnag. I'll debug once I get home. I believe we have an issue with MPU6050 driver (most likely the 6050 revision detection code).

digitalentity · 2017-11-21T10:19:30Z

@stronnag strangely I can't reproduce on either of my two SPRacingF3 boards. Can you do a flash with full erase and check the values of Accelerometer in Sensors tab?

stronnag · 2017-11-21T18:39:57Z

The two SPRF3 seem OK after a reflash. Apologies for the diversion.

digitalentity · 2017-11-21T23:04:40Z

No worries @stronnag. There must be a place in gyroConfig which changed and should be reset. I'll find and fix it to make sure config is consistent.

digitalentity · 2017-11-21T23:10:32Z

Ok, I believe this can be merged now. This PR is already very big - I'll improve the code in next PRs.

stronnag · 2018-02-24T17:48:04Z

It would help immeasurably if you could tell us which FC, which version of iNav and provide a CLI diff. And if it's not 1.9-RC1, please try that.

digitalentity added the Don't merge label Oct 8, 2017

digitalentity added this to the 1.9 milestone Oct 8, 2017

digitalentity self-assigned this Oct 8, 2017

martinbudden reviewed Oct 9, 2017

View reviewed changes

digitalentity force-pushed the de_busdev_migration branch from 9cbe712 to a4d9d5f Compare October 24, 2017 13:39

digitalentity force-pushed the de_busdev_migration branch from a4d9d5f to 23e8dee Compare November 1, 2017 12:55

digitalentity force-pushed the de_busdev_migration branch 2 times, most recently from 2d82dfb to 1477eeb Compare November 14, 2017 14:59

digitalentity added Testing Required and removed Don't merge labels Nov 14, 2017

digitalentity changed the title ~~Migration to busDevice infrastructure~~ Migration to busDevice infrastructure: acc/gyro/mag/osd Nov 14, 2017

digitalentity mentioned this pull request Nov 14, 2017

Migration to busDevice infrastructure #2478

Closed

15 tasks

digitalentity mentioned this pull request Nov 17, 2017

Support for FRSKYF4 board #2486

Merged

digitalentity added Ready to merge and removed Testing Required labels Nov 21, 2017

digitalentity force-pushed the de_busdev_migration branch from d55af2a to 0a28733 Compare November 22, 2017 15:13

busDevice migration (acc/gyro/baro/compass/rangefinder)

8d9f68c

digitalentity force-pushed the de_busdev_migration branch from 0a28733 to 8d9f68c Compare November 22, 2017 15:26

digitalentity merged commit fff72a0 into development Nov 22, 2017

digitalentity deleted the de_busdev_migration branch November 22, 2017 15:58

stronnag mentioned this pull request Nov 24, 2017

busdev confused by inverted power on for SPRF3 #2503

Closed


		spiSetSpeed(MPU6000_SPI_INSTANCE, SPI_CLOCK_INITIALIZATON);

		if (!mpu6000InitDone) {


		#define BUSDEV_MAX_DEVICES 8

		static busDevice_t busDevPool[BUSDEV_MAX_DEVICES];

Migration to busDevice infrastructure: acc/gyro/mag/osd #2290

Migration to busDevice infrastructure: acc/gyro/mag/osd #2290

Uh oh!

Conversation

digitalentity commented Oct 8, 2017

Uh oh!

digitalentity commented Oct 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinbudden Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinbudden Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinbudden Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinbudden Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

digitalentity Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinbudden Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinbudden commented Oct 9, 2017

Uh oh!

martinbudden commented Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

digitalentity commented Oct 9, 2017

Uh oh!

digitalentity commented Nov 10, 2017

Uh oh!

digitalentity commented Nov 15, 2017

Uh oh!

stronnag commented Nov 16, 2017

Uh oh!

digitalentity commented Nov 17, 2017

Uh oh!

digitalentity commented Nov 17, 2017

Uh oh!

digitalentity commented Nov 17, 2017

Uh oh!

digitalentity commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martinbudden Oct 9, 2017 •

edited

Loading

martinbudden Oct 9, 2017 •

edited

Loading

martinbudden Oct 9, 2017 •

edited

Loading

martinbudden Oct 9, 2017 •

edited

Loading

digitalentity Oct 9, 2017 •

edited

Loading

martinbudden Oct 9, 2017 •

edited

Loading

martinbudden commented Oct 9, 2017 •

edited

Loading

digitalentity commented Nov 17, 2017 •

edited

Loading

digitalentity commented Nov 21, 2017 •

edited

Loading