node_modules/bme280.lua broken

[stromnet]

Expected behavior

bme280 LUA module bme280_startreadout providing valid readout values in callback

Actual behavior

bme280 LUA module bme280_startreadout provides zero values in callback

NodeMCU startup banner

NodeMCU 3.0.0.0 built with Docker provided by frightanic.com
	branch: release
	commit: 4f6792773f93f36a7255cfd28dca7aa6c4aa9552
	release: 3.0-release_20201107
	release DTS: 202011071523
	SSL: false
	build type: float
	LFS: 0x20000 bytes total capacity
	modules: adc,bit,bme280_math,crypto,dht,file,gpio,i2c,mqtt,net,node,ow,sjson,spi,tmr,uart,wifi
 build 2020-11-07 21:24 powered by Lua 5.1.4 on SDK 3.0.1-dev(fce080e)

Hardware

ESP12F + BME280

Problematic code

In https://github.com/nodemcu/nodemcu-firmware/blob/release/lua_modules/bme280/bme280.lua#L133 there is an attempt to avoid using bit operations. However, this produces invalid data (at least in my setup).

I'm running in 1x oversampling on everything, no IIR, and using forced mode.
That gives the value for _config[3]= 0b00100101 = 0x25.
Applying math_floor(self._config[3]:byte(1)/4)+ 1 on that yields (0x25 / 4) + 1 = 9 + 1 = 10 = 0x0a =0b00001010
This means that Temperature sampling is turned of, and when the bme280_math.read is trigged, the first thing it does if there is no temperature is to return nil.

Changing the line to instead use bit.band(self._config[3]:byte(1), 0xFC) + 1 resolve the problem and the sensor starts working.

I might be missing something but I cannot see how this would work at all :)

[stromnet]

Forgot to ping relevant tickets: #3132 #3126

[nwf]

That... yes, that seems like a bug. If it's important to avoid the use of bit (I'm not entirely convinced) then it looks like it should have been 4 * math_floor(self._config[3]:byte(1)/4) + 1.

Please do open a PR.

[vsky279]

I vote for correcting the bug via 4 * math_floor(self._config[3]:byte(1)/4) + 1. Anyway that was the intention.
If the module was to be used solely under Lua 5.3, I would use directly bitwise operators. But as it is also intended for Lua 5.1 I used this workaround not to introduce dependency on the bit module.

[vsky279]

It seems to me there are several lines of Lua code that it would be nice to review once Lua 5.1 is abandoned.
It would be nice to start marking these lines with some common comment so it can be easily identified later on.

This formula where the bug was being a nice example:

4*math_floor(self._config[3]:byte(1)/4)+ 1

can be rewritten to simple

(self._config[3]:byte(1) & 0xFC) | 1

[HHHartmann]

It would be nice to start marking these lines with some common comment so it can be easily identified later on.

I thought about that too. How about a comment
-- TODO LUA51
or just
-- LUA51
maybe have the Lua 5.3 code around as comment too if possible.

[vsky279]

I like the -- LUA51. I'm gathering some minor changes into 1 commit. Once it grows sufficiently I will submit a PR.

The discussed part of the code will look this way:

  write_reg(self.id, self.addr, BME280_REGISTER_CONTROL, 4*math_floor(self._config[3]:byte(1)/4)+ 1) -- LUA51
    -- 4*math_floor(self._config[3]:byte(1)/4)+ 1
    --   an awful way to avoid bit operations but calculate (config[3] & 0xFC) | BME280_FORCED_MODE
    -- Lua 5.3: write_reg(self.id, self.addr, BME280_REGISTER_CONTROL, (self._config[3]:byte(1) & 0xFC) | 1)

I think we can close this issue.

[nwf]

Closing at request of @vsky279.