drivers/lpsxxx: refactor lps331ap and add support for lps25hb + lps22hb by aabadie · Pull Request #10695 · RIOT-OS/RIOT

aabadie · 2019-01-03T09:14:56Z

Contribution description

This PR is a rework of the initial ST lps331ap atmospheric pressure and temperature sensor. It does several things:

move the original driver name from lps331ap to lpsxxx
add support for lps25hb and lps22hb sensors
use pseudo-modules for specific names
add a check on the device type using the WHO_AM_I register
change the default address from 0x44 to 0x5D (why was it 0x44 initially, I don't know). The default address depends on the SDO/SA0 pad: when connected to ground it is 0x5D, when connected to power supply, it is 0x5C. On ST x-nucleo-iks01a2 (lps22hb) and x-nucleo-iks01a1 (lps25hb) extensions, the pad is connected to ground, thus using 0x5D by default, makes more sense (to me)
The driver API has changed to use return codes (specific to this driver) instead of returning the measurement values. The SAUL adaption was adapted accordingly
A default configuration for lps22hb is provided for the ST B-l475E-IOT01A board since it embeds this sensor.

For simplicity, only basic polling mode with continuous measurements is provided by the driver. It should be possible to implement the other modes: One shot, Interrupt (with data ready) or FIFO in an extend version. Maybe a good candidate for a follow-up ? (Note that it might become a bit messy because of #ifdefs).

Testing procedure

This PR can be fully tested using the IoT-LAB testbed, because some boards provides the 3 types of sensors:

Start an experiment on IoT-LAB with boards providing the different sensor: iotlab-m3 (lps331ap), st-iotnode (b-l475e-iot01a with lps22hb), arduino-zero (lps25hb on x-nucleo-iks01a1 extension) and nrf51dk (lps22hb on x-nucleo-iks01a2 extension):

$ iotlab-experiment submit -n test_pr -d 120 -l 1,archi=arduino-zero:xbee+site=saclay -l 1,archi=st-iotnode:multi+site=saclay -l 1,archi=m3:at86rf231+site=saclay -l 1,archi=nrf51dk:ble+site=saclay

Build and flash the tests/driver_lpsxxx test application on the different nodes and verify that it works:

$ make BOARD=iotlab-m3 IOTLAB_NODE=auto-ssh -C tests/driver_lpsxxx flash term
$ DRIVER=lps25hb make BOARD=arduino-zero IOTLAB_NODE=auto-ssh -C tests/driver_lpsxxx flash term
$ DRIVER=lps22hb make BOARD=nrf51dk IOTLAB_NODE=auto-ssh -C tests/driver_lpsxxx flash term
$ DRIVER=lps22hb make BOARD=b-l475e-iot01a IOTLAB_NODE=auto-ssh -C tests/driver_lpsxxx flash term

To check saul integration, build and flash the examples/default with the right driver module loaded (not needed on iotlab-m3 and b-l475e-iot01a as lps331ap and lps22hb are automatically provided):

$ make BOARD=iotlab-m3 IOTLAB_NODE=auto-ssh -C examples/default flash term
$ USEMODULE=lps25hb make BOARD=arduino-zero IOTLAB_NODE=auto-ssh -C examples/default flash term
$ USEMODULE=lps22hb make BOARD=nrf51dk IOTLAB_NODE=auto-ssh -C examples/default flash term
$ make BOARD=b-l475e-iot01a IOTLAB_NODE=auto-ssh -C examples/default flash term

Issues/PRs references

closes #7267 (quite an old one, isn't it?), tick an item of #7585

MrKevinWeiss

Looks pretty good. Maybe take a look at the comments and I can start the testing once you have made the decision how to handle them (either ignore or implement). I would also like to wait for @smlng or someone else to take a look at but that can come after testing.

MrKevinWeiss · 2019-01-03T11:01:37Z

drivers/lpsxxx/lpsxxx.c

+    float res = TEMP_BASE;      /* reference value -> see datasheet */
+
+    i2c_acquire(DEV_I2C);
+    if (i2c_read_reg(DEV_I2C, DEV_ADDR, LPSXXX_REG_TEMP_OUT_L, &tmp, 0) < 0) {


Less lines if you read 16 bits and only swap if big endian?

I was done this way initially. Reading multiple bytes in one go requires extra play with BDU bit in CTRL_REG1 register. Otherwise it doesn't work with lps25h/lps22h.

Hmmm I see the point...

I am now curious if there is either a problem with an update that could occur after reading the lsb or if updates just don't work if only reading temperature since
1. To guarantee the correct behavior of BDU feature, PRESS_OUT_H (2Ah) must be the last address read.

@MrKevinWeiss, I have just tested it and there's no problem on both lps25hb and lps22hb.

even if I don't read the pressure measure at all.

Hmm OK, then do we worry about the MSB updating after the LSB read?

Or can we assume the BDU takes care of that? If so I suppose I am happy and can start testing with iotlabs

can we assume the BDU takes care of that?

I would say so.

MrKevinWeiss · 2019-01-03T11:01:58Z

drivers/lpsxxx/lpsxxx.c

+
+    i2c_acquire(DEV_I2C);
+
+    if (i2c_read_reg(DEV_I2C, DEV_ADDR, LPSXXX_REG_PRESS_OUT_XL, &tmp, 0) < 0) {


Same as above but with 3 bytes.

MrKevinWeiss · 2019-01-03T15:03:34Z

tests/driver_lpsxxx/main.c

+        lpsxxx_enable(&dev);
+        xtimer_sleep(1); /* wait a bit for the measurements to complete */
+
+        lpsxxx_read_temp(&dev, &temp);


Maybe it would be interesting to read/print several temperatures in a row to see if a change occurs then read the pressures. This can expose the update problem I am thinking of.

Good idea, I'll try this.

aabadie · 2019-01-07T13:27:10Z

@MrKevinWeiss, still interested in testing this ?

MrKevinWeiss · 2019-01-07T13:42:17Z

Yup, starting soon!

MrKevinWeiss · 2019-01-07T13:55:44Z

Wow, it seems like it is pretty hot in saclay 😆
iotlab-m3:

saul read 6
Reading from #6 (lps331ap|SENSE_TEMP)
Data:	     39.25°C

aabadie · 2019-01-07T14:00:18Z

it seems like it is pretty hot in saclay

It may look a bit weird indeed !
It's just that the node is inside a box with another embedded Linux board and the whole thing has no air cooling => it's a bit warm inside the box. The other sensors (lps22hb/lps25hb) are "outside".

MrKevinWeiss · 2019-01-07T14:03:19Z

So no beach sand and sun glasses?

aabadie · 2019-01-07T14:07:26Z

So no beach sand and sun glasses?

Unfortunately no... 😎

MrKevinWeiss · 2019-01-07T14:09:37Z

Testing is good, ran all the tests described above as well as manually reading multiple temperatures with saul to ensure the temperature gets updated without having to read pressure. I would like to wait for @smlng before continuing. I will ACK the testing side though.

aabadie · 2019-01-08T10:03:37Z

@MrKevinWeiss, I don't think the ARM and Impact:major labels are of any sense for this PR: the driver is supposed to be used on any architecture and the impact is quite low (only a single driver is modified and 2 new are added). Maybe the New feature label makes more sense.

miri64 · 2019-01-08T10:05:35Z

I second that.

miri64 · 2019-01-08T10:07:09Z

"Impact: major" should always be read like: might require more than one reviewer (i.e. also at least two ACKs) to be merged.

MrKevinWeiss · 2019-01-08T10:23:30Z

I was told any API change should be major, I agree though that this doesn't seem like a major change.

aabadie · 2019-02-13T17:34:22Z

sure, please squash

done !

smlng · 2019-02-15T10:20:39Z

drivers/include/lpsxxx.h

+ * @brief   The sensors default output data rate (ODR)
+ */
+#if MODULE_LPS331AP || MODULE_LPS25HB
+#define LPSXXX_DEFAULT_RATE     (LPSXXX_RATE_7HZ)


why not simply use LPSXXX_RATE_1HZ as default for all?

smlng · 2019-02-15T10:21:24Z

looks good in general, untested ACK.

aabadie · 2019-02-19T13:52:14Z

I fixed the small issue in the test README that was reported by codacy. And directly squashed.

Now the green merge button is very tempting ;)

kaspar030 · 2019-02-19T14:05:18Z

drivers/include/lpsxxx.h

+ * @brief   Read a temperature value from the given sensor, returned in m°C
+ *
+ * @param[in] dev       device descriptor of sensor to read from
+ * @param[out] temp     temperature value in c°C


m°C or c°C?

kaspar030 · 2019-02-19T14:17:44Z

drivers/lpsxxx/lpsxxx.c

+    res += ((float)val) / TEMP_DIVIDER;
+
+    /* return temperature in c°C */
+    *temp = (int16_t)(res * 100);


I think you can get rid of the float conversion & division:

(TEMP_BASE + (val / TEMP_DIVIDER)) * 100 == (TEMP_BASE * 100) + (val * 100 / TEMP_DIVIDER)

So this should be equivalent:

#define TEMP_BASE=4250U; #define TEMP_DIVIDER=480U; uint32_t res = TEMP_BASE; uint32_t val = 0; // ... res += (val * 100) / TEMP_DIVIDER; *temp = (int16_t)res;

(maybe use signed int).

I tried but that doesn't work on lps331ap/lps25hb.

The default address is the same one

aabadie · 2019-03-29T08:49:40Z

Any chance to get this merged ? It's all green for quite some time now.

smlng · 2019-03-29T08:55:58Z

I give Murdock another spin, if it still holds green, I'd hit it.

aabadie · 2019-03-29T09:03:39Z

Thanks @smlng :)

aabadie · 2019-03-29T09:20:46Z

It's all green @smlng

aabadie requested review from MrKevinWeiss and smlng January 3, 2019 09:14

aabadie mentioned this pull request Jan 3, 2019

Track support for ST b-l475e-iot01a board #7585

Open

12 tasks

aabadie requested a review from vincent-d January 3, 2019 10:05

aabadie added this to the Release 2019.01 milestone Jan 3, 2019

MrKevinWeiss reviewed Jan 3, 2019

View reviewed changes

MrKevinWeiss assigned MrKevinWeiss and unassigned MrKevinWeiss Jan 7, 2019

MrKevinWeiss added Platform: ARM Platform: This PR/issue effects ARM-based platforms Impact: major The PR changes a significant part of the code base. It should be reviewed carefully labels Jan 7, 2019

MrKevinWeiss added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Jan 7, 2019

miri64 removed Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Jan 8, 2019

aabadie force-pushed the pr/drivers/lpsxxx branch from ef522ec to 794a5e1 Compare February 13, 2019 17:31

smlng reviewed Feb 15, 2019

View reviewed changes

smlng approved these changes Feb 15, 2019

View reviewed changes

smlng added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Feb 15, 2019

aabadie force-pushed the pr/drivers/lpsxxx branch from 794a5e1 to a4fa24a Compare February 19, 2019 12:56

kaspar030 reviewed Feb 19, 2019

View reviewed changes

aabadie force-pushed the pr/drivers/lpsxxx branch from a4fa24a to 51d3f00 Compare February 20, 2019 07:15

aabadie force-pushed the pr/drivers/lpsxxx branch from 51d3f00 to db6463d Compare February 28, 2019 09:18

PeterKietzmann mentioned this pull request Mar 5, 2019

drivers: add support for lps25h #7267

Closed

aabadie added 7 commits March 20, 2019 14:43

drivers/lpsxxx: unify lps331ap and lps25h drivers

1f4dee9

tests/driver_lpsxx: use new unified driver

f3b7b60

sys/auto_init: rename lps331ap to lpsxxx unified version

5338d04

boards/iotlab: update lps331ap device addr define

80b337b

The default address is the same one

drivers/lpsxxx: add support for lps22hb

7c285eb

tests/driver/lpsxxx: update with support for lps22hb

0916126

boards/b-l475e-iot01a: add defines and deps for lps22hb

a218bc6

aabadie force-pushed the pr/drivers/lpsxxx branch from db6463d to a218bc6 Compare March 20, 2019 13:43

smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 29, 2019

aabadie merged commit 14f47bf into RIOT-OS:master Mar 29, 2019

aabadie deleted the pr/drivers/lpsxxx branch April 10, 2019 19:29


		i2c_acquire(DEV_I2C);

		if (i2c_read_reg(DEV_I2C, DEV_ADDR, LPSXXX_REG_PRESS_OUT_XL, &tmp, 0) < 0) {

Conversation

aabadie commented Jan 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

MrKevinWeiss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie commented Jan 7, 2019

Uh oh!

MrKevinWeiss commented Jan 7, 2019

Uh oh!

MrKevinWeiss commented Jan 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aabadie commented Jan 7, 2019

Uh oh!

MrKevinWeiss commented Jan 7, 2019

Uh oh!

aabadie commented Jan 7, 2019

Uh oh!

MrKevinWeiss commented Jan 7, 2019

Uh oh!

aabadie commented Jan 8, 2019

Uh oh!

miri64 commented Jan 8, 2019

Uh oh!

miri64 commented Jan 8, 2019

Uh oh!

MrKevinWeiss commented Jan 8, 2019

Uh oh!

aabadie commented Feb 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smlng commented Feb 15, 2019

Uh oh!

aabadie commented Feb 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie commented Mar 29, 2019

Uh oh!

smlng commented Mar 29, 2019

Uh oh!

aabadie commented Mar 29, 2019

Uh oh!

aabadie commented Mar 29, 2019

Uh oh!

Reviewers

aabadie commented Jan 3, 2019 •

edited

Loading

MrKevinWeiss commented Jan 7, 2019 •

edited

Loading