RIOT icon indicating copy to clipboard operation
RIOT copied to clipboard

drivers/abp2: add abp2 driver

Open dpproto opened this issue 1 year ago • 17 comments

Contribution description

  • Add a driver for ABP2 Honeywell pressure and temperature series.
  • Implement a SAUL interface.
  • Support the SPI version, but not the I2C version (I don't have that one). However, prepare the code for easy I2C support.
  • Document the code.

Testing procedure

Use the test application in RIOT/tests/drivers/abp2.

Output of the test application:

=========================
        Measuring
=========================
Pressure range = 0 .. 160000
Data:            0.0003 Bar
Data:            17.151 °C 
Data:            0.0003 Bar
Data:            17.777 °C 
Data:            0.0003 Bar
Data:            17.207 °C
------------------------------
Switch to blocking mode
------------------------------
Data:            0.0003 Bar
Data:            17.111 °C
Data:            0.0003 Bar
Data:            17.180 °C
Data:            0.0003 Bar
Data:            17.139 °C
------------------------------
Switch to SAUL mode
------------------------------

Dev: Pressure sensor (abp2)     Type: SENSE_PRESS
Data:            0.0003 Bar

Dev: Temperature sensor (abp2)  Type: SENSE_TEMP
Data:            17.180 °C

Dev: Pressure sensor (abp2)     Type: SENSE_PRESS
Data:            0.0003 Bar

Dev: Temperature sensor (abp2)  Type: SENSE_TEMP
Data:            17.220 °C

Dev: Pressure sensor (abp2)     Type: SENSE_PRESS
Data:            0.0003 Bar

Dev: Temperature sensor (abp2)  Type: SENSE_TEMP
Data:            17.166 °C
------------------------------
Switch to non-blocking mode
------------------------------
Data:            0.0003 Bar
Data:            17.247 °C
Data:            0.0003 Bar
Data:            17.201 °C
Data:            0.0003 Bar
Data:            17.175 °C
------------------------------
Switch to blocking mode
------------------------------

Issues/PRs references

Fixes #20233

dpproto avatar Feb 16 '24 20:02 dpproto

your commits messages will need rewording

https://github.com/RIOT-OS/RIOT/blob/master/CONTRIBUTING.md#commit-conventions

kfessel avatar Mar 13 '24 15:03 kfessel

your commits messages will need rewording

https://github.com/RIOT-OS/RIOT/blob/master/CONTRIBUTING.md#commit-conventions

Could you be more accurate ?

dpproto avatar Mar 14 '24 19:03 dpproto

your commits messages will need rewording https://github.com/RIOT-OS/RIOT/blob/master/CONTRIBUTING.md#commit-conventions

Could you be more accurate ?

Each commit should target changes of specific parts/modules of RIOT. The commits use the following pattern:

area of code: description of changes

You can use multi-line commit messages if you want to detail more the changes. For example:

periph/timer: Document that set_absolute is expected to wrap

Most timers are implemented this way already, and keeping (documenting)
it that way allows the generic timer_set implementation to stay as
simple as it is.

so you commit messages should be for example

driver/abp2: driver for SPI sensors prepare for I2C

tests/driver/abp2: test implementation

or something similar

the scheme is <arena of work>: <description>

kfessel avatar Mar 18 '24 10:03 kfessel

OK, done. Thanks for the details. It's my 1st contribution...

dpproto avatar Mar 18 '24 12:03 dpproto

aside from the leftover in the test app this seems good to go

That's good news!

please provide current test results from the current version starting at shell prompt (I don't have the hardware to test)

What do you mean? What about the program output in the 1st message of this thread?

dpproto avatar Mar 22 '24 17:03 dpproto

aside from the leftover in the test app this seems good to go

That's good news!

please provide current test results from the current version starting at shell prompt (I don't have the hardware to test)

What do you mean? What about the program output in the 1st message of this thread?

i see you updated it ( 1h ago there where non english words in it )

kfessel avatar Mar 22 '24 17:03 kfessel

Murdock results

:heavy_check_mark: PASSED

b7198cc3fbb1756425e1601766ac8526d75e5a3b tests/drivers/abp2: test implementation

Success Failures Total Runtime
10025 0 10026 10m:15s

Artifacts

riot-ci avatar Mar 22 '24 17:03 riot-ci

diff --git a/tests/drivers/saul_drivers/Makefile b/tests/drivers/saul_drivers/Makefile
index c729dc575e..da6d708ee5 100644
--- a/tests/drivers/saul_drivers/Makefile
+++ b/tests/drivers/saul_drivers/Makefile
@@ -53,6 +53,9 @@ ifneq (,$(filter vl6180x,$(DRIVERS)))
   USEMODULE += vl6180x_als
   USEMODULE += vl6180x_rng
 endif
+ifneq (,$(filter abp2,$(DRIVERS)))
+  USEMODULE += abp2_spi
+endif
 
 USEMODULE += saul_default

kfessel avatar Mar 25 '24 14:03 kfessel

diff --git a/tests/drivers/saul_drivers/Makefile b/tests/drivers/saul_drivers/Makefile
index c729dc575e..da6d708ee5 100644
--- a/tests/drivers/saul_drivers/Makefile
+++ b/tests/drivers/saul_drivers/Makefile
@@ -53,6 +53,9 @@ ifneq (,$(filter vl6180x,$(DRIVERS)))
   USEMODULE += vl6180x_als
   USEMODULE += vl6180x_rng
 endif
+ifneq (,$(filter abp2,$(DRIVERS)))
+  USEMODULE += abp2_spi
+endif
 
 USEMODULE += saul_default

Should I apply these changes and force push?

Also, I noticed that entries in this file are sorted. So these lines should be added further up in the file.

dpproto avatar Mar 25 '24 15:03 dpproto

Should I apply these changes and force push?

yes (problem noticed by murdock)

Also, I noticed that entries in this file are sorted. So these lines should be added further up in the file.

please add the lines where they belong - good you noticed

kfessel avatar Mar 25 '24 15:03 kfessel

btw are you also working towards i2c support?

kfessel avatar Mar 25 '24 15:03 kfessel

btw are you also working towards i2c support?

I have no plan to do so because I don't have a I2C version at hand.

dpproto avatar Mar 25 '24 15:03 dpproto

Hi, :up: Any chance this driver will ever make its way upstream ? :up:

dpproto avatar Aug 05 '24 14:08 dpproto

I feel a little uncomfortable with the i2c integration hints (code) and pseudo_module, please remove. Talked to other maintainers and we seem to align on that. The comments "i2c communication should go here" -even though it is obvious ( everywhere where spi is called i2c would also be)- is ok for me.

  • implementation wise it would be better to be able to use spi and i2c the at the same time -> the current hints are not the right direction (the module would provide either one or the other interface and work only for one i2c adress.
  • at some places this will seem like this driver supports i2c (eg if you list available modules) but it isn't -- this leads to: "hey RIOT supports abp2-i2c" -- "just the implementation is missing".
  • since you do not intend to write it, it will probably not exist.

If you want to hint: "hey the is something open TODO" write a TODO

kfessel avatar Aug 09 '24 14:08 kfessel

implementation wise it would be better to be able to use spi and i2c the at the same time

Why?

benpicco avatar Aug 09 '24 16:08 benpicco

Why?

Why should we Block that usecase (one might have a mix of both in one experiment or just have multiple sensors) the design hints just point into the direction of there will only be one sensor

simple example speed/flow (pito) and pressure measurement would need one differential and one absolute sensor (or two absolute) (ofcause these can be two spi (different cs), but with these hints i2c would not work (neither 2 i2c sensors nor can it be combined with spi)).

kfessel avatar Aug 14 '24 10:08 kfessel