drivers/bme680: replace pkg with module

[Jnae]

Contribution description

• added a RIOT driver for the BME680 sensor, which does not rely on the Bosch implementation any longer
• fixed SPI communication

Testing procedure

• the test tests/driver_bme680 can be run as before with BOARD=... DRIVER=bme680_i2c make flash term or BOARD=... DRIVER=bme680_spi make flash term

[gschorcht]

The motiviation for this PR is not really clear for me.

In the discussion in PR #9909 it was decided to use the vendor driver as a package instead of implementing a new one from scratch as provided in PR #9909 by @dylad. This decision was also made because the driver is not so easy to implement without the deep insight knowledge of Bosch. At that time, I also offered to provide my BME680 driver that I have developed three years ago for esp-open-rtos and ESP-IDF (https://github.com/gschorcht/bme680-esp-idf).

This was discussed again in the follow-up PR #12717. But the decision remained the same, also because the plan was to also import the BSEC as a package to be able to obtain IAQ values instead of resistance values for gas quality.

Could you please explain in more detail what was the motivation for the new development, what are the advantages and disadvantages and why we should use this driver instead of the vendor driver?

[maribu]

I'm in favor of an internal driver rather than using a package.

pros:

• lower code size and complexity due to one abstraction layer less. (Our SPI and I2C abstraction layers are very fine, no need to do impedance matching between this abstraction layer and the bosh abstractionlayer)
• code quality. (IMO the Bosh code quality sucks.)
• more control over the driver
  • we are not limited to the design decision done by Bosch

cons:

• maintainability
  • but device driver are among the most stable code part in RIOT
• makes using the BSEC as a package to convert resistance to IAQ values for air quality more difficult
  • but I doubt we would get this package anyway, due to the license agreement they ask before you can download the cod
  • IMO we should avoid opening this can of worms. When Bosch asks their users to explicitly agree before offering a download and we work around this, we might get into legal trouble.

Generally speaking, IMO device drivers are usually better implemented as RIOT modules rather than packages.

[maribu]

There are some white space errors (missing \n at the end of the file, white space at end of lines). The CI is pointing them out.

Also: The SAUL implementation is very similar to the original one. It is better to keep the original header (especially the author and copyright section).

[fjmolinas]

Could you please explain in more detail what was the motivation for the new development, what are the advantages and disadvantages and why we should use this driver instead of the vendor driver?

I would also like to hear the motivations.

I'm in favor of an internal driver rather than using a package.

I agree with the pros/cons exposed by @maribu, but here I think maintainablity is the best pro. Are there important advantages from this implementation on the technical side (footprint, speed, etc.. )?

[Jnae]

I think the main motivations are well summarised by @maribu; the goal was to build a driver that reduces the driver's complexity by taking the abstraction layer from it. In my opinion it should be easier to maintain as well.

The size of the new module would be another advantage. Compiling 'tests/driver_bme680' reduces the RAM requirement from 2.54 KiB to 2.45 KiB. (The Flash size increases a bit (17.36 KiB to 18.19 KiB))
The speed should be similar.

[gschorcht]

Other cons I see:

1. The bme680_read function is now a blocking function. The former approach was
   • to trigger the measurement,
   • to let the driver determine the duration of the measurement and
   • then read the data after this duration.
     There were good reasons for this. The duration of a measurement cycle can be from a few milliseconds to several seconds, depending on the heating duration, the measurements used, and the heating profile used.
2. Even though the wrapper driver in RIOT didn't have a declared function to set the heating profile, it was possible to declare the function of BOSCH's driver as external and use it to set the heating profile. For the measurement of gas quality it is important to be able to define several heating profiles.That is, this new driver does not allow to use the complete functionality of the sensor, while the existing driver does, even if it is a bit complicated.

[maribu]

I think it would be helpful to split the discussion into two topics:

1. Should we allow replacing drives implemented as package with custom drivers?
2. The technical discussion of the implementations

Right now this PR has not yet received technical review. (Not even internal review, as we decided for transparency to do provide review publicly via the normal PR process.) But this technical is the very process which results in technically improving a PR. However, it is strongly demotivating for me to review this PR and I suppose it is demotivating for @Jnae to improve this PR when there seems to be fundamental non-technical opposition against replacing packages with internal drivers.

So, let's please for now wait with the technical review and first reach an agreement on whether a custom driver is acceptable when a pkg with the same features is already present. If, and only if, we agree that this is the case, it makes IMO sense to engage in the technical review. And then we can do a fair technical comparison once this PR has matured due to the review process.

Jumping in the non-technical discussion: IMHO, we shouldn't favor packages over modules just because we don't have to maintain external code. First, we have a lot of code in RIOT that provides the same features that external code could provide. But still, we have a custom network stack, a custom scheduler, custom IPC mechanism, etc. Those were created because we choose to implement things differently, approach problems differently, and do different trade-offs. Second, packages also need to be maintained. If you look at the recently removed emb6 network stack - the effort to maintain that pkg compared to our GNRC would have certainly be less, but still emb6 remained unmaintained and was dropped for that reason while GNRC is well maintained. So IMO it is not only about the effort to maintain, but also (and even more importantly) about whether we have people willing to do the work.

We (ComSys) have decided on the BME680 sensor for our testbed nodes. Hence, we have interest in having the best possible support of the device in RIOT. I currently cannot see any legal path forward for friction-less support of gas quality measurement based on Bosch's code due to license issues. In the end, I see no alternative to providing a custom implementation of that via reverse engineering the underlying conversion function of the proprietary Bosch library. But that would best be integrated into a single driver module.

Finally, I don't see that the package and the module are mutually exclusive. While there is interest in having both, we could have the pkg and add a bme680 driver as module, if and when our usual technical review process concludes the PR is ready. If interest in or maintenance of one of the driver implementation ceases, we could simply drop it in favor of the other.

[gschorcht]

In general, using vendor or other drivers as a package and integrating them into RIOT through a simple wrapper interface has the advantage that thousands of existing drivers can be integrated into RIOT very easily. Such an approach was used in PR #10363 the first time. Unfortunately, this PR is waiting for review for over two years 😟 The general approach and the standardized wrapper interface were discussed in issue #10506. The BME680 driver as provided in PR #10502 and PR #12717 followed this approach. The ATWINC15x0 driver provided in PR #13754 also uses this approach.

Regarding the BME680 driver, I'm not strictly against replacing the vendor driver with a native one if you strongly recommend it, f
for example, because the BSEC integration will never be possible anyway due to licensing problems and the BOSCH code is of poor quality, which I fully agree with. Of course, the native driver should at least have no disadvantages.

One of my concerns is that the verdor driver at least theoretically gives the users the ability to integrate the BSEC if they agree to the license terms. This will not be possible with the native driver. However, without the BSEC and the ability to convert gas resistance values to IAQ indices, the BME680 is nothing more than a simple temperature, humidity and pressure sensor. Its most important function remains unused.

Regarding the code. It would be good if it could be improved a bit before a review. The code has a lot of code style violations that should be fixed before.

[maribu]

OK, let my try to summarize / conclude:

1. At least until a native driver with support for proper gas quality measurement is merged, the pkg driver has the benefit to allow users to hook in the BSEC library if they agree with the license. Hence, it should remain at least until then.
2. If and when it technically makes sense to merge this PR, it can in principle be merged. (But it should not replace the existing driver, but rather be an alternative option.)
3. I start to review now and ping @gschorcht, @dylad and @fjmolinas once I believe that this PR is in a good shape and offers technical benefits, so that you can join the review and the technical decision on merging can be made.

Is that correct?

[fjmolinas]

Is that correct?

Sounds good to me :)

[maribu]

The code looks generally quite good for a first PR. Please have a look at the "files changed" tab in the github view - the CI now annotates the code with automatically detected style issues. Hence, I didn't comment on those further.

Also: The module name, file names and function names need to be changed to avoid conflicts with the current package, as this shouldn't replace the existing driver. (This would also allow to use both at the same time, which can ease testing quite a bit.)

In one of my first comments I suggested to change the architecture of the module by splitting it into bme680_common, bme680_i2c and bme680_spi - effectively providing two drivers (but with all code but the communication shared via the bme680_common module). This would also directly solve the name conflicts, as the functions would be bme680_i2c_init() and bme680_i2c_read() (and same for SPI).

[maribu]

Suggested change

	#define ENABLE_DEBUG 1
	#define ENABLE_DEBUG 0

[maribu]

Suggested change

	#ifdef MODULE_BME680_I2C
	#if IS_USED(MODULE_BME680_I2C)

[maribu]

I personally prefer a different approach than used here (and also in bmx280, at86rf2xx, and the previous package): How about providing three modules instead, e.g. bme680_common, bme680_i2c and bme680_spi?

The bme680_common would contain a libraries that converts raw register values to physical data and does not care about how the contents of the registers are obtained. It would also provide an bme680_common_t structure that contains e.g. the calibration data.

The bme680_i2c module would contain the code to communicate with an BME680 via I2C. It would provide an bme680_i2c_t that contains an bme680_common_t as member. It will call into functions provided by bme680_common for anything that is not related to I2C communication with the sensor.

And bme680_spi would be the same as bme680_i2c, but for SPI.

Pros:

• Better structure
  • code for communication and processing are clearly organized and I2C and SPI no longer mixed up
• Most of the preprocessor conditionals can be removed
  • All the #if IS_ACTIVE(BME680_SPI_MODE) ... #else ... #endif handling is no longer needed
• User can use both SPI and I2C connected devices, if they want.
  • For some monitoring applications it is highly beneficial to trigger measurements (almost) simultaneously, so that measurements for different locations taken at (almost) the same point in time. By using a dedicated bus for each device and DMA, it should be possible to trigger a measurement on each device almost simultaneously. For such a setup it can make sense to mix I2C and SPI to make use of all interfaces a board has
  • Generally speaking, I think it is the job of an OS to enable users doing things, not preventing them. If they want to use SPI and I2C attached at the same time, this should be fine - regardless of how much sense this seems to make to us. So unless there are technical reasons to prevent simulations use of both interface, we should IMO try to enable this.

Cons:

• Duplication of the header files (we now need bme680_i2c.h and bme680_spi.h and two sets of public APIs, e.g. bme680_spi_read() and bme680_i2c_read()
  • But the implementations are not duplicated, as the common stuff is moved to bme680_common.

[maribu]

Suggested change

	int bme680_init(bme680_t* dev, const bme680_params_t* params)
	int bme680_init(bme680_t *dev, const bme680_params_t *params)

It is better to have the * next to the variable. E.g. in this example:

int *a, b, *c; /* here it is intuitive that "a" and "c" are "int *", and b is "int" */
int* d, e; /* here intuitively both are of "int*", but e actually is "int" */

[maribu]

Suggested change

xtimer_init();

xtimer_init() is called during auto-initialization. It should never be initialized by drivers.

[maribu]

It increases readability, if those are better aligned. E.g. check the bme680_params.h of the current driver for an example.

[maribu]

Please also add "Mesotic SAS" and "Gunar Schorcht" here, as it seems at least parts of the code are inspired by the current driver. Same for the @author fields.

[maribu]

Suggested change

	uint8_t ambient_temp;
	uint8_t temp_os;
	uint8_t press_os;
	uint8_t hum_os;
	bool meas_gas;
	uint16_t gas_heating_time;
	uint16_t gas_heating_temp;
	uint8_t filter;
	bme680_intf_t intf;
	bme680_intf_t intf; /**< please document me */
	uint16_t gas_heating_time; /**< and me */
	uint16_t gas_heating_temp; /**< and me */
	uint8_t ambient_temp; /**< and me */
	uint8_t temp_os; /**< and me */
	uint8_t press_os; /**< and me */
	uint8_t hum_os; /**< and me */
	uint8_t filter; /**< and me */
	bool meas_gas; /**< and me */

Please add documentation to the members. (And also please align the documentation.)

In addition, it is useful to spent some thought into the memory layout of a structure. E.g. consider this code:

#include <stdint.h>
#include <stdio.h>

struct a {
    uint8_t m1;
    uint32_t m2;
    uint8_t m3;
    uint32_t m4;
    uint16_t m5;
};

struct b {
    uint32_t m2;
    uint32_t m4;
    uint16_t m5;
    uint8_t m1;
    uint8_t m3;
};

int main(void)
{
    printf("sizeof(struct a) = %u, sizeof(struct b) = %u\n", (unsigned)sizeof(struct a), (unsigned)sizeof(struct b));
    return 0;
}

On most 32-bit and 64-bit machines this will print sizeof(struct a) = 20, sizeof(struct b) = 12. So struct a - despite having the exact same members as struct b - is 8 bytes larger. This is because the compiler will add padding before the struct members, if the alignment of the member is not fulfilled. A simple algorithm to yield the most efficient memory use is to order members by the size of their type. But this is not the only way to get the lowest number of padding bytes needed. Feel free to order struct members as you like, but it would be good if you could avoid padding whenever possible.

For more details, this is a good read: https://hownot2code.com/2016/11/10/the-lost-art-of-c-structure-packing/

[maribu]

OK, here I agree that no meaningful documentation is possible for the members. But again, due to padding memory is wasted.

[maribu]

This should be dropped in favor of using negative errno constants.

[gschorcht]

What was the reason for removing the python test script, it should also work with the new driver?

[Jnae]

@maribu thank you for this constructive review! I will change it accordingly.

[Jnae]

As suggested bei @maribu, I now split the module into two seperate ones called bme680_i2c and bme680_spi

[maribu]

Some more comments inline. Most of them regarding the formulas taken from the Bosch code. (I think we can restructure / rearrange the formula to remain identical, but become readable.)

Thanks for the rework! :-)

[maribu]

As the current driver needs to remain, this file should be left unmodified

[maribu]

Again, this should be left unmodified

[maribu]

This file should remain, to keep the current driver

[maribu]

Suggested change

	var1 = (((int32_t) (dev->params.ambient_temp) * dev->calib.par_g3) / 1000) * 256;
	var1 = (((int32_t)(dev->params.ambient_temp) * dev->calib.par_g3) / 1000) * 256;

[maribu]

Suggested change

	var2 = (dev->calib.par_g1 + 784) * (((((dev->calib.par_g2 + 154009) * (dev->params.gas_heating_temp) * 5) / 100) + 3276800) / 10);
	var2 = (dev->calib.par_g2 + 154009) * dev->params.gas_heating_temp * 5;
	var2 = (var2 / 100 + 3276800) / 10;
	var2 *= dev->calib.par_g1 + 784;

This line is a bit long (> 100 chars) and hard to read. Maybe this can just be split, e.g. as above. (But please double check that this is still the exact same formula.)

[maribu]

Suggested change

	void calc_gas(const bme680_common_t *dev, uint32_t *res, uint8_t gas_range, uint16_t gas_adc)
	static uint32_t _calc_gas(const bme680_common_t *dev, uint8_t gas_range, uint16_t gas_adc)

[maribu]

Suggested change

	var1 = (int64_t)(((1340 + (5 * (int64_t) dev->calib.range_sw_error)) * ((int64_t)const_array1_int[gas_range])) >> 16);
	var1 = ((1340 + (5 * (int64_t)dev->calib.range_sw_error)) * const_array1_int[gas_range])) >> 16;

[maribu]

Note that (int64_t)(gas_adc << 15) and ((int64_t)gas_adc << 15) have different results: In the former, 15 of 16 bits are lost, in the second, all bits are taken into account. Can you confirm that this formula is correct with losing 15 bits?

[maribu]

Suggested change

	gas_res = (int32_t)((((int64_t)(const_array2_int[gas_range] * (int64_t)var1) >> 9) + (var2 >> 1)) / var2);
	gas_res = (((int64_t)(const_array2_int[gas_range] * var1) >> 9) + (var2 >> 1)) / var2;

[maribu]

Suggested change

	*res = gas_res;
	return gas_res;

[gschorcht]

The following questions are still not discussed/answered:

The bme680_read function is now a blocking function. The former approach was

• to trigger the measurement,
• to let the driver determine the duration of the measurement and
• then read the data after this duration.

There were good reasons for this approach. The duration of a measurement cycle can vary from a few milliseconds to several seconds, depending on the heating duration, the measurements used, and the heating profile used.

What was the reason for removing the python test script, it should also work with the new driver?

[maribu]

The bme680_read function is now a blocking function. The former approach was

* to trigger the measurement,

* to let the driver determine the duration of the measurement and

* then read the data after this duration.

IMO that is a pretty ugly API. Neither driver does support setting the heating profile or the number of measurements used, so right now there is no need for an async API. Once those are added, the blocking API remains useful: Even when a measurement takes seconds, there are use cases in which the blocking API is easier to work with. Hence, a non-blockig API should IMO be added only in addition, but not replace the blocking API.

Something like this would make more sense IMO:

typedef void (*bme680_spi_data_ready_cb_t)(bme680_spi_t *dev);

int bme680_spi_start_measurement(bme680_spi_t *dev, bme680_spi_data_ready_cb_t callback);

Here, an software timer is set and callback is called in IRQ context when the data is ready. The advantage of this is that the blocking API can be trivially be implemented on top, by unlocking a temporary mutex with the callback on which the caller of the blocking API waits.

For full convenience, additionally the following could be added:

typedef void (*bme680_spi_data_cb_t)(bme680_spi_t *dev, bme680_data_t *result);

int bme680_spi_read_async(bme680_spi_t *dev, bme680_data_cb_t callback);

Here, an event handler thread is used to fetch the result and the callback is called in the context of the thread handler, directly with the result of the measurement supplied. This could be a submodule, as this would depend on USEMODULE += event_thread. The implementation could reuse bme680_spi_start_measurement().

What was the reason for removing the python test script, it should also work with the new driver?

@Jnae: Please add the test script also to the new driver. Note that the test should be in tests-with-config, as only tests that do not require additional hardware and/or additional configuration (the values for the foo_params_t) should be in a tests folder. (E.g. do a rebase on top of upstream master, as there the renaming has already happened.)

[gschorcht]

Is there still some work in progress for this PR? There are a lot of change requests but no code changes since almost a year.

[maribu]

@Jnae was a student assistant of us, but she is now working on her thesis instead. She has a branch with has more progress that I promised to take a look at but didn't manage to do so far. I will eventually pick this up and carry on from there. We still have interest in a more integrated driver as a module, but since feature-wise there is nothing to gain compared to the pkg, this is not really high priority.

I was also considering to make a thesis topic of reverse engineering the formula for the air quality out of the binary driver (legal, clean room reverse engineering without ever looking at proprietary source code or decompiling - just by running the binary over a lot of fake input and fitting a function to match the output as closely as possible), so that the module could gain support for that.

But as said, this is not pressing to us in any way, so progress will be slow.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.