AP_Compass: Add in BMM350 Driver #26987

cuav-chen2 · 2024-05-06T06:56:49Z

No description provided.

cuhome · 2024-05-18T01:44:07Z

@tridge Can you check it? It will be used in CUAV's new flight controller.

peterbarker

I haven't checked this against the data sheet, but it looks pretty good structure-wise.

Have you got logs of this with other existing compasses to compare it against? How else have you tested this compass?

peterbarker · 2024-05-20T06:13:48Z

libraries/AP_Compass/AP_Compass_BMM350.h

+    AP_HAL::OwnPtr<AP_HAL::Device> _dev;
+
+    /**
+     * @brief BMM350 magnetometer dut offset coefficient structure


peterbarker · 2024-05-20T06:17:55Z

libraries/AP_Compass/AP_Compass_BMM350.h

+    /**
+     * @brief BMM350 magnetometer cross axis compensation structure
+     */
+    struct bmm350_cross_axis


Suggested change

struct bmm350_cross_axis

struct cross_axis

bmm350 is redundant here as you're within the class. Similarly elsewhere

peterbarker · 2024-05-20T06:20:06Z

libraries/AP_Compass/AP_Compass_Backend.h

@@ -74,6 +74,7 @@ class AP_Compass_Backend
        DEVTYPE_AK09918 = 0x14,
        DEVTYPE_AK09915 = 0x15,
    	DEVTYPE_QMC5883P = 0x16,
+        DEVTYPE_BMM350 = 0x17,


Need this number in Tools/scripts/decode_devid.py

peterbarker · 2024-05-20T06:23:36Z

libraries/AP_Compass/AP_Compass_BMM350.cpp

+ */
+bool AP_Compass_BMM350::read_otp_data()
+{
+    uint8_t data = 0;


Suggested change

uint8_t data = 0;

peterbarker · 2024-05-20T06:24:09Z

libraries/AP_Compass/AP_Compass_BMM350.cpp

+    uint8_t msb = 0;
+    uint8_t lsb = 0;


Suggested change

uint8_t msb = 0;

uint8_t lsb = 0;

narrow the scope of variables as much as possible.

peterbarker · 2024-05-20T06:31:50Z

libraries/AP_Compass/AP_Compass_BMM350.cpp

+        otp_data[index] = ((uint16_t)(msb << 8) | lsb) & 0xFFFF;
+    }
+
+    {


Probably don't need this level of indent here.

peterbarker · 2024-05-20T06:36:54Z

libraries/AP_Compass/AP_Compass_BMM350.cpp

+    uint8_t chip_id = 0;
+    uint8_t data = 0;


Move closer to where they're used, remove the initialisation as their value should only be checked after a successful read.

peterbarker · 2024-05-20T06:37:59Z

libraries/AP_Compass/AP_Compass_BMM350.cpp

+
+    // Configure interrupt settings and enable DRDY
+    // Set INT mode as PULSED | active polarity | PUSH_PULL | unmap | DRDY interrupt
+    data = BMM350_INT_MODE_PULSED | BMM350_INT_POL_ACTIVE_HIGH | BMM350_INT_OD_PUSHPULL | BMM350_INT_OUTPUT_DISABLE | BMM350_INT_DRDY_EN;


peterbarker · 2024-05-20T06:39:19Z

libraries/AP_Compass/AP_Compass_BMM350.cpp

+    int32_t magx_raw = 0;
+    int32_t magy_raw = 0;
+    int32_t magz_raw = 0;
+    int32_t temp_raw = 0;
+


Suggested change

int32_t magx_raw = 0;

int32_t magy_raw = 0;

int32_t magz_raw = 0;

int32_t temp_raw = 0;

peterbarker · 2024-05-20T06:39:35Z

libraries/AP_Compass/AP_Compass_BMM350.cpp

+    }
+
+    // Converts raw data to signed integer value
+    magx_raw = fix_sign((data.magx[0] + ((uint32_t)data.magx[1] << 8) + ((uint32_t)data.magx[2] << 16)), BMM350_SIGNED_24_BIT);


Suggested change

magx_raw = fix_sign((data.magx[0] + ((uint32_t)data.magx[1] << 8) + ((uint32_t)data.magx[2] << 16)), BMM350_SIGNED_24_BIT);

const int32_t magx_raw = fix_sign((data.magx[0] + ((uint32_t)data.magx[1] << 8) + ((uint32_t)data.magx[2] << 16)), BMM350_SIGNED_24_BIT);

etc

andyp1per · 2024-05-20T06:25:03Z

libraries/AP_Compass/AP_Compass.cpp

@@ -515,7 +516,7 @@ const AP_Param::GroupInfo Compass::var_info[] = {
    // @Param: DISBLMSK
    // @DisplayName: Compass disable driver type mask
    // @Description: This is a bitmask of driver types to disable. If a driver type is set in this mask then that driver will not try to find a sensor at startup
-    // @Bitmask: 0:HMC5883,1:LSM303D,2:AK8963,3:BMM150,4:LSM9DS1,5:LIS3MDL,6:AK09916,7:IST8310,8:ICM20948,9:MMC3416,11:DroneCAN,12:QMC5883,14:MAG3110,15:IST8308,16:RM3100,17:MSP,18:ExternalAHRS
+    // @Bitmask: 0:HMC5883,1:LSM303D,2:AK8963,3:BMM150,4:LSM9DS1,5:LIS3MDL,6:AK09916,7:IST8310,8:ICM20948,9:MMC3416,11:DroneCAN,12:QMC5883,14:MAG3110,15:IST8308,16:RM3100,17:MSP,18:ExternalAHRS,21:BMM350


Shouldn't you use 19 here?

andyp1per · 2024-05-20T06:36:43Z

libraries/AP_Compass/AP_Compass_BMM350.cpp

+        otp_data[index] = ((uint16_t)(msb << 8) | lsb) & 0xFFFF;
+    }
+
+    {


Don't think you need this - C++ allows declarations in the middle

andyp1per · 2024-05-20T06:37:13Z

libraries/AP_Compass/AP_Compass_BMM350.cpp

+            return false;
+        }
+
+        otp_data[index] = ((uint16_t)(msb << 8) | lsb) & 0xFFFF;


need to make this explicitly unsigned

andyp1per · 2024-05-20T06:37:55Z

libraries/AP_Compass/AP_Compass_BMM350.cpp

+        mag_comp.dut_offset_coef.z = fix_sign(off_z_lsb_msb, BMM350_SIGNED_12_BIT);
+        mag_comp.dut_offset_coef.temp = t_off / 5.0f;
+
+        sens_x = (otp_data[BMM350_MAG_SENS_X] & 0xFF00) >> 8;


explicitly unsigned etc

andyp1per · 2024-05-20T06:39:04Z

libraries/AP_Compass/AP_Compass_BMM350.cpp

+        sens_z = (otp_data[BMM350_MAG_SENS_Z] & 0xFF00) >> 8;
+        sens_t = (otp_data[BMM350_TEMP_OFF_SENS] & 0xFF00) >> 8;
+
+        mag_comp.dut_sensit_coef.x = sens_x / 256.0f;


usually our pattern is to multiple 1/constant to avoid slow STM32 instructions

andyp1per · 2024-05-20T06:40:39Z

libraries/AP_Compass/AP_Compass_BMM350.cpp

+        mag_comp.dut_sensit_coef.z = sens_z / 256.0f;
+        mag_comp.dut_sensit_coef.temp = sens_t / 512.0f;
+
+        tco_x = (otp_data[BMM350_MAG_TCO_X] & 0x00FF);


you could probably just cast otp_data as uint8_t* and avoid a lot of this masking.

andyp1per · 2024-05-20T06:42:08Z

libraries/AP_Compass/AP_Compass_BMM350.cpp

+        return false;
+    }
+    // Wait for sensor complete bit reset
+    hal.scheduler->delay(14);


be nice to have a comment as to whether these delays are mandated by the spec

andyp1per · 2024-05-20T06:42:44Z

libraries/AP_Compass/AP_Compass_BMM350.cpp

+    if (mode == BMM350_POWER_MODE_NORMAL) {
+        hal.scheduler->delay(38);   // Switch from suspend mode to normal mode
+    } else {
+        hal.scheduler->delay(20);   // AVERAGING_4


Not clear what this comment is about

andyp1per · 2024-05-20T06:44:20Z

libraries/AP_Compass/AP_Compass_BMM350.cpp

+
+void AP_Compass_BMM350::read()
+{
+	drain_accumulated_samples(_compass_instance);


wrong whitespace

AP_Compass: Add in BMM350 Driver

654620c

cuav-chen2 force-pushed the cuav_pr_compass_bmm350 branch from c618911 to 654620c Compare May 6, 2024 07:42

peterbarker requested changes May 20, 2024

View reviewed changes

andyp1per requested changes May 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AP_Compass: Add in BMM350 Driver #26987

AP_Compass: Add in BMM350 Driver #26987

cuav-chen2 commented May 6, 2024

cuhome commented May 18, 2024

peterbarker left a comment

peterbarker May 20, 2024

peterbarker May 20, 2024

peterbarker May 20, 2024

peterbarker May 20, 2024

peterbarker May 20, 2024

peterbarker May 20, 2024

peterbarker May 20, 2024

peterbarker May 20, 2024

peterbarker May 20, 2024

peterbarker May 20, 2024

andyp1per May 20, 2024

andyp1per May 20, 2024

andyp1per May 20, 2024

andyp1per May 20, 2024

andyp1per May 20, 2024

andyp1per May 20, 2024

andyp1per May 20, 2024

andyp1per May 20, 2024

andyp1per May 20, 2024

	data = BMM350_INT_MODE_PULSED \| BMM350_INT_POL_ACTIVE_HIGH \| BMM350_INT_OD_PUSHPULL \| BMM350_INT_OUTPUT_DISABLE \| BMM350_INT_DRDY_EN;
	const uint8_t data = BMM350_INT_MODE_PULSED \| BMM350_INT_POL_ACTIVE_HIGH \| BMM350_INT_OD_PUSHPULL \| BMM350_INT_OUTPUT_DISABLE \| BMM350_INT_DRDY_EN;

	magx_raw = fix_sign((data.magx[0] + ((uint32_t)data.magx[1] << 8) + ((uint32_t)data.magx[2] << 16)), BMM350_SIGNED_24_BIT);
	const int32_t magx_raw = fix_sign((data.magx[0] + ((uint32_t)data.magx[1] << 8) + ((uint32_t)data.magx[2] << 16)), BMM350_SIGNED_24_BIT);

AP_Compass: Add in BMM350 Driver #26987

Are you sure you want to change the base?

AP_Compass: Add in BMM350 Driver #26987

Conversation

cuav-chen2 commented May 6, 2024

cuhome commented May 18, 2024

peterbarker 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

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