Add support for ICM20789 IMU/Baro #6449

tridge · 2017-06-17T05:18:05Z

The ICM20789 is an interesting variant of the invensense IMUs that has gyro, accel and barometer.

OXINARF

I made a couple of comments.

OXINARF · 2017-06-23T01:31:09Z

libraries/AP_InertialSensor/AP_InertialSensor_Invensense.cpp

@@ -922,9 +946,12 @@ bool AP_InertialSensor_Invensense::_hardware_init(void)
        /* bus-dependent initialization */
        if ((_dev->bus_type() == AP_HAL::Device::BUS_TYPE_I2C) && (_mpu_type == Invensense_MPU9250)) {
            /* Enable I2C bypass to access internal AK8963 */
-            _register_write(MPUREG_INT_PIN_CFG, BIT_BYPASS_EN);
+            if (_mpu_type != Invensense_ICM20789) {


This is always true (given the previous check). I'm a bit confused though because this bit is set on start just for this MPU type, why not do it here instead?

Will be trying to do it here.

OXINARF · 2017-06-23T01:39:01Z

libraries/AP_InertialSensor/AP_InertialSensor_Invensense.cpp

+
+    if (_mpu_type == Invensense_ICM20789) {
+        // setup to allow for barometer
+        user_ctrl &= ~BIT_USER_CTRL_I2C_MST_EN;


Do we need to do this on every FIFO reset? Unless I'm missing something, user_ctrl should always have BIT_USER_CTRL_I2C_MST_EN unset - it is only set when _configure_slaves is called, where you added a check so that it returns immediately.

Will remove this.

OXINARF · 2017-06-23T01:46:03Z

libraries/AP_Baro/AP_Baro_ICM20789.cpp

+static struct {
+    uint32_t Praw, Traw;
+    float T, P;
+} dd;


Not dramatic in terms of space, but we should probably ifdef this inside a debug macro.

Will create BARO_ICM20789_DEBUG and also use it to define the Debug(..) macro

OXINARF · 2017-06-23T01:48:32Z

libraries/AP_Baro/AP_Baro_ICM20789.cpp

+        return;
+    }
+
+    if (_sem->take(0)) {


Can you use HAL_SEMAPHORE_BLOCK_FOREVER please? @peterbarker did a PR changing all places, I don't want to go back 🙂

Easy enough :-)

OXINARF · 2017-06-23T01:53:54Z

libraries/AP_Baro/AP_Baro_ICM20789.cpp

+    }
+
+    dev_icm->get_semaphore()->give();
+#endif // HAL_INS_MPU60x0_NAME


If this isn't defined, shouldn't we return false here? Or are you relying on the methods below to fail?

I'm going to add an early exit in an #else

OXINARF · 2017-06-23T02:06:47Z

libraries/AP_Baro/AP_Baro_ICM20789.h

+
+private:
+    AP_Baro_ICM20789(AP_Baro &baro, AP_HAL::OwnPtr<AP_HAL::Device> dev);
+    virtual ~AP_Baro_ICM20789(void) {};


Any reason to define the destructor?

Probably because "that's how all the backends do it".

Will remove all the empty backend destructors.

OXINARF · 2017-06-23T02:08:23Z

libraries/AP_Baro/AP_Baro_ICM20789.cpp

+    return true;
+
+ failed:
+    dev_icm->get_semaphore()->give();


What if HAL_INS_MPU60x0_NAME isn't defined? 🙂

Going to change things so we don't take the semaphore if the name isn't defined.

OXINARF · 2017-06-23T02:08:59Z

libraries/AP_Baro/AP_Baro_ICM20789.h

+
+    uint8_t instance;
+    AP_HAL::OwnPtr<AP_HAL::Device> dev;
+    AP_HAL::OwnPtr<AP_HAL::Device> dev_icm;


Why keep dev_icm? Looks like it is only needed for initialization.

Will make it a stack variable.

OXINARF · 2017-06-23T02:11:07Z

libraries/AP_Baro/examples/ICM20789/ICM20789.cpp

+#define BIT_PWR_MGMT_1_DEVICE_RESET          0x80 // reset entire device
+#define BIT_USER_CTRL_I2C_IF_DIS             0x10 // Disable primary I2C interface and enable hal.spi->interface
+#define BIT_PWR_MGMT_1_CLK_XGYRO             0x01 // PLL with X axis gyroscope reference
+#define BIT_PWR_MGMT_1_CLK_ZGYRO             0x03 // PLL with Z axis gyroscope reference


So many beautiful macros you don't use anywhere 😛

They're cheap - and might be useful to someone in the future...

OXINARF · 2017-06-23T02:13:38Z

libraries/AP_Baro/examples/ICM20789/ICM20789.cpp

+
+
+    i2c_dev = std::move(hal.i2c_mgr->get_device(1, 0x63));
+    lidar_dev = std::move(hal.i2c_mgr->get_device(1, 0x29));


What is the VL53L0X doing in the middle of this example? 😜

Will generally fix the example.

lucasdemarchi · 2017-06-29T16:28:08Z

libraries/AP_Baro/AP_Baro_ICM20789.cpp

+      Bypass Mode: Set register INT_PIN_CFG (Address: 55 (Decimal); 37 (Hex)) bit 1 to value 1 
+      and I2C_MST_EN bit is 0 Address: 106 (Decimal); 6A (Hex)). 
+      Pressure sensor data can then be accessed using the procedure described in Section 10.
+    */


This comment should be on the function above. And it looks strange... Are you setting it to read in bypass mode so you have both the IMU on spi bus and baro on I2C bus (i.e. you have real access to both SPI pins and I2C pins and they are not the same)? This would be different from all the other Invensense sensors in which there's an I2C transaction over the SPI bus. By the name of this register (I2C_MST_EN) it looks like you are indeed enabling the I2C master engine on the sensor.

lucasdemarchi · 2017-06-29T18:17:13Z

libraries/AP_Baro/AP_Baro_ICM20789.cpp

+    dev_icm->write_register(0x6B, 0x01);
+
+    hal.scheduler->delay(1);
+    dev_icm->write_register(0x6A, 0x10);


You are setting an IMU register without reading whatever the INS set there... This will give us trouble if later we need to set something in this register there. It would be nice to keep this IMU part on the INS driver like it is when we have a mag rather than a baro.

And... now that we have access to the plain I2C/SPI dev, we can also simplify the AuxBus infra a little bit.

@lucasdemarchi Should I move the initialisation into the IMU, or is it OK where it is for the time being?

baro is initialised before ins, which is why we need to init here, as ins doesn't yet exist

long ago I had this same problem when MS5611 was on the auxiliary bus of MPU6K... hence the infra for that. So if later on the INS drivers writes to this register, whatever we did here is lost. I think it's much safer to "keep INS registers on the INS driver and baro registers on the baro driver".

OXINARF · 2017-06-30T15:08:30Z

@lucasdemarchi Only @tridge can really answer, but my guess was that the current baro driver is that way because the INS driver isn't added to any board - so there is no init there and hence why everything is directly manipulated here. I should say though that the baro driver isn't added too and that, if that happens one day, it likely makes sense to add both the INS and baro drivers.

lucasdemarchi · 2017-06-30T16:10:14Z

@OXINARF isn't the imu support it exactly the other commit in this pr?

OXINARF · 2017-06-30T22:53:13Z

Yes, what I meant is that maybe @tridge is using the baro driver on some board without using the INS driver.

OXINARF · 2018-01-08T13:12:50Z

Closing this in favor of #7477.

@lucasdemarchi Please comment there!

tridge added 2 commits June 17, 2017 15:12

AP_InertialSensor: added ICM-20789 support

5c828df

AP_Baro: implement ICM20789 barometer

ed7060d

tridge force-pushed the pr-icm20789 branch from ff9729b to ed7060d Compare June 17, 2017 06:30

OXINARF added the Library label Jun 19, 2017

OXINARF requested changes Jun 23, 2017

View reviewed changes

lucasdemarchi reviewed Jun 29, 2017

View reviewed changes

peterbarker mentioned this pull request Jan 2, 2018

Add support for ICM20789 #7477

Closed

OXINARF closed this Jan 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for ICM20789 IMU/Baro #6449

Add support for ICM20789 IMU/Baro #6449

tridge commented Jun 17, 2017

OXINARF left a comment

OXINARF Jun 23, 2017

peterbarker Jan 2, 2018

OXINARF Jun 23, 2017

peterbarker Jan 2, 2018

OXINARF Jun 23, 2017

peterbarker Jan 2, 2018

OXINARF Jun 23, 2017

peterbarker Jan 2, 2018

OXINARF Jun 23, 2017

peterbarker Jan 2, 2018 •

edited

OXINARF Jun 23, 2017

peterbarker Jan 2, 2018

OXINARF Jun 23, 2017

peterbarker Jan 2, 2018

OXINARF Jun 23, 2017

peterbarker Jan 2, 2018

OXINARF Jun 23, 2017

peterbarker Jan 2, 2018

OXINARF Jun 23, 2017

peterbarker Jan 2, 2018

lucasdemarchi Jun 29, 2017

lucasdemarchi Jun 29, 2017

peterbarker Jan 2, 2018

tridge Jan 3, 2018

lucasdemarchi Jan 4, 2018

OXINARF commented Jun 30, 2017

lucasdemarchi commented Jun 30, 2017

OXINARF commented Jun 30, 2017

OXINARF commented Jan 8, 2018



		i2c_dev = std::move(hal.i2c_mgr->get_device(1, 0x63));
		lidar_dev = std::move(hal.i2c_mgr->get_device(1, 0x29));

Add support for ICM20789 IMU/Baro #6449

Add support for ICM20789 IMU/Baro #6449

Conversation

tridge commented Jun 17, 2017

OXINARF left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbarker Jan 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OXINARF commented Jun 30, 2017

lucasdemarchi commented Jun 30, 2017

OXINARF commented Jun 30, 2017

OXINARF commented Jan 8, 2018

peterbarker Jan 2, 2018 •

edited