drivers/bmx280: reworked driver and added SPI mode #8383

haukepetersen · 2018-01-19T10:44:35Z

Issues/PRs references

For supporting the ruuvitag it is needed to interface the bmx280 sensor in SPI mode. While adding this mode, I found the driver was in a very bad state. This PR adds the possibility to use the driver in SPI mode and it significantly improves the drivers structure and style:

bugs fixed (I wonder how these were able to go by previous reviews...):

move global variables into device descriptor
guard bus access (use acquire and release)

added functionality:

enable SPI mode

structural improvements:

reduce stack usage
simplify the driver's structure
centralize bus access code
use assertions
cleanup includes
use shortcuts for bus access

style changes:

fix line length
cleanup and improve doxygen
unify pointer notation (char var over char var)
unify (error) return messages
use #ifdef MODULE_BME280 instead of #if defined(BME..)
unify debug messages -> using [bmx28] x: msg scheme

This PR also adapts and improves the test application accordingly and also add the bme280_spi module as default SAUL module to the ruuvitag.

Note on commits: as it was impossible to cleanly split the rework and cleanup efforts from the SPI mode addition to the driver, I put all changes into a single commit...

Testing:

The changes are tested and verified for the bmx280_spi and bmp280_spi on the ruuvitag. I don't have access to any board having these sensors connected via I2C, so the I2C mode still needs to be tested.

haukepetersen · 2018-01-31T11:35:27Z

rebased and fixed initialization and init_master order for the i2c mode (according to #8446).

haukepetersen · 2018-03-02T08:47:46Z

rebased. Anyone willing to review?

aabadie · 2018-03-02T11:09:26Z

drivers/Makefile.dep

@@ -47,14 +47,27 @@ ifneq (,$(filter bmp180,$(USEMODULE)))
  USEMODULE += xtimer
 endif

-ifneq (,$(filter bmx055,$(USEMODULE)))
-  FEATURES_REQUIRED += periph_i2c
+ifneq (,$(filter bme280_spi,$(USEMODULE)))


Why not use the wildcard % and merge this one with the block under ? I think the wildcard could also be used in the block below that handles the distinction between spi and i2c.

as this block and the one below include different base modules (bme vs bmp).

But for the last block of this module I could actually use wildcards, will give this a try.

Simplified the dependencies a little bit, better now?

aabadie · 2018-03-02T11:14:22Z

drivers/bmx280/bmx280.c

+
+#ifdef MODULE_BME280
+    /* ctrl_hum must be written before ctrl_meas for changes to become
+     * effective ... */


The 3 dots are superfluous IMHO

aabadie · 2018-03-02T11:18:25Z

tests/driver_bmx280/main.c


-#define MAINLOOP_DELAY  (2 * 1000 * 1000u)      /* 2 seconds delay between printf's */
+#define MAINLOOP_DELAY  (2 * US_PER_SEC)      /* read sensor every 2 seconds */


We could just use xtimer_sleep(2) below and drop this line I think

aabadie

I already added some comments but looking more in depth, I find it difficult to make the distinction between what is related to new spi feature and what is pure refactoring.
So I would recommend splitting this PR in 2: a first one only containing the refactoring and a second one with the new SPI feature.

Otherwise, I would also recommend avoiding comments like this: While adding this mode, I found the driver was in a very bad state. It's very subjective and initial authors/contributors/reviewers may not appreciate that.

aabadie · 2018-05-24T12:38:39Z

Maybe @dylad is interested by this.

haukepetersen · 2018-05-24T15:16:49Z

Sorry for the subjective grading, there was a high level of frustration involved when working on this driver so I got in a bad mood...

I know that splitting this PR would make a lot of sense from a reviewing perspective, but unfortunately this is not so easy. Adding the new SPI feature and reworking the driver's structure is highly correlated, so that splitting the changes would probably take another 1-2 days on top, which is not feasible for me to do. Do you guys think we could get this in somehow in the current state?

haukepetersen · 2018-05-24T15:43:01Z

addressed the so far open comments.

aabadie · 2018-05-24T16:26:12Z

Do you guys think we could get this in somehow in the current state?

Yes, on my side. But we need to wait for the I2C embargo first.

haukepetersen · 2018-05-24T16:44:35Z

awesome, thanks! Then lets get back to this PR once the I2C stuff is merged.

tests/driver_bmx280/main.c

PeterKietzmann · 2018-08-28T17:07:01Z

awesome, thanks! Then lets get back to this PR once the I2C stuff is merged.

It is!

haukepetersen · 2018-09-11T10:16:00Z

Will try to find the time and rebase this during the next days, stay tuned :-) (and sorry for the delay!)

herjulf · 2019-11-22T12:53:10Z

   Better..

Temperature [�°C]: 20.31
Pressure [Pa]: 103965
Humidity [%rH]: 31.78

benpicco · 2019-11-22T12:55:20Z

@haukepetersen are you aware of #12772?
If it's a simple fix, it's probably better to just include it here to avoid the additional churn.

haukepetersen · 2019-11-22T13:03:12Z

Nope, I was not aware of #12772. But IMHO we might just handle both PRs separately, it should be straight forward to rebase #12772 on top of this PR, right?

bugs fixed: - move global variables into device descriptor - guard bus access (use acquire and release) added functionality: - enable SPI mode structural improvements: - reduce stack usage - simplify the driver's structure - centralize bus access code - use assertions - cleanup includes - use shortcuts for bus access style changes: - fix line length - cleanup and improve doxygen - unify pointer notation (char *var over char* var) - unify (error) return messages - use `#ifdef MODULE_BME280` instead of `#if defined(BME..)` - unify debug messages -> using `[bmx28] x: msg` scheme

functional changes: - enable test to test the driver in SPI mode style changes and code simplification: - enable SPI mode - fixed typos in doxygen - fixed line length issues - simplified code - use fmt for formatting numbers - use US_PER_SEC instead of magic value - use named return values provided by driver - use puts where ever applicable

haukepetersen · 2019-11-22T13:05:23Z

@herjulf thanks! I took the freedom to squash again...

benpicco · 2019-11-22T13:09:28Z

Yea but I thought when you are editing the file anyway it would be trivial to add those lines.
_do_measurement() is only called in bmx280_read_temperature() and since both other functions rely on the temperature too, it makes sense to just call it there.

Or maybe just have a general bmx280_read(int16_t *temperature, uint32_t *pressure, uint16_t *humidity) (where arguments may be NULL) to avoid multiple measurements if all values are needed.

But that could also be done in a separate PR.

haukepetersen · 2019-11-22T13:18:35Z

But then I get in trouble for not keeping authorship for selected contributions :-)

maribu

I don't want this to go in without a proper discussion about the architecture of the driver. I'll add reasons later.

haukepetersen · 2019-11-22T13:28:40Z

I don't want this to go in without a proper discussion about the architecture of the driver. I'll add reasons later.

Are you serious? Please open a separate issue, you are always welcome to rework this driver, but please lets get it merged for now!

maribu · 2019-11-22T13:35:56Z

@haukepetersen: Out of all people working on RIOT, you're the single person that really just should not complain if a PR is blocked last minute.

benpicco · 2019-11-22T13:37:06Z

I also think that an iterative approach is sensible here.
Just because the code is merged doesn't mean it's forever frozen in ice.
I agree that a modular architecture is more desirable, but this is not the only driver that uses this 'old' pattern. I think having a working state committed and then improving on that has more values for users of the SPI version now than optimizing for people who might want to connect both the I2C and the SPI version of that sensor to the same board.

haukepetersen · 2019-11-22T13:41:21Z

Its not about the last minute, its about the reasoning. The architecture of this driver as proposed in this PR is in line with the driver design as of now. I think we can all agree, that this PR makes the situation better than it was before the PR, so it does not hurt. Also it does not introduce anything architecturally weird that breaks with design practices, so again, IMO no blocker there.

Now starting a more generic discussion on how to design drivers in general is something we can always have, but please in a generic context!

maribu · 2019-11-22T14:03:20Z

drivers/bmx280/bmx280.c

+#if defined(MODULE_BME280_SPI) || defined(MODULE_BME280_I2C)
+    /* ctrl_hum must be written before ctrl_meas for changes to become
+     * effective */
+    reg = dev->params.humid_oversample;
+    if (_write_reg(dev, BME280_CTRL_HUM_REG, reg) != BMX280_OK) {
+        goto err;
+    }
+#endif


It is not even possible to have a BMP280 and a BME280 using the same interface.

Nope, it is not. BUT
a) it is an extremely rare use case to have both connected concurrently
b) you might just use the bme280 driver config with both devices as a workaround
c) the convention in RIOT we always used to follow is not to over complicate things for the last 1% of exotic use cases
d) this has nothing to do with the content of this PR

kaspar030 · 2019-11-22T14:05:02Z

Now starting a more generic discussion on how to design drivers in general is something we can always have, but please in a generic context!

+1.

@benpicco #12772 is a distinct change and should be seperate, if it eases reviewing. This PR's review is basically done, so adding changes will certainly not ease reviewing. Actually, at this point it is against procedures. Also, the other change is from a different author and came months after this PR. Let's not force re-opening this PR on @haukepetersen, integrate #12772 mentally, ...

@maribu IIRC this PR doesn't change the driver's architecture by much, but improves the current implementation. The architecture discussion should be kept seperate.

maribu · 2019-11-22T14:35:24Z

but support for different driver modes simultaneously: definitively NO.

It was a deliberate design choice to implement bus support as different driver modes. And this design choice is what I challenge.

Now starting a more generic discussion on how to design drivers in general is something we can always have, but please in a generic context!

It is not really my intent to start a discussion, but rather enforce general design goals already agreed upon.

Citing RIOT's driver guide:

Third, it should always be possible, to handle more than a single device of one kind. Drivers and their interfaces are thus designed to keep their state information in a parameterized location instead of driver defined global variables.

The wording of the sentence is not super clear, but to me this clearly means that a driver should not prevent a user from using multiple devices of the same kind even though attached to different buses.

Also there is a broad consensus on the use if ifdefs: They should be avoided, unless solid technical reasons can be found to justify them. (See e.g. #12626 as a recent confirmation of that consensus.)

Not properly factoring out yields to such cases. This is clearly not the worst, e.g. when looking at the at86rf2xx driver we in stuff like this:

https://github.com/RIOT-OS/RIOT/blob/master/drivers/at86rf2xx/at86rf2xx.c#L51-58

https://github.com/RIOT-OS/RIOT/blob/master/drivers/at86rf2xx/at86rf2xx.c#L63-L71

https://github.com/RIOT-OS/RIOT/blob/master/drivers/at86rf2xx/at86rf2xx.c#L76-L87

https://github.com/RIOT-OS/RIOT/blob/master/drivers/at86rf2xx/at86rf2xx.c#L127-L136

https://github.com/RIOT-OS/RIOT/blob/master/drivers/at86rf2xx/at86rf2xx_getset.c#L232-L240

https://github.com/RIOT-OS/RIOT/blob/master/drivers/at86rf2xx/at86rf2xx_internal.c#L147-L154

https://github.com/RIOT-OS/RIOT/blob/master/drivers/at86rf2xx/at86rf2xx_netdev.c#L218-L226

Someone starts to go the lazy route to just hack in "flavors" of the driver using #ifs and #ifdefs, and next time you like they are all over the place.

maribu · 2019-11-22T14:38:31Z

I also think that an iterative approach is sensible here.
Just because the code is merged doesn't mean it's forever frozen in ice.

Agreed. And maybe it makes sense to migrate the driver guideline to github, so this is more visible and content changes in there are properly reviewed.

The architecture can be fixed in a separte PR

haukepetersen · 2019-11-22T15:34:20Z

Citing RIOT's driver guide:

Third, it should always be possible, to handle more than a single device of one kind. Drivers and their interfaces are thus designed to keep their state information in a parameterized location instead of driver defined global variables.

It happens that I know that paragraph quite well :-) In general the state of the bmx280 driver does actually comply to that paragraph - at least to me the of one kind means that we should support e.g. multiple bme280 or multiple bmp280, but mixing them is IMHO not of one kind.

But don't get me wrong, I am not against discussing and adapting the design guide lines and best practices in driver design for RIOT - after all circumstances change, we all gain experience, and hence we assess decisions differently now. I think we should just keep those generic discussions should not per default block everything in its path...

haukepetersen · 2019-11-22T16:00:14Z

@benpicco all green, would you do me the honor?

@maribu thanks for yielding in this case. Looking forward to the design discussion. Especially how a fully dynamic configuration will compare in terms of efficiency ;-)

haukepetersen assigned aabadie and kaspar030 Jan 19, 2018

PeterKietzmann self-requested a review January 22, 2018 09:00

haukepetersen force-pushed the opt_driver_bmx280spi branch from e9c73e4 to 1f06485 Compare January 31, 2018 11:34

jnohlgard added the Area: SAUL Area: Sensor/Actuator Uber Layer label Feb 19, 2018

haukepetersen force-pushed the opt_driver_bmx280spi branch from 1f06485 to f75590e Compare March 2, 2018 08:47

aabadie reviewed Mar 2, 2018

View reviewed changes

aabadie requested changes Mar 2, 2018

View reviewed changes

haukepetersen force-pushed the opt_driver_bmx280spi branch from f75590e to d98c63b Compare May 24, 2018 15:39

haukepetersen mentioned this pull request May 25, 2018

periph/i2c: tracking the remodeling #6577

Closed

47 tasks

This was referenced May 27, 2018

drivers: bmx280: adapt to new I2C api. #9206

Merged

cpu: efm32: adapt to new I2C interface #9208

Merged

kaspar030 mentioned this pull request Jun 15, 2018

bmx280 test: use constants for bmx280_init return values #9313

Closed

nmeum reviewed Jun 18, 2018

View reviewed changes

tests/driver_bmx280/main.c Outdated Show resolved Hide resolved

silkeh mentioned this pull request Oct 1, 2018

drivers/bmx280: improve documentation #10094

Merged

haukepetersen added 7 commits November 22, 2019 14:04

board/ruuvitag: enable bme280 for saul default

3c0d504

boards/sltb001a: use i2c mode of bme280 driver

f467318

drivers/bmx280: improved documentation

9f5f6f4

boards/avr-rss2: use bme280_i2c module

96c6313

tests/driver_bmx280: add Makefile.ci

5e51030

haukepetersen force-pushed the opt_driver_bmx280spi branch from fe8f7bb to 5e51030 Compare November 22, 2019 13:04

maribu previously requested changes Nov 22, 2019

View reviewed changes

maribu reviewed Nov 22, 2019

View reviewed changes

maribu merged commit 16ef0a7 into RIOT-OS:master Nov 22, 2019

haukepetersen deleted the opt_driver_bmx280spi branch November 22, 2019 16:57

maribu mentioned this pull request Nov 22, 2019

drivers/bmx280: Fix incorrect endian conversion #12789

Merged

fjmolinas added this to the Release 2020.01 milestone Dec 13, 2019


		#define MAINLOOP_DELAY (2 * 1000 * 1000u) /* 2 seconds delay between printf's */
		#define MAINLOOP_DELAY (2 * US_PER_SEC) /* read sensor every 2 seconds */

drivers/bmx280: reworked driver and added SPI mode #8383

drivers/bmx280: reworked driver and added SPI mode #8383

Uh oh!

Conversation

haukepetersen commented Jan 19, 2018

Issues/PRs references

Testing:

Uh oh!

haukepetersen commented Jan 31, 2018

Uh oh!

haukepetersen commented Mar 2, 2018

Uh oh!

aabadie Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

haukepetersen May 24, 2018

Choose a reason for hiding this comment

Uh oh!

haukepetersen May 24, 2018

Choose a reason for hiding this comment

Uh oh!

aabadie Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

aabadie Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

aabadie commented May 24, 2018

Uh oh!

haukepetersen commented May 24, 2018

Uh oh!

haukepetersen commented May 24, 2018

Uh oh!

aabadie commented May 24, 2018

Uh oh!

haukepetersen commented May 24, 2018

Uh oh!

Uh oh!

PeterKietzmann commented Aug 28, 2018

Uh oh!

haukepetersen commented Sep 11, 2018

Uh oh!

herjulf commented Nov 22, 2019

Uh oh!

benpicco commented Nov 22, 2019

Uh oh!

haukepetersen commented Nov 22, 2019

Uh oh!

haukepetersen commented Nov 22, 2019

Uh oh!

benpicco commented Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haukepetersen commented Nov 22, 2019

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Nov 22, 2019

Uh oh!

maribu commented Nov 22, 2019

Uh oh!

benpicco commented Nov 22, 2019

Uh oh!

haukepetersen commented Nov 22, 2019

Uh oh!

maribu Nov 22, 2019

Choose a reason for hiding this comment

Uh oh!

haukepetersen Nov 22, 2019

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Nov 22, 2019

Uh oh!

maribu commented Nov 22, 2019

Uh oh!

maribu commented Nov 22, 2019

benpicco commented Nov 22, 2019 •

edited

Loading