driver/bme680: add I2C/SPI driver for BME680 device #12717

dylad · 2019-11-15T16:34:28Z

Contribution description

This PR provides support for BME680 sensor using the vendor driver from Bosch Sensortech as package.

The sensor provides measurement for temperature, humidity, pressure and VOC gas concentration. It can be connected either via I2C or via SPI. The driver supports multiple BME680 sensor and mixed I2C/SPI configurations.

The driver supports SAUL.

Testing procedure

Apply the parameters you want in the bme680_params.h file run tests/driver_bme680

with one I2C device

DRIVER='bme680_i2c'  BOARD=... make -C tests/driver_bme680 flash test

with one SPI device

DRIVER='bme680_spi'  BOARD=... make -C tests/driver_bme680 flash test

or with one I2C and one SPI device

DRIVER='bme680_spi bme680_i2c'  BOARD=... make -C tests/driver_bme680 flash term

SAUL can be tested with tests/saul. The used interface(s) has/have to be defined with USEMODULE, for example

USEMODULE='bme680_spi bme680_i2c' BOARD=... make -C tests/saul flash term

Issues/PRs references

Replaces #10502

tests/driver_bme680/tests/01-run.py

drivers/Makefile.dep

drivers/Makefile.include

gschorcht · 2019-11-15T16:51:24Z

I will review and test it tomorrow.

gschorcht · 2019-11-15T17:06:56Z

A comment on the naming of the package. If the number of packages imported in this way increases, it might be better to order them by category for readability. If not in separate directories then at least by starting the name with the category as we already do for test applications.

Therefore, I would suggest to call the package driver_bme680. It is the way I have already done for the VL53L1X in PR #10363. The vendor package used by the driver is called driver_vl53l1x. We should use a consistent naming scheme for drivers.

What do you think about it?

dylad · 2019-11-15T17:59:56Z

Therefore, I would suggest to call the package driver_bme680

I am not against and I'll apply this change unless someone is opposed to such naming scheme.

If the number of packages imported in this way increases

Agreed, but let's wait for some time if new drivers/pkgs appears in the future. I think I dedicated name with a category is better than a subfolder which will increase build complexity.

gschorcht · 2019-11-16T09:21:35Z

pkg/bme680_driver/contrib/bme680_hal.c

+#endif /* BME680_SPI_MODE */
+
+#ifndef BME680_NSS_PIN
+#define BME680_NSS_PIN  GPIO_PIN(PA, 5)


Please use a platform independent port identifier here to ensure that it is is compilable on all platforms.

Suggested change

#define BME680_NSS_PIN GPIO_PIN(PA, 5)

#define BME680_NSS_PIN GPIO_PIN(0, 5)

drivers/bme680/bme680.c

pkg/bme680_driver/include/bme680_hal.h

drivers/Makefile.dep

drivers/bme680/include/bme680_params.h

pkg/bme680_driver/contrib/bme680_hal.c

tests/driver_bme680/Makefile

gschorcht · 2019-11-16T11:16:04Z

tests/driver_bme680/main.c

+        ret = bme680_get_sensor_data(&data, &(dev.dev));
+        if(!ret) {
+#ifndef BME680_FLOAT_POINT_COMPENSATION
+        printf("[bme680]: T %d degC, P %ld hPa, H %ld ", data.temperature,


Please use platform independent format defines here:

Suggested change

printf("[bme680]: T %d degC, P %ld hPa, H %ld ", data.temperature,

printf("[bme680]: T %" PRIi16 " degC, P %" PRIu32 " hPa, H %" PRIu32, data.temperature,

Fixed, I also enforce display formatting. Tell me if it's wrong.

tests/driver_bme680/main.c

gschorcht · 2019-11-16T11:55:15Z

Tested with I2C and it works as expected 👍

pkg/bme680_driver/contrib/bme680_hal.c

herjulf · 2019-11-20T14:19:29Z

Many moons ago I did a implementation for Contiki
https://github.com/posjodin/contiki/tree/master/dev/bme680

IMO the Bosch lib was to bloated so I just rewrote from user manual code examples to keep the code compact, readable and with few dependencies. But I don't the implementation is very tested.

herjulf · 2019-11-20T14:58:45Z

Also I bit disappointing to see there still no mapping between the resistance and the indoor Air Quality. Bosch is to blame.
IMO Bosch should provide a mapping. Otherwise the device is not so useful.

tcschmidt · 2019-11-21T03:38:39Z

@MrKevinWeiss is this a poster child for SPI HIL testing?

MrKevinWeiss · 2019-11-21T08:26:42Z

I don't know if this is a good candidate as this is a driver. If there are problems with the CPU implementations of the SPI than the HiL could assist with debugging.

gschorcht · 2019-11-21T11:03:05Z

@herjulf I did exactly the same two years ago for ESP-IDF which could be easily ported to RIOT and probably would have done it already 😉

This PR is the result of the review of PR #9909 which provided a driver that based on the vendor code but had only required parts of the code. At that time, we decided to try to use the original vendor driver as a package, see also #10502.

If code size is your concern, it may be interesting to know what code size PR #9909 and what code size this PR has.

herjulf · 2019-11-21T13:11:35Z

@gschorcht . Thanks for this history and background and seems like we have similar experiences with drivers w/o Bosch API. Code size is of course interesting but my concerns is more about control and avoiding dependencies for long time maintainability. Anyway current work is now based on the Bosch library.

dylad · 2019-12-11T17:28:06Z

@gschorcht I pushed a couple of changes, especially the interface selection in the bme680_params.h file. Would you mind have another look please ?

gschorcht · 2020-01-10T10:00:44Z

pkg/bme680_driver/README.md

+In addition, this driver can use floating point if available on your MCU. By default, this package do not use it.
+
+## Usage
+Add `USEPKG += bme680_driver` in your Makefile and `#include "bme680_hal.h"` to your code. Refer to the code documentation for more information on the API.


Please remove USEPKG += bme680_driver from documentation. It isn't necessary to add it to the application's makefile.

Even more, if you move

#include "bme680_hal.h" #include "bme680_defs.h"

to bme680.h, the application has not to include them. Then it is enough to include bme680.h and bme680_params.h by the application. This makes the usage easier.

Suggested change

Add `USEPKG += bme680_driver` in your Makefile and `#include "bme680_hal.h"` to your code. Refer to the code documentation for more information on the API.

Refer to the code documentation for more information on the API.

drivers/include/bme680.h

gschorcht · 2020-01-10T10:14:25Z

Therefore, I would suggest to call the package driver_bme680

I am not against and I'll apply this change unless someone is opposed to such naming scheme.

What about this suggested change? I just want to have a naming that allows us to sort the large amount of different package types a bit. As the number of driver packages increases, they could all be called driver_*.

drivers/bme680/include/bme680_params.h

gschorcht · 2020-03-11T19:29:57Z

FYI. It works w. I2C BOARD=avr-rss2 with AtMega256RFR2. Also compiles with ENABLE_FP=1 but I'll assume float libs is needed for float printout. Capital O in ohms?

Thanks for testing. Sounds good.

gschorcht · 2020-03-11T19:30:25Z

We should get it merged in near future, there is already a conflict with master again 😟

May I rebase and squash?

roberthartung · 2020-03-12T06:51:27Z

We should get it merged in near future, there is already a conflict with master again

May I rebase and squash?

Yes, please do so. Will give the code a final review. Testing should be complete at this point. Thanks @janschlicht

roberthartung · 2020-03-12T07:02:19Z

It is now dynamic. If the value returned from the driver is greater than INT16_MAX, it uses kOhm. Otherwise it uses Ohm.

I still wrap my head around this. Is it safe to just switch from ohm to kilo ohm? Can there be a double meaning of a value?

gschorcht · 2020-03-12T07:27:48Z

It is now dynamic. If the value returned from the driver is greater than INT16_MAX, it uses kOhm. Otherwise it uses Ohm.

I still wrap my head around this. Is it safe to just switch from ohm to kilo ohm? Can there be a double meaning of a value?

Hm, there is no other chance. Since the phydat module uses int16_t, there no other way than to scale down a value greater than +32,767 for SAUL. That's why the the value is divided by 1000 and the scale is set to 3 in that case. The phydat module then automatically converts the unit string from ohm to kohm as it does for other units. Even though the official unit would be kiloohm or also kilo ohm, that the way it is handled by the phydat module.

Add the vendor driver implementation from Bosch Sensortech as a package.

roberthartung · 2020-03-12T07:42:42Z

It is now dynamic. If the value returned from the driver is greater than INT16_MAX, it uses kOhm. Otherwise it uses Ohm.

I still wrap my head around this. Is it safe to just switch from ohm to kilo ohm? Can there be a double meaning of a value?

Hm, there is no other chance. Since the phydat module uses int16_t, there no other way than to scale down a value greater than +32,767 for SAUL. That's why the the value is divided by 1000 and the scale is set to 3 in that case. The phydat module then automatically converts the unit string from ohm to kohm as it does for other units. Even though the official unit would be kiloohm or also kilo ohm, that the way it is handled by the phydat module.

Oh I see now. So you still know if you read the value, if it is in ohms or kilo ohms, you just have to look at the scale then. Nevermind my comment then :)

Add the support for BME680 based on the BME680 vendor driver package.

roberthartung

@janschlicht and I tested some more cases. I2C, SPI, SPI + I2C and two I2C devices -> works like expected!

ACK from my side.

dylad · 2020-03-12T09:09:58Z

@janschlicht and I tested some more cases. I2C, SPI, SPI + I2C and two I2C devices -> works like expected!

Wow that's nice !

roberthartung · 2020-03-12T09:25:01Z

@janschlicht and I tested some more cases. I2C, SPI, SPI + I2C and two I2C devices -> works like expected!

Wow that's nice !

Yes! Even though adding the second set of I2C Parameters wasn't too easy. But I don't see a neat way of changing it. I suggest leaving the driver as is for now, until (when/if) this becomes an issue!

roberthartung · 2020-03-12T09:25:38Z

@gschorcht I think some of your requested changes prevent merge?

roberthartung · 2020-03-12T12:05:04Z

@MichelRottleuthner @gschorcht @PeterKietzmann We can merge?!

MichelRottleuthner · 2020-03-12T12:09:32Z

@roberthartung yes sure go ahead!

dylad · 2020-03-12T13:05:47Z

Awesome ! Thanks for merging @roberthartung
and thanks for tackling this @gschorcht !

gschorcht · 2020-03-12T13:11:58Z

Thank you all for contributing, reviewing, testing and for merging.

The merge of this PR is an important milestone, as it is a good example of how a vendor driver can be used package. There are other PRs that do this in the same way, for example #10363. With our Arduino compatibility layer in sys/arduino it even becomes possible to reuse Arduino driver libraries.

maribu · 2020-03-24T14:03:25Z

drivers/bme680/bme680_saul.c

+            data->val[0] = _gas[dev_index];
+            data->scale = 0;
+        }
+        data->unit = UNIT_OHM;


Yep, the gas measurement is returned as the resistance of the gas sensitive layer. What ever it means. Unfortunately, the datasheet gives no explanation or hints on how the value has to be interpreted 😟

They just recommend to use the BSEC to get IAQ.

I have the import of the BSEC as package on my ToDo list.

@boschsensortec: Care to help out here? Could you give a formula how to convert that value into something useful?

The biggest chalange will be that each user of the BSCE has to accept the license before it is downloadable.

dylad 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: pkg Area: External package ports Area: drivers Area: Device drivers labels Nov 15, 2019

dylad assigned maribu and gschorcht Nov 15, 2019

dylad mentioned this pull request Nov 15, 2019

pkg/bme680 driver: add support for I2C/SPI BME680 driver #10502

Closed

gschorcht requested changes Nov 15, 2019

View reviewed changes

tests/driver_bme680/tests/01-run.py Show resolved Hide resolved

gschorcht requested changes Nov 15, 2019

View reviewed changes

drivers/Makefile.dep Show resolved Hide resolved

gschorcht requested changes Nov 15, 2019

View reviewed changes

drivers/Makefile.include Outdated Show resolved Hide resolved

dylad added this to the Release 2020.01 milestone Nov 15, 2019

gschorcht reviewed Nov 16, 2019

View reviewed changes

gschorcht requested changes Nov 16, 2019

View reviewed changes

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

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

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

gschorcht reviewed Nov 16, 2019

View reviewed changes

pkg/bme680_driver/contrib/bme680_hal.c Outdated Show resolved Hide resolved

herjulf mentioned this pull request Nov 19, 2019

Adding: boards/avr-rss2 and AtMega256RFR2 based board See doc.txt for details. #12668

Merged

gschorcht requested changes Jan 10, 2020

View reviewed changes

drivers/bme680/include/bme680_params.h Show resolved Hide resolved

pkg: add Bosch Sensortech BME680 driver

90fb789

Add the vendor driver implementation from Bosch Sensortech as a package.

gschorcht added 6 commits March 12, 2020 08:42

drivers: add support for BME680 driver

9dd11eb

Add the support for BME680 based on the BME680 vendor driver package.

tests: add test application for BME680 driver

2338944

sys/phydat: add ohm as unit

558322b

drivers/saul: add common gas class

1118f80

drivers/bme680: add SAUL capabilities

c4325e4

drivers/saul: add auto_init for BME680

a1e2f36

gschorcht force-pushed the pr/bme680_pkg_driver branch from d367276 to a1e2f36 Compare March 12, 2020 07:52

roberthartung approved these changes Mar 12, 2020

View reviewed changes

gschorcht removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Mar 12, 2020

gschorcht approved these changes Mar 12, 2020

View reviewed changes

gschorcht 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 12, 2020

roberthartung merged commit dc6665d into RIOT-OS:master Mar 12, 2020

dylad deleted the pr/bme680_pkg_driver branch March 12, 2020 13:05

maribu reviewed Mar 24, 2020

View reviewed changes

gschorcht mentioned this pull request Jan 11, 2021

drivers/bme680: replace pkg with module #15738

Closed

	#define BME680_NSS_PIN GPIO_PIN(PA, 5)
	#define BME680_NSS_PIN GPIO_PIN(0, 5)

	printf("[bme680]: T %d degC, P %ld hPa, H %ld ", data.temperature,
	printf("[bme680]: T %" PRIi16 " degC, P %" PRIu32 " hPa, H %" PRIu32, data.temperature,

	Add `USEPKG += bme680_driver` in your Makefile and `#include "bme680_hal.h"` to your code. Refer to the code documentation for more information on the API.
	Refer to the code documentation for more information on the API.

driver/bme680: add I2C/SPI driver for BME680 device #12717

driver/bme680: add I2C/SPI driver for BME680 device #12717

Conversation

dylad commented Nov 15, 2019 • edited by gschorcht Loading

Contribution description

Testing procedure

Issues/PRs references

gschorcht commented Nov 15, 2019

gschorcht commented Nov 15, 2019

dylad commented Nov 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gschorcht commented Nov 16, 2019

herjulf commented Nov 20, 2019

herjulf commented Nov 20, 2019

tcschmidt commented Nov 21, 2019

MrKevinWeiss commented Nov 21, 2019

gschorcht commented Nov 21, 2019 • edited Loading

herjulf commented Nov 21, 2019

dylad commented Dec 11, 2019

Choose a reason for hiding this comment

gschorcht commented Jan 10, 2020

gschorcht commented Mar 11, 2020 • edited Loading

gschorcht commented Mar 11, 2020

roberthartung commented Mar 12, 2020

roberthartung commented Mar 12, 2020 • edited Loading

gschorcht commented Mar 12, 2020

roberthartung commented Mar 12, 2020

roberthartung left a comment

Choose a reason for hiding this comment

dylad commented Mar 12, 2020

roberthartung commented Mar 12, 2020

roberthartung commented Mar 12, 2020

roberthartung commented Mar 12, 2020

MichelRottleuthner commented Mar 12, 2020

dylad commented Mar 12, 2020

gschorcht commented Mar 12, 2020

Choose a reason for hiding this comment

gschorcht Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maribu Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylad commented Nov 15, 2019 •

edited by gschorcht

Loading

gschorcht commented Nov 21, 2019 •

edited

Loading

gschorcht commented Mar 11, 2020 •

edited

Loading

roberthartung commented Mar 12, 2020 •

edited

Loading

gschorcht Mar 24, 2020 •

edited

Loading

maribu Mar 24, 2020 •

edited

Loading