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.

[PetervdPerk-NXP]

Code looking good, could you remove the merge commit, we typically rebase pull requests instead of merging them.

[Viliuks]

Code looking good, could you remove the merge commit, we typically rebase pull requests instead of merging them.

Rebased and changed the head to remove merge commit.

[jlaitine]

LGTM!

[dagar]

Breaking out the configuration to parameters is fine if you're actually going to use it, but sometimes less is more. For example the estimator isn't even going to do anything with magnetometer data faster than 100 Hz. On the extreme lower end it's just going to cause problems hitting timeouts and delayed data (EKF2_MAG_DELAY) that's wildly different than typical.

A good setting would be whatever maximizes the actual data sampling (minimize potential for aliasing), while fitting comfortably on the bus at a reasonable rate (eg 100 kHz), doesn't add any additional filtering or latency in the device itself, and most importantly has actually been tested end-to-end.