drivers: sensor: add bosch bmm350 driver #85174

maxd-nordic · 2025-02-04T17:42:22Z

upstream Bosch's BMM350 driver.
this is necessary for the thingy91x.

pdgendt · 2025-02-10T10:00:05Z

drivers/sensor/bosch/bmm350/CMakeLists.txt

@@ -0,0 +1,8 @@
+# Copyright (c) 2024 Bosch Sensortec GmbH


Update the copyright year

Suggested change

# Copyright (c) 2024 Bosch Sensortec GmbH

# Copyright (c) 2025 Bosch Sensortec GmbH

I'm not working for Bosch, I'm just integrating their code. Not sure if that's something I'm allowed to do.

What do you mean with "their code", if you copied code then you should adhere to the original code's copyright. Otherwise if you produced this code, you shouldn't be putting Bosch's copyright, but your own.

Bosch wrote this code, including the copyright headers and sent it to me. I was told it's okay to share, so I'm doing that here.

I guess this is OK? @carlescufi

Bosch needs to clarify this directly I guess.

@kegov Could you confirm?

@maxd-nordic & @gmarull , I confirm.

drivers/sensor/bosch/bmm350/Kconfig

drivers/sensor/bosch/bmm350/bmm350.c

gmarull

this PR contains many portions copied from https://github.com/boschsensortec/BMM350_SensorAPI, which is BSD-3 licensed. Unless Bosch explicitly says they are happy to share this code as Apache-2.0, this PR can't go in.

drivers/sensor/bosch/bmm350/bmm350.c

gmarull · 2025-02-11T09:18:38Z

drivers/sensor/bosch/bmm350/bmm350.c

+				BMM350_GET_BITS(rx_buf[2], BMM350_PMU_CMD_VALUE);
+		}
+	} else {
+		ret = BMM350_E_NULL_PTR;


-1 on custom error handling schemes here, this is copy&paste from their "hal"

Is this a blocker?

gmarull · 2025-02-11T09:38:31Z

samples/sensor/bmm350/src/main.c

+		report_mag[0] = mag[0].val1 + mag[0].val2 / 100.0;
+		report_mag[1] = mag[1].val1 + mag[1].val2 / 100.0;
+		report_mag[2] = mag[2].val1 + mag[2].val2 / 100.0;


something's wrong if you can't use sensor_value_to_double/float

I'll fix the order of magnitude here

gmarull · 2025-02-11T09:38:57Z

samples/sensor/bmm350/src/main.c

+	/* Setting scale in G, due to loss of precision if the SI unit m/s^2
+	 * is used
+	 */
+	sampling_freq.val1 = 25;       /* Hz from 0.78 to 400*/
+	sampling_freq.val2 = 0;			 /*not use*/
+	oversampling.val1 = 1;          /*1 Normal mode  0suspend mode*/
+	oversampling.val2 = 0;          /*not use*/
+	sensor_attr_set(dev, SENSOR_CHAN_MAGN_XYZ,
+			SENSOR_ATTR_SAMPLING_FREQUENCY,
+			&sampling_freq);
+	/* Set sampling frequency last as this also sets the appropriate
+	 * power mode. If already sampling, change to 0.0Hz before changing
+	 * other attributes
+	 */
+	sensor_attr_set(dev, SENSOR_CHAN_MAGN_XYZ, SENSOR_ATTR_OVERSAMPLING,
+			&oversampling);


doing pseudo-power management here?

which change would you like me to do here?

how does oversampling relate to "1 Normal mode 0suspend mode", this looks wrong

This is just odd naming. It does match the chip's datasheet though. The driver only supports "normal mode" and "suspend mode", but you could theoretically put other values in the register.

This still looks very wrong, this chip does support oversampling mode, I gave comments in other area, that it should be able to be configured

you can configure oversampling with this. there is just a special check for the value "0", that turns the chip into low power mode. What change do I need to make here?

maxd-nordic · 2025-02-14T09:29:52Z

this PR contains many portions copied from https://github.com/boschsensortec/BMM350_SensorAPI, which is BSD-3 licensed. Unless Bosch explicitly says they are happy to share this code as Apache-2.0, this PR can't go in.

It was Bosch who gave me these files with the Apache-2.0 headers.

maxd-nordic · 2025-03-03T15:15:56Z

Hey folks! Thanks for the reviews! Please have another look. I think I caught all of the change requests.

XenuIsWatching · 2025-03-03T15:26:31Z

drivers/sensor/bosch/bmm350/bmm350.c

+}
+static uint8_t acc_odr_to_reg(const struct sensor_value *val)
+{
+	double odr = sensor_value_to_double((struct sensor_value *)val);


Do you have to use double here? This could bring in a lot of code and cause some overhead for FPU-less processors

Please don't make me rework this whole driver. Other Bosch drivers that are already part of Zephyr do the same.

A later PR can change this behavior for all drivers that have it.

I'm not sure why you need to rework this whole driver? You can easily do this with UQ27.5 fixed point format... might as well start somewhere with not using doubles then

static uint8_t acc_odr_to_reg(const struct sensor_value *val) { /* Convert sensor_value to fixed-point UQ27.5 format */ uint32_t fixed_odr = (val->val1 << 5) + (((uint64_t)val->val2 * 32) / 1000000); uint8_t reg = BMM350_DATA_RATE_100HZ; if ((fixed_odr >= 25) && (fixed_odr <= 50)) { /* 0.78125Hz to 1.5625Hz */ reg = BMM350_DATA_RATE_1_5625HZ; } else if ((fixed_odr > 50) && (fixed_odr <= 100)) { /* 1.5625Hz to 3.125Hz */ reg = BMM350_DATA_RATE_3_125HZ; } else if ((fixed_odr > 100) && (fixed_odr <= 200)) { /* 3.125Hz to 6.25Hz */ reg = BMM350_DATA_RATE_6_25HZ; } else if ((fixed_odr > 200) && (fixed_odr <= 400)) { /* 6.25Hz to 12.5Hz */ reg = BMM350_DATA_RATE_12_5HZ; } else if ((fixed_odr > 800) && (fixed_odr <= 1600)) { /* 12.5Hz to 25Hz */ reg = BMM350_DATA_RATE_25HZ; } else if ((fixed_odr > 1600) && (fixed_odr <= 3200)) { /* 25Hz to 50Hz */ reg = BMM350_DATA_RATE_50HZ; } else if ((fixed_odr > 3200) && (fixed_odr <= 6400)) { /* 50Hz to 100Hz */ reg = BMM350_DATA_RATE_100HZ; } else if ((fixed_odr > 6400) && (fixed_odr <= 12800)) { /* 100Hz to 200Hz */ reg = BMM350_DATA_RATE_200HZ; } else if ((fixed_odr > 12800) && (fixed_odr <= 25600)) { /* 200Hz to 400Hz */ reg = BMM350_DATA_RATE_400HZ; } else if (fixed_odr > 25600) { /* 400Hz */ reg = BMM350_DATA_RATE_400HZ; } return reg; }

actually, on second thought, there are a few different ways to approach this such as with sensor_value_to_micro... but I have to agree with @teburd in another comment about this fuzzy approach, I'm reverting my block on this now

XenuIsWatching · 2025-03-03T15:34:03Z

drivers/sensor/bosch/bmm350/bmm350.c

+		if ((odr == BMM350_DATA_RATE_400HZ) && (performance >= BMM350_AVERAGING_2)) {
+			performance_fix = BMM350_NO_AVERAGING;
+		} else if ((odr == BMM350_DATA_RATE_200HZ) && (performance >= BMM350_AVERAGING_4)) {
+			performance_fix = BMM350_AVERAGING_2;
+		} else if ((odr == BMM350_DATA_RATE_100HZ) && (performance >= BMM350_AVERAGING_8)) {
+			performance_fix = BMM350_AVERAGING_4;
+		}


suggestion: it may be good to print a LOG_WRN about how it's adjusting the oversampling rate from what the application tried to specify

XenuIsWatching · 2025-03-03T15:39:52Z

drivers/sensor/bosch/bmm350/Kconfig

+thread_stack_size = 1024
+source "drivers/sensor/Kconfig.trigger_template"
+
+config BMM350_SAMPLING_RATE_RUNTIME


I can't see where this is used

XenuIsWatching · 2025-03-03T15:41:13Z

drivers/sensor/bosch/bmm350/Kconfig

+config BMM350_THREAD_PRIORITY
+	int "Own thread priority"
+	depends on BMM350_TRIGGER_OWN_THREAD
+	default 10
+	help
+	  Priority of the thread used by the driver to handle interrupts.
+
+config BMM350_THREAD_STACK_SIZE
+	int "Own thread stack size"
+	depends on BMM350_TRIGGER_OWN_THREAD
+	default 1024
+	help
+	  Stack size of thread used by the driver to handle interrupts.


These look like they are defined twice as they are already defined with the Kconfig.trigger_template

XenuIsWatching · 2025-03-03T15:49:38Z

drivers/sensor/bosch/bmm350/bmm350.c

+		reg = BMM350_AVERAGING_8;
+		break;
+	default:
+		LOG_ERR("unsupported oversampling rate");


shouldn't this return a status code (-EINVAL) and not set to a 'default' value if a wrong value is tried, then of course bubble up the error code

.

gmarull · 2025-03-17T12:58:06Z

will let sensor maintainers/collaborators finish the review

teburd

Please, no part specific samples. Either make the sample generic for the task or add overlays to existing samples (magn_polling/magn_trig) for the board+part you want to use.

samples/sensor/bmm350/README.rst

teburd · 2025-03-17T14:11:11Z

drivers/sensor/bosch/bmm350/bmm350.c

+
+	uint8_t reg = BMM350_DATA_RATE_100HZ;
+
+	if ((odr >= 0.78125) && (odr <= 1.5625)) {


I really don't love these fuzzy ODR setters, but I know there's precedence in the tree for them.

What's weird about this idea is that you can set arbitrary ODRs and from the API response and call it may seem as if any ODR is possible because hey, it didn't fail!

The reality is the actual ODR is set to something else entirely... and unless you go read this attribute back you don't know.

Maybe ODR should be something we do more explicitly rather than with attributes. It's incredibly common, typically gets tied to some sample power mode (toggled/always on are common...) and really should give back the ODR in use rather than simply accepting any ODR and replying back with "Ok!"

TL;DR no changes required but I don't love this

drivers/sensor/bosch/bmm350/bmm350.c

teburd · 2025-03-17T14:19:19Z

drivers/sensor/bosch/bmm350/bmm350.c

+	return rslt;
+}
+
+static int bmm350_sample_fetch(const struct device *dev, enum sensor_channel chan)


What happens if the sensor is suspended through PM API? We don't have a solid answer for this today with the fetch+get API other than the application doing the power management reference counting on the sensors. But if the device is suspended and this is called, what happens?

All registers are available in suspend mode. They will not update when no measurements are run. Do you need this to be changed?

No, but this might be surprising behavior to a caller, maybe worth a warning log if CONFIG_DEBUG or something is toggled?

I think this behaves as it should with the PM model.

It doesn't make sense to me to clutter the driver with this. Other drivers will also just try to fetch, no matter if the power mode allows for it.

So will the registers continue to hold a reading from some past moment in time then or be all zeros?

Add driver for Bosch BMM350 magnetometer sensor. Signed-off-by: Maximilian Deubel <maximilian.deubel@nordicsemi.no>

maxd-nordic · 2025-03-18T12:13:24Z

@teburd @XenuIsWatching @yperess Could you please unblock this PR?

XenuIsWatching

Please see https://github.com/zephyrproject-rtos/zephyr/pull/85174/files#r2002362614

XenuIsWatching

I changed my mind after think it about it more on the last comment,

lgtm now

No blocking issues left, though I think some things need to be clarified in how sensor drivers should be written.

carlescufi · 2025-03-20T10:36:59Z

Agreed to @MaureenHelm to add @teburd and @yperess as assignees as she is on vacation this week

zephyrbot added area: Samples Samples area: Sensors Sensors labels Feb 4, 2025

zephyrbot requested review from asemjonovs, avisconti, jeppenodgaard, kartben, MaureenHelm, nashif, teburd, tristan-google, ubieda and yperess February 4, 2025 17:43

zephyrbot assigned MaureenHelm Feb 4, 2025

maxd-nordic force-pushed the bmm350-upstream branch 5 times, most recently from 984e072 to 7f86bca Compare February 5, 2025 12:50

maxd-nordic mentioned this pull request Feb 5, 2025

drivers: bmm350 nrfconnect/sdk-nrf#20069

Closed

maxd-nordic force-pushed the bmm350-upstream branch from 7f86bca to e809e15 Compare February 10, 2025 09:51

pdgendt reviewed Feb 10, 2025

View reviewed changes

maxd-nordic force-pushed the bmm350-upstream branch from e809e15 to ae8ec0b Compare February 10, 2025 12:29

maxd-nordic requested a review from pdgendt February 10, 2025 13:21

gmarull previously requested changes Feb 11, 2025

View reviewed changes

gmarull reviewed Feb 11, 2025

View reviewed changes

drivers/sensor/bosch/bmm350/bmm350.c Outdated Show resolved Hide resolved

gmarull reviewed Feb 11, 2025

View reviewed changes

maxd-nordic force-pushed the bmm350-upstream branch from ae8ec0b to a7d3868 Compare February 14, 2025 10:16

maxd-nordic requested review from XenuIsWatching, yasinustunerg, kegov and yperess March 3, 2025 15:15

maxd-nordic force-pushed the bmm350-upstream branch 2 times, most recently from f98c10a to 71d6a76 Compare March 3, 2025 15:44

XenuIsWatching requested changes Mar 3, 2025

View reviewed changes

maxd-nordic force-pushed the bmm350-upstream branch 2 times, most recently from 7c4e77a to afa35aa Compare March 14, 2025 14:55

maxd-nordic requested a review from XenuIsWatching March 14, 2025 14:56

teburd requested review from afontaine-invn and removed request for kegov and yasinustunerg March 17, 2025 14:04

teburd previously requested changes Mar 17, 2025

View reviewed changes

maxd-nordic force-pushed the bmm350-upstream branch from afa35aa to 75c6726 Compare March 17, 2025 16:38

maxd-nordic requested a review from teburd March 17, 2025 16:40

drivers: sensor: add Bosch BMM350 magnetometer driver

cc9f68d

Add driver for Bosch BMM350 magnetometer sensor. Signed-off-by: Maximilian Deubel <maximilian.deubel@nordicsemi.no>

maxd-nordic force-pushed the bmm350-upstream branch from 75c6726 to cc9f68d Compare March 18, 2025 11:52

yperess approved these changes Mar 18, 2025

View reviewed changes

XenuIsWatching requested changes Mar 19, 2025

View reviewed changes

XenuIsWatching approved these changes Mar 19, 2025

View reviewed changes

carlescufi assigned teburd and yperess Mar 20, 2025

carlescufi merged commit 878ab53 into zephyrproject-rtos:main Mar 20, 2025
20 checks passed

	# Copyright (c) 2024 Bosch Sensortec GmbH
	# Copyright (c) 2025 Bosch Sensortec GmbH


		uint8_t reg = BMM350_DATA_RATE_100HZ;

		if ((odr >= 0.78125) && (odr <= 1.5625)) {

drivers: sensor: add bosch bmm350 driver #85174

drivers: sensor: add bosch bmm350 driver #85174

Conversation

maxd-nordic commented Feb 4, 2025 • edited Loading

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

gmarull 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

maxd-nordic commented Feb 14, 2025

maxd-nordic commented Mar 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XenuIsWatching Mar 19, 2025 • edited Loading

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

gmarull commented Mar 17, 2025

teburd left a comment • edited Loading

Choose a reason for hiding this comment

teburd Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teburd Mar 17, 2025 • edited Loading

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

maxd-nordic commented Mar 18, 2025

XenuIsWatching left a comment

Choose a reason for hiding this comment

XenuIsWatching left a comment

Choose a reason for hiding this comment

carlescufi commented Mar 20, 2025

maxd-nordic commented Feb 4, 2025 •

edited

Loading

XenuIsWatching Mar 19, 2025 •

edited

Loading

teburd left a comment •

edited

Loading

teburd Mar 17, 2025 •

edited

Loading

teburd Mar 17, 2025 •

edited

Loading