Add support for BMP581 #61535

MeisterBob · 2023-08-16T08:13:20Z

This Pullrequest add support for BMP581 Temperature/Pressure Sensor.

I started where @talhaHavadar stopped in #51849 and fixed the open issues that where left in this pull request. I also added altitude interpolation from a lookup table, because libc minimal doesn't come with pow() that is needed by the altitude calculation that was already implemented by @talhaHavadar.

The Commits must be squashed before merge, just left them as multiple commits for easier review.

talhaHavadar · 2023-08-16T08:24:25Z

Thank you very much for the effort @MeisterBob, I wasnt able to find time to finalize it.

I see that it was failing due to email mismatch, I have added my email into commits so it should be ok now.

talhaHavadar · 2023-08-16T09:03:23Z

tests/drivers/build_all/sensor/i2c.dtsi

@@ -552,6 +552,12 @@ test_i2c_bmp388: bmp388@55 {
 	int-gpios = <&test_gpio 0 0>;
 };

+test_i2c_bmp581: bmp581@46 {


I see that tests are failing due to duplicated address here. 46 is also being used by tmp112. It might be best to move bmp581 entry in dts to down to the end of the devices here.

The test was also failing because of a missing CONFIG_BMP581 in the tests prj.conf.

@talhaHavadar is right, the convention is to add new device at the end of the file with the next sequential address. Note this is just a build test and the address does not need to match the sensor datasheet.

Also, you should not need to set CONFIG_BMP581 in prj.conf if you configure the Kconfig symbol to default y

MaureenHelm

Please separate these changes into multiple commits, one for each bullet:

drivers: bmp581: add BMP581 driver

addition work I did on top of @talhaHavadar's code.
- i2c_addr isn't used by the driver as this is already present in the
  i2c_dt_spec struct. Also changed the i2c_dt_spec struct in bmp581_data
  to a pointer, no need to store the information twice.
- fixed soft-reset: ret was checked on time too often and it didn't use
  a helper function that was already present.
- added altitude interpolation: altitude calculation from pressure
  requires pow() that isn't present in minimal libc. The inpolation
  method interpolates the altitude between the values from a LUT. The
  calculation methods ca be selected with the
  `CONFIG_BMP581_ALTITUDE_CALC_[MATH|LUT|OFF]` Kconfig options.
- fixed tests

MaureenHelm · 2023-08-17T20:09:51Z

drivers/sensor/bmp581/Kconfig

+#
+
+config BMP581
+	bool "BMP581"


Suggested change

bool "BMP581"

bool "BMP581 altitude sensor"

default y

MaureenHelm · 2023-08-17T20:11:54Z

drivers/sensor/bmp581/bmp581.c

+int reg_read(uint8_t reg, uint8_t *data, uint16_t length, struct bmp581_data *drv)
+{
+	return i2c_burst_read_dt(&drv->i2c, reg, data, length);
+}
+
+int reg_write(uint8_t reg, const uint8_t *data, uint16_t length, struct bmp581_data *drv)
+{
+	return i2c_burst_write_dt(&drv->i2c, reg, data, length);
+}


Wrappers like this don't really add anything unless the sensor can support multiple bus interfaces (i.e., I2C and SPI). Please remove

The sensor also has an SPI interface. I can remove the wrapper or leave it to someone to add SPI support in the future. Please tell me how you want it.

Could you please give me some input about that?

The way these wrappers are currently implemented won't work when you add SPI (consider the case if you have two instances of this sensor, one attached to a SPI bus and the other to an I2C bus), so I suggest using i2c_burst_read_dt and friends directly for now.

I removed the wrappers.

MaureenHelm · 2023-08-17T20:12:32Z

drivers/sensor/bmp581/bmp581.c

+
+	ret = get_power_mode(&current_powermode, drv);
+	if (ret != BMP5_OK) {
+		LOG_DBG("Couldnt set the power mode because something went wrong when getting the "


MaureenHelm · 2023-08-17T20:13:25Z

drivers/sensor/bmp581/bmp581.c

+		 * mode or continuous mode.
+		 */
+
+		ret = reg_read(BMP5_REG_ODR_CONFIG, &odr, 1, drv);


Use i2c_reg_read_byte_dt

MaureenHelm · 2023-08-17T20:14:39Z

drivers/sensor/bmp581/bmp581.c

+			 */
+			odr = BMP5_SET_BITSLICE(odr, BMP5_DEEP_DISABLE, BMP5_DEEP_DISABLED);
+			odr = BMP5_SET_BITS_POS_0(odr, BMP5_POWERMODE, BMP5_POWERMODE_STANDBY);
+			ret = reg_write(BMP5_REG_ODR_CONFIG, &odr, 1, drv);


i2c_reg_write_byte_dt

MaureenHelm · 2023-08-17T20:18:42Z

drivers/sensor/bmp581/bmp581.c

+		sensor_value_from_double(&drv->last_sample.temperature,
+								(raw_temperature / 65536.0));


Don't need to use floating point to divide by a power of 2

You lose the decimal places when you divide an int by an int.

Some platforms in Zephyr don't have a hardware FPU so we try to avoid floating point operations if possible. I think you can do something like this:

/* use the integer part directly (may need to sign extend) */ drv->last_sample.temperature.val1 = data[2]; /* convert the fractional part to decimal */ drv->last_sample.temperature.val2 = 1000000 * ([data[1] << 8 | data[0]);

MaureenHelm · 2023-08-17T20:20:33Z

drivers/sensor/bmp581/bmp581.h

+#define BMP5_E_NULL_PTR		 -1
+#define BMP5_E_COM_FAIL		 -2
+#define BMP5_E_DEV_NOT_FOUND	 -3
+#define BMP5_E_INVALID_CHIP_ID	 -4
+#define BMP5_E_POWER_UP		 -5
+#define BMP5_E_POR_SOFTRESET	 -6
+#define BMP5_E_INVALID_POWERMODE -7
+#define BMP5_E_INVALID_THRESHOLD -8
+#define BMP5_E_FIFO_FRAME_EMPTY	 -9
+#define BMP5_E_NVM_INVALID_ADDR	 -10
+#define BMP5_E_NVM_NOT_READY	 -11


Please use standard values from errno.h

MaureenHelm · 2023-08-17T20:22:50Z

drivers/sensor/bmp581/bmp581.c

+	drv->i2c = cfg->i2c;
+
+	drv->i2c_addr = cfg->i2c_addr;


This shouldn't be needed

removed struct i2c_dt_spec i2c from struct bmp581_data

MaureenHelm · 2023-08-17T20:45:20Z

tests/drivers/build_all/sensor/i2c.dtsi

@@ -552,6 +552,12 @@ test_i2c_bmp388: bmp388@55 {
 	int-gpios = <&test_gpio 0 0>;
 };

+test_i2c_bmp581: bmp581@46 {


@talhaHavadar is right, the convention is to add new device at the end of the file with the next sequential address. Note this is just a build test and the address does not need to match the sensor datasheet.

Also, you should not need to set CONFIG_BMP581 in prj.conf if you configure the Kconfig symbol to default y

MaureenHelm · 2023-08-29T13:19:01Z

drivers/sensor/bmp581/bmp581.c

+	uint8_t odr = 0;
+	enum bmp5_powermode current_powermode;
+
+	if (dev == NULL) {


Most drivers don't check this. You can use CHECKIF to give the option to compile out the check

replaced all those NULL pointer checks with CHECKIF().

MaureenHelm · 2023-08-29T13:40:36Z

drivers/sensor/bmp581/bmp581.c

+int reg_read(uint8_t reg, uint8_t *data, uint16_t length, struct bmp581_data *drv)
+{
+	return i2c_burst_read_dt(&drv->i2c, reg, data, length);
+}
+
+int reg_write(uint8_t reg, const uint8_t *data, uint16_t length, struct bmp581_data *drv)
+{
+	return i2c_burst_write_dt(&drv->i2c, reg, data, length);
+}


The way these wrappers are currently implemented won't work when you add SPI (consider the case if you have two instances of this sensor, one attached to a SPI bus and the other to an I2C bus), so I suggest using i2c_burst_read_dt and friends directly for now.

jakkra · 2023-08-29T15:57:54Z

drivers/sensor/bmp581/bmp581.c

+		 */
+		double last_pressure = sensor_value_to_double(&drv->last_sample.pressure);
+		double last_temperature = sensor_value_to_double(&drv->last_sample.temperature);
+		double sealevel_pressure = 101325.0;


I think having this altitude feature in the current form is missleading, to get good accurate altitude measurements from pressure, you need to feed in the actual pressure at sea level at the location you are at, typically this can be fetched from APIs on internet if you know your location. The pressure at sea level will depend alot on the weather and will change quite a bit just during a day. By hardcoding the sea-level pressure you will get inaccurate altitude, and this could be confusing for users. My suggestion is to remove this feature from the driver as it's missleading for users that are not aware of this. The only way I see this feature useful, is to calculate relative height if you compare samples taken close in time, but still I'm not sure if it should be in the driver and called altitude.

Just my thought, what do you think?

I left it in because it was in @talhaHavadar's original PR. I'm totaly fine with removing the altitude calculation from the driver as altitude isn't a value you can read from the sensor.

MaureenHelm

My earlier comment about floating point arithmetic still needs to be addressed.

MaureenHelm · 2023-10-20T21:39:11Z

drivers/sensor/bmp581/bmp581.c

+		 * mode or continuous mode.
+		 */
+
+		ret = i2c_burst_read_dt(&conf->i2c, BMP5_REG_ODR_CONFIG, &odr, 1);


For single byte reads and writes, you can use i2c_reg_read_byte_dt and i2c_reg_write_byte respectively

dts/bindings/sensor/bosch,bmp581.yml

MeisterBob · 2023-11-10T10:24:08Z

@MaureenHelm I addressed the remaining issues and rebased onto main.

jilaypandya · 2023-11-16T16:11:24Z

tests/drivers/build_all/sensor/i2c.dtsi

+
+test_i2c_bmp581: bmp581@79 {
+	compatible = "bosch,bmp581";
+	reg = <0x70>;


should be 0x79

Rebased again and increased to 0x80 due to merge conflict.

teburd

looks like a nice driver add

This commit adds source and header files required for bmp581 I2C driver. I have used bmp581_user.h to add more usage related definitions but bmp581.h to add hardware related definitions. Signed-off-by: Talha Can Havadar <havadartalha@gmail.com> Signed-off-by: Gerhard Jörges <joerges@metratec.com>

MeisterBob · 2024-01-22T14:26:43Z

I rebased once more to fix the device address in tests/drivers/build_all/sensor/i2c.dtsi

MeisterBob · 2024-01-23T08:53:58Z

@teburd would you please reapprove

MeisterBob requested review from nashif, carlescufi, galak and MaureenHelm as code owners August 16, 2023 08:13

zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding area: Sensors Sensors labels Aug 16, 2023

zephyrbot requested review from avisconti, teburd, tristan-google and yperess August 16, 2023 08:13

zephyrbot assigned MaureenHelm Aug 16, 2023

talhaHavadar reviewed Aug 16, 2023

View reviewed changes

MeisterBob force-pushed the bmp581 branch 5 times, most recently from a1e7c2f to a9320a9 Compare August 17, 2023 07:31

MaureenHelm requested changes Aug 17, 2023

View reviewed changes

MeisterBob force-pushed the bmp581 branch 4 times, most recently from 576fa3b to 149c09e Compare August 22, 2023 10:12

MeisterBob force-pushed the bmp581 branch from 149c09e to c806035 Compare August 25, 2023 11:53

MaureenHelm reviewed Aug 29, 2023

View reviewed changes

jakkra reviewed Aug 29, 2023

View reviewed changes

MeisterBob force-pushed the bmp581 branch from c806035 to 441ca41 Compare August 30, 2023 09:51

carlescufi requested a review from MaureenHelm October 20, 2023 10:20

MaureenHelm requested changes Oct 20, 2023

View reviewed changes

MeisterBob force-pushed the bmp581 branch from 441ca41 to 1d3edf6 Compare November 10, 2023 10:06

MeisterBob force-pushed the bmp581 branch from 1d3edf6 to 96de8d0 Compare November 10, 2023 10:18

jilaypandya requested changes Nov 16, 2023

View reviewed changes

MeisterBob force-pushed the bmp581 branch 2 times, most recently from 12dec0c to 8006eb6 Compare November 17, 2023 08:29

teburd previously approved these changes Dec 15, 2023

View reviewed changes

MeisterBob dismissed teburd’s stale review via 97817ad January 22, 2024 14:25

MeisterBob force-pushed the bmp581 branch from 8006eb6 to 97817ad Compare January 22, 2024 14:25

MaureenHelm approved these changes Jan 22, 2024

View reviewed changes

teburd approved these changes Jan 24, 2024

View reviewed changes

nashif merged commit 4ce0555 into zephyrproject-rtos:main Jan 24, 2024
19 checks passed

		sensor_value_from_double(&drv->last_sample.temperature,
		(raw_temperature / 65536.0));

Add support for BMP581 #61535

Add support for BMP581 #61535

Conversation

MeisterBob commented Aug 16, 2023

talhaHavadar commented Aug 16, 2023 • edited

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

MaureenHelm 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakkra Aug 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaureenHelm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MeisterBob commented Nov 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teburd left a comment

Choose a reason for hiding this comment

MeisterBob commented Jan 22, 2024

MeisterBob commented Jan 23, 2024

talhaHavadar commented Aug 16, 2023 •

edited

jakkra Aug 29, 2023 •

edited