Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AP_Compass: Add in BMM350 Driver #26987

Closed
wants to merge 1 commit into from

Conversation

cuav-chen2
Copy link
Contributor

No description provided.

@cuhome
Copy link
Contributor

cuhome commented May 18, 2024

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

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

libraries/AP_Compass/AP_Compass_BMM350.h Outdated Show resolved Hide resolved
libraries/AP_Compass/AP_Compass_BMM350.h Outdated Show resolved Hide resolved
libraries/AP_Compass/AP_Compass_Backend.h Show resolved Hide resolved
libraries/AP_Compass/AP_Compass_BMM350.cpp Outdated Show resolved Hide resolved
libraries/AP_Compass/AP_Compass_BMM350.cpp Outdated Show resolved Hide resolved
libraries/AP_Compass/AP_Compass_BMM350.cpp Outdated Show resolved Hide resolved
libraries/AP_Compass/AP_Compass_BMM350.cpp Outdated Show resolved Hide resolved
libraries/AP_Compass/AP_Compass_BMM350.cpp Outdated Show resolved Hide resolved
libraries/AP_Compass/AP_Compass_BMM350.cpp Outdated Show resolved Hide resolved
libraries/AP_Compass/AP_Compass_BMM350.cpp Outdated Show resolved Hide resolved
@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you use 19 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

19:MMC5XX3,20:QMC5883P seems to be missing here, should I complete it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A follow-up PR would be good :-)

libraries/AP_Compass/AP_Compass_BMM350.cpp Outdated Show resolved Hide resolved
libraries/AP_Compass/AP_Compass_BMM350.cpp Outdated Show resolved Hide resolved
libraries/AP_Compass/AP_Compass_BMM350.cpp Outdated Show resolved Hide resolved
libraries/AP_Compass/AP_Compass_BMM350.cpp Outdated Show resolved Hide resolved
libraries/AP_Compass/AP_Compass_BMM350.cpp Outdated Show resolved Hide resolved
libraries/AP_Compass/AP_Compass_BMM350.cpp Outdated Show resolved Hide resolved
libraries/AP_Compass/AP_Compass_BMM350.cpp Outdated Show resolved Hide resolved
libraries/AP_Compass/AP_Compass_BMM350.cpp Outdated Show resolved Hide resolved
@cuav-chen2
Copy link
Contributor Author

Thanks to the developers for your reviews and suggestions, I will modify it based on your suggestions.

@cuav-chen2
Copy link
Contributor Author

Here is the data log of BMM350 compared to other compasses. mag0 is the BMM350, mag1 is another internal compass and mag2 is a external IST8310 compass.
x
y
z

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good.

Thanks for providing the testing data.

Marking for discussion at DevCallEU tonight

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A follow-up PR would be good :-)

{
uint8_t data[read_len + 2];

if (!_dev->read_registers(reg, data, read_len + 2)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!_dev->read_registers(reg, data, read_len + 2)) {
if (!_dev->read_registers(reg, data, ARRAY_SIZE(data))) {

while (boot_retries--) {
// Soft reset
if (!_dev->write_register(BMM350_REG_CMD, BMM350_CMD_SOFTRESET)) {
continue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No delay in this case. Not sure if that's a problem or not.

@peterbarker
Copy link
Contributor

I've created a separate PR wuith the commits appropriately split as I could not force-push your branch.

Thanks for this contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants