Add Bosch BMM350 magnetometer

[Viliuks]

Solved Problem

Added Bosch BMM350 magnetometer released in 2023 with variable sampling, averaging rates. And another option to set the pad drive settings, to fix issues with data overshooting/undershooting that happen using long wires.

New parameters:

BMM350_ODR - sets ODR mode
BMM350_AVG - sets averaging mode
BMM350_DRIVE - sets pad drive mode

Context

https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmm350-ds001.pdf

[PetervdPerk-NXP]

@jlaitine You made a BMM350 driver as well right for PX4?
Could you review this, I don't have hardware yet with a BMM350 but we're planning to.

[jlaitine]

@jlaitine You made a BMM350 driver as well right for PX4? Could you review this, I don't have hardware yet with a BMM350 but we're planning to.

Yes, I also offered to upstream it back then, but there was no interest I guess at the time. Our version of the driver is here, if needed for reference: tiiuae#692 .

I can check it out for sure, but not until some time next week

[jlaitine]

I started by trying it out on my BMM350 board. The driver initializes and values change when turning the mag, but the values I am getting are suspicious:

listener sensor_mag

TOPIC: sensor_mag
sensor_mag
timestamp: 572952315 (0.003969 seconds ago)
timestamp_sample: 572951853 (462 us before timestamp)
device_id: 15012873 (Type: 0xE5, I2C:1 (0x14))
x: -14.34939
y: -23.14031
z: 19.13099
temperature: 57.90157
error_count: 0

Earth's magnetic field is supposed to be < 1 gauss, it looks like there is some very large offset on all axis. I didn't debug what that might be, but is it possible that self test internal flux was left on on the axis?

[jlaitine]

I added some minor comments, overall it looks quite nice to me!

I wonder if it is reasonable to poll the sensor at ODR rate, or should the default ODR be lower than 200MHz? All the older mags are polled less frequently. I don't really have a strong opinion on this, but at least I don't have use for higher frequency mag data than 50Hz or so. Just maybe it is better to set the ODR & averaging in a way that the mag produces more averaged data less frequently?

I don't think I have rights to bring review any further than commenting, so approvals need to come from maintainers...

[Viliuks]

I added some minor comments, overall it looks quite nice to me!

I wonder if it is reasonable to poll the sensor at ODR rate, or should the default ODR be lower than 200MHz? All the older mags are polled less frequently. I don't really have a strong opinion on this, but at least I don't have use for higher frequency mag data than 50Hz or so. Just maybe it is better to set the ODR & averaging in a way that the mag produces more averaged data less frequently?

I don't think I have rights to bring review any further than commenting, so approvals need to come from maintainers...

Thank you for the review, great notes! I have updated the code accordingly. As for the big offsets I had the OTP word LSB/MSB registers mixed up.

Regarding the polling rate, I wasnt too sure too, I left it as is. If needed I can modify it to use the 50Hz polling rate as the older BMM150 magnetometer does.

[PetervdPerk-NXP]

Maybe make ODR configurable using a CLI argument and default to 50Hz.
Sometimes more is better but since it's an I2C sensor you've to share the bus with other peripherals as well.

[Viliuks]

Maybe make ODR configurable using a CLI argument and default to 50Hz. Sometimes more is better but since it's an I2C sensor you've to share the bus with other peripherals as well.

Should I then remove the parameter settings for it? Or do I extend the functionality which would use the set parameters if no CLI arguments are passed and keep the default values for the parameters 50Hz?

[PetervdPerk-NXP]

Maybe make ODR configurable using a CLI argument and default to 50Hz. Sometimes more is better but since it's an I2C sensor you've to share the bus with other peripherals as well.

Should I then remove the parameter settings for it? Or do I extend the functionality which would use the set parameters if no CLI arguments are passed and keep the default values for the parameters 50Hz?

Nvm parameters are fine, boards can override using the param set-default command let's keep it as is.
Regarding default I would say 50Hz then.

[Viliuks]

Maybe make ODR configurable using a CLI argument and default to 50Hz. Sometimes more is better but since it's an I2C sensor you've to share the bus with other peripherals as well.

Should I then remove the parameter settings for it? Or do I extend the functionality which would use the set parameters if no CLI arguments are passed and keep the default values for the parameters 50Hz?

Nvm parameters are fine, boards can override using the param set-default command let's keep it as is. Regarding default I would say 50Hz then.

Perfect, updated the module.yaml 😄

[jlaitine]

LGTM!

I also made a quick smoke test on my desk. It worked out of the box and the outputs look good.

I don't have a possibility to make any comprehensive ( or flight ) testing atm. But after this is merged, we'll replace our own driver with this one, which will then take it to the air in TII/SSRC organization.

Suggest squashing the commits before merging though

[DronecodeBot]

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/review-request-bosch-bmm350-driver/39830/1

[jlaitine]

I see that this one is still pending merging, @PetervdPerk-NXP is this waiting for something?

[justas-]

We need an approval from someone with write access.

[PetervdPerk-NXP]

@justas- Could you rebase on main and fix the CI errors?

../../src/drivers/magnetometer/bosch/bmm350/BMM350.cpp: In member function 'int BMM350::ReadOTPWord(uint8_t, uint16_t*)':
../../src/drivers/magnetometer/bosch/bmm350/BMM350.cpp:643:27: error: 'lsb' may be used uninitialized in this function [-Werror=maybe-uninitialized]
  643 |    *lsb_msb = ((msb << 8) | lsb) & 0xffff;
      |               ~~~~~~~~~~~~^~~~~~
compilation terminated due to -Wfatal-errors.