Rewrite bme280 driver in Lua+C

[nwf]

In an effort to decrease the project's technical debt, I've proposed (in #3124) that the C-only bme280 be cut into a Lua layer that drives communication and a C module that does any requisite low-level fiddling. This will be similar to #2819 and may, incidentally, address #2527 correctly as well. (As you can see, between that and #2241, this module has a history of being sufficiently troublesome that issues have been worked around rather than fixed.) This rewrite will also eliminate yet another source of static allocation and artificial limitation in our driver layer. All in all, deeply worth it.

I would like it very much if the result of this rewrite included a test driver, as well. Ideally bme280 becomes an example of how all drivers should look. No pressure (ha!). ;)

OK, so, what does this entail, concretely?

Looking at the existing bme280.c, we almost immediately see that there's a bunch of goo associated with performing i2c transactions: bme280_i2c_addr, r8u_n, r8u, and w8u. These can all be directly transcoded to Lua. The results from the i2c package will be Lua strings.

The bme280_data structure holds per-device state obtained during setup, which means we probably want to hold it instead in a Lua userdata object that represents the sensor. Reading setup, it looks like this takes 3 or 4 I2C transactions to set up, including getting the chip ID, so I think our sensor object constructor should take 2 or 3 Lua strings (DIG_T, DIG_P, and DIG_H) to build itself. Since it appears that the static values bme280_t_fine, bme280_h, and bme280_hc may outlive a particular measurement, they should perhaps join the bme280_data in the per-device userdata object.

Armed with the sensor parameters, we can look at interacting with the device: bme280_lua_startreadout does some i2c transactions, easily done instead in Lua, and schedules a timer, also easily done in Lua. (Can the device not raise IRQs?)

Once it's time to read data, bme280_lua_read does some more i2c transactions and a lot of math, including storing "fine" constants between procedure calls. To continue the theme, the transactions should be done in Lua, and passed to a C routine that takes the sensor userdata object and does the requisite math. For my $0.02, the results should just land in a Lua table.

The abbreviated bme280_lua_temp, bme280_lua_baro, and bme280_lua_humi functions are readily achieved by having the Lua replacement for bme280_lua_read perform fewer transactions and call into the C function with fewer string arguments. That function should return a table with fewer keys as a result.

For the purposes of PRing, I'd suggest placing code in

• app/modules/bme280_math.c for the C layer
• lua_modules/bme280/bme280.lua for the Lua top-level and main driver
• lua_modules/bme280/bme280_test.lua for the test script (ideally in mispec, I think? See Create unit tests in Lua #2145).

[vsky279]

Being original contributor of the BME208 driver I'm registering myself in the discussion. I need to re-think your suggestions to fully understand the logic behind.
If @heikomat is not going to implement it now I think I will take the challenge.

[heikomat]

@vsky279 I'm definitely interested, but as stated in the PR, somewhat short on time. Don't let my curiosity stop you from going for it! :D

[vsky279]

@nwf Nathaniel, I'm trying to figure out more in detail the way the new driver should be designed. So I try to reformulate the workflow:

1. [bme280.lua] Contacts sensor and reads its setup registers (BME280_REGISTER_DIG_T, BME280_REGISTER_DIG_P, BME280_REGISTER_DIG_H1, BME280_REGISTER_DIG_H2). Collects a string of responses passed as parameter to step 2.
2. [bme280_math.c: create(configuration parameters)] Creates UD representing setup state of the sensor. Returns the UD and string (or several strings - one string = one register?) to configure the sensor.
3. [bme280.lua] Configures sensor by writing registers BME280_REGISTER_CONFIG, BME280_REGISTER_CONTROL_HUM, BME280_REGISTER_CONTROL
4. [bme280.lua] Performs readout by reading register BME280_REGISTER_PRESS. Result is stored in a string passed to next step.
5. [bme280_math.c: decode(UD, readout string)] Does all the calculations and returns Lua table {temp, air pressure, relative humidity, QNH}.
   Or is it preferred to return table indexed by name? (this seems to me a bit inefficient)
   Why not to return multiple results as it is in existing bme280.read()?

bme280_math.c should also provide functions qfe2qnh, altitude and dewpoint.

Existing functions bme280.baro(), bme280.humi() and bme280.temp() are legacy and do not have to be retained when the driver is to be completely rewritten (or can be easily implemented in bme280.lua).

Forced readout mode should be implemented in bme280.lua as no further support from bme280_math.c is neede (I think).

Please comment. This does not seem to difficult to be implemented...

[nwf]

@nwf Nathaniel, I'm trying to figure out more in detail the way the new driver should be designed. So I try to reformulate the workflow:

Excellent! Story-boarding is a very, very good idea.

1. [bme280.lua] Contacts sensor and reads its setup registers (BME280_REGISTER_DIG_T, BME280_REGISTER_DIG_P, BME280_REGISTER_DIG_H1, BME280_REGISTER_DIG_H2). Collects a string of responses passed as parameter to step 2.
2. [bme280_math.c: create(configuration parameters)] Creates UD representing setup state of the sensor. Returns the UD and string (or several strings - one string = one register?) to configure the sensor.

Sounds good. I'd suggest "1 string : 1 transaction" -- if there are bytes that get sent to registers 0x10 through 0x13 and then later 0x18 through 0x20, that sort of feels like two strings to me. But this is a judgement call.

3. [bme280.lua] Configures sensor by writing registers BME280_REGISTER_CONFIG, BME280_REGISTER_CONTROL_HUM, BME280_REGISTER_CONTROL

If these are contiguous, one "configuration transaction" may be possible, with one string back from C. if not, maybe three. But I think this is a great general flow: Lua acting as a message-passing layer between the C driver core and the device itself.

4. [bme280.lua] Performs readout by reading register BME280_REGISTER_PRESS. Result is stored in a string passed to next step.

Sounds great and is the reverse direction of message passing from step 3. :)

5. [bme280_math.c: decode(UD, readout string)] Does all the calculations and returns Lua table {temp, air pressure, relative humidity, QNH}.
   Or is it preferred to return table indexed by name? (this seems to me a bit inefficient)
   Why not to return multiple results as it is in existing bme280.read()?

Multiple returns also probably works fine, so long as it's not like... dozens of them. Four is probably fine.

bme280_math.c should also provide functions qfe2qnh, altitude and dewpoint.

Additional utility functions seem perfectly reasonable; these, presumably, take the UD and a readout string as well or do they need something more or less ...?

Existing functions bme280.baro(), bme280.humi() and bme280.temp() are legacy and do not have to be retained when the driver is to be completely rewritten (or can be easily implemented in bme280.lua).

Agreed.

Forced readout mode should be implemented in bme280.lua as no further support from bme280_math.c is neede (I think).

I believe that's correct as well; a tmr object can be controlled from Lua.

Please comment. This does not seem to difficult to be implemented...

I hope it is not difficult, yes! But it is, nevertheless, something of an experiment in a new (hopefully more developer-approachable and -maintainable!) architecture for NodeMCU drivers. And it will probably cause us to shake things throughout the stack, including our image builders (see #3128 for broader discussion).

[stromnet]

While trying to figure out what was going on in #3132, I noticed that the bme280 lua module defines the read_reg and write_reg functions.
As these are veery common i2c patterns, it might make sense to create some generic i2c lua module (or even make them in C, if that yields smaller firmware size = more space for user code), so that these can be shared?

[nwf]

I think we would gladly take a i2c_helpers.lua module or an expansion of the i2c module API which added the utility functions for a single read-only transaction and a write/repeated-start/read transaction pair.

[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. Thank you for your contributions.