Add driver for SPS30 particulate matter sensor #13256

MichelRottleuthner · 2020-01-31T18:50:43Z

Contribution description

This PR provides a sensor driver for the Sensirion SPS30 particulate matter sensor.
~~I added the WIP label for now because I'm still planning to add SAUL support and improve the test application a bit.~~ [edit:] done
For now I only implemented I2C support but the sensor could also be used via UART.
Any opinions on whether I should add that too? If yes, how (i.e. runtime vs. compiletime selection) ?

Testing procedure

Check the documentation of sps30.h to see if it is sufficient to get it running and run the test application under tests/driver_sps30.
[edit:]
SAUL adaption is now included so the generic saul example can also be used for testing:
USEMODULE="sps30" BOARD=<your_board_name> make -C examples/saul all flash term

[edit 2:]
Keep in mind that the I2C bus must be running in standard mode speed. You might need to change that in your boards periph_conf.h

Issues/PRs references

Might be a good candidate for discussing about drivers with support for multiple interfaces (i.e. #13208).

drivers/include/sps30.h

smlng · 2020-02-03T10:49:43Z

drivers/include/sps30.h

+ * @brief   SPS30 device parameters
+ */
+typedef struct {
+    i2c_t i2c_dev;                /**< I2C dev the sensor is connected to */


with the above this could later be changed to union to allow for I2C and UART support.

drivers/sps30/sps30.c

MichelRottleuthner · 2020-02-03T17:38:24Z

Since the WIP label is still set I allowed myself to squash and rebase directly..

MichelRottleuthner · 2020-02-07T09:22:19Z

@smlng want to give it another look?

smlng

some more comments, and suggestions. No serious blockers though.

drivers/include/sps30.h

drivers/sps30/sps30.c

sys/auto_init/saul/auto_init_sps30.c

drivers/include/sps30.h

smlng · 2020-02-15T20:26:58Z

finally got it working - after changing cables twice, the device seems to be very sensible 😦 However, driver test and saul test are working as expected.

Please fix CI erros and squash afterwards. Then I'll make a final review round.

MichelRottleuthner · 2020-02-17T10:33:58Z

@smlng thanks for testing! Can you explain what exactly was sensible and which board you used? Was it that the sensor required stronger pull-ups, didn't work with long wires, or something like that?

smlng · 2020-02-17T11:10:49Z

For testing I used a bluepill board, and yes I think the wires were too long and/or didn't make good connection - not sure I first tried a "nice" wiring on a bread board with these u-shaped jumper bridges/wires (?), these didn't work.

Finally I used short (10cm) jumper wires, I also used the pull-ups you provided, which worked.

MichelRottleuthner · 2020-02-18T12:38:18Z

fixed CI errors and some typos reported by travis and directly squashed to avoid repeated CI builds. Travis also complains about the line containing the datasheet link being to long but there seems to be no nice way to break the url and keep the link working...

smlng · 2020-02-18T12:43:35Z

tests/driver_sps30/main.c

+#define TEST_START_DELAY_US          (2 * US_PER_SEC)
+#define SENSOR_RESET_DELAY_US        (10 * US_PER_SEC)
+#define SENSOR_STARTUP_DELAY_US      (10 * US_PER_SEC)
+#define POLL_FOR_READY_US            (1000000U)


looks like you could go for seconds and use xtimer_sleep instead of xtimer_usleep?

also the latter should be 1 * US_PER_SEC otherwise?

smlng · 2020-02-18T12:52:14Z

mhm, maybe we need to adjust the vera++ settings @jia200x

100 chars per line to error seems a bit harsh, maybe try 120
what about links in markdown?

MichelRottleuthner · 2020-02-18T13:24:55Z

what about links in markdown?

tried that, nothing seems to be working. I took the lazy way out using an url shortener.

smlng · 2020-02-18T13:29:19Z

I took the lazy way out using an url shortener.

lets hope that one is stable enough in the long run 🤞

MichelRottleuthner · 2020-02-18T13:33:55Z

yeah my first thought was also against that but practically there is no difference between the URL shortener quitting service and the manufacturer breaking the links. Actually I encountered the latter more often :P ...and we need to update the link anyway then. Maybe a custom url shortener under riot-os.org would be a nice thing for stuff like that 🤔

smlng · 2020-02-18T13:34:39Z

drivers/include/saul.h

@@ -148,41 +148,76 @@ enum {
 * and the SAUL intra-category ID (six least significant bits).
 */
 enum {
-    SAUL_ACT_ANY            = SAUL_CAT_ACT | SAUL_ACT_ID_ANY,               /**< any actuator - wildcard */


not sure if we really want to change it this way, maybe its better to relax vera++ settings to allow 120 chars per line before error

ah damn coding conventions are strict for 100 char tops, and I don't want to start a lengthy discussion (now)

So we leave it as is?

yeah for now, maybe we should discuss the 100 char line limit in our coding conventions and argue for 120. IIRC Python has 120 max, bc 80 is nice to have but often a bit to strict.

just compiled the doc, still looks good - so let's go for it, regardless of the 100 - 120 chars discussion.

the only thing I don't like about this change is that now there are two different styles of documentation for enums in this file.

Yeah but I still favor same-line-comments if possible.

smlng · 2020-02-18T13:37:22Z

Maybe a custom url shortener under riot-os.org would be a nice thing for stuff like that

yep thought the same

smlng · 2020-02-20T13:07:23Z

drivers/include/sps30.h

+ *
+ * @return      #SPS30_OK on success, negative #sps30_error_code_t on error
+ */
+int sps30_start_fan_clean(const sps30_t *dev);


nit picky: there is no sps30_stop_fan_clean 😮

Yep because that is not a feature documented by the manufacturer. It could be possible to implement that via the reset command (not tested) but then this is a very unlikely thing to be needed. The clean will be done after a fixed time of 10 seconds anyway. And what kind of firmware would first trigger the clean and then decide it is better to stop it now?^^

yeah sure, was just bike-shedding 😄

If any I would change this to: sps30_clean_fan or even a simple sps30_clean.

ahh I leave it as is. I actually chose the name like that because directly maps to how the command is named in the datasheet.

drivers/sps30/sps30.c

smlng

I think we are good here 😄

MichelRottleuthner added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers labels Jan 31, 2020

smlng reviewed Feb 3, 2020

View reviewed changes

MichelRottleuthner force-pushed the pr_sps30 branch from d9643f6 to 676c43e Compare February 3, 2020 17:37

MichelRottleuthner added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Feb 7, 2020

smlng reviewed Feb 10, 2020

View reviewed changes

smlng reviewed Feb 13, 2020

View reviewed changes

drivers/include/sps30.h Show resolved Hide resolved

MichelRottleuthner force-pushed the pr_sps30 branch from 3b0aad5 to 874e83b Compare February 17, 2020 10:29

MichelRottleuthner requested review from aabadie, fjmolinas, leandrolanzieri, miri64 and MrKevinWeiss as code owners February 17, 2020 10:29

MichelRottleuthner force-pushed the pr_sps30 branch 2 times, most recently from ac6be65 to 80bd08f Compare February 18, 2020 12:34

smlng reviewed Feb 18, 2020

View reviewed changes

MichelRottleuthner force-pushed the pr_sps30 branch from 80bd08f to 2c2eab3 Compare February 18, 2020 13:20

smlng reviewed Feb 18, 2020

View reviewed changes

smlng mentioned this pull request Feb 18, 2020

code: relax max line length to 120 #13400

Closed

smlng reviewed Feb 20, 2020

View reviewed changes

drivers/sps30/sps30.c Outdated Show resolved Hide resolved

MichelRottleuthner force-pushed the pr_sps30 branch from 2c2eab3 to d515ad9 Compare February 20, 2020 13:19

MichelRottleuthner added 6 commits February 20, 2020 14:26

drivers: add driver for Sensirion SPS30 particulate matter sensor

c4093c2

tests: add test application for Sensirion SPS30 sensor driver

b5658e6

drivers/saul: add class and string for size measurements

46091b6

sys/phydat: add unit for count per cubic meter

36fca36

drivers/sps30: add saul integration

7e089bc

drivers/saul: satisfy vera++ (shorten lines to below 100 chars)

1743f96

MichelRottleuthner force-pushed the pr_sps30 branch from d515ad9 to 1743f96 Compare February 20, 2020 13:27

smlng approved these changes Feb 20, 2020

View reviewed changes

benpicco merged commit aa00191 into RIOT-OS:master Feb 21, 2020

leandrolanzieri added this to the Release 2020.04 milestone Mar 13, 2020

gschorcht mentioned this pull request May 14, 2025

boards: add Wemos D1 R32 (aka ESPDuino-32) board definition #21479

Merged

Add driver for SPS30 particulate matter sensor #13256

Add driver for SPS30 particulate matter sensor #13256

Uh oh!

Conversation

MichelRottleuthner commented Jan 31, 2020 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MichelRottleuthner commented Feb 3, 2020

Uh oh!

MichelRottleuthner commented Feb 7, 2020

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smlng commented Feb 15, 2020

Uh oh!

MichelRottleuthner commented Feb 17, 2020

Uh oh!

smlng commented Feb 17, 2020

Uh oh!

MichelRottleuthner commented Feb 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smlng commented Feb 18, 2020

Uh oh!

MichelRottleuthner commented Feb 18, 2020

Uh oh!

smlng commented Feb 18, 2020

Uh oh!

MichelRottleuthner commented Feb 18, 2020

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!

smlng commented Feb 18, 2020

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!

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

MichelRottleuthner commented Jan 31, 2020 •

edited

Loading