BME280: Wrong humidity calibration value

AmedeeBulle commented 3 months ago

The problem

As documented in the BME280 datasheet, the h4/h5 humidity calibration values are 12 bits signed integers.

https://github.com/esphome/esphome/blob/56aa58780da5fe73637ed9f583fafbd0b3a26db7/esphome/components/bme280_base/bme280_base.cpp#L152-L153

The ESPHome code correctly extract the 12 bits in signed 16 bits integer variables, but in case of negative value does not propagate the sign bit.

We should have something like the following to properly handle negative values:

this->calibration_.h4 = (this->calibration_.h4 & 0x7ff) - (this->calibration_.h4 & 0x800);
this->calibration_.h5 = (this->calibration_.h5 & 0x7ff) - (this->calibration_.h5 & 0x800);

Which version of ESPHome has the issue?

Latest github to date

What type of installation are you using?

Docker

Which version of Home Assistant has the issue?

N/A

What platform are you using?

ESP32

Board

Wemos D1 Mini

Component causing the issue

bme280_i2c

Example YAML snippet

No response

Anything in the logs that might be useful for us?

No response

Additional information

Incidentally came across that while doing code review to identify an issue with one of my sensors...

stas-sl commented 2 months ago

Or you can just cast to signed int before left shift, then sign bit will be automatically promoted when shifting:

this->calibration_.h4 = ((int16_t)(int8_t)read_u8_(BME280_REGISTER_DIG_H4) << 4) | 
                        (read_u8_(BME280_REGISTER_DIG_H4 + 1) & 0x0F);
this->calibration_.h5 = ((int16_t)(int8_t)read_u8_(BME280_REGISTER_DIG_H5 + 1) << 4) | 
                        (read_u8_(BME280_REGISTER_DIG_H5) >> 4);

As they do in the official(I suppose) implementation: https://github.com/boschsensortec/BME280_SensorAPI/blob/07a4960f31956ceee8ed071c9074c1cf05b68e76/bme280.c#L1520

int16_t dig_h4_lsb;
int16_t dig_h4_msb;
int16_t dig_h5_lsb;
int16_t dig_h5_msb;
dig_h4_msb = (int16_t)(int8_t)reg_data[3] * 16;
dig_h4_lsb = (int16_t)(reg_data[4] & 0x0F);
calib_data->dig_h4 = dig_h4_msb | dig_h4_lsb;
dig_h5_msb = (int16_t)(int8_t)reg_data[5] * 16;
dig_h5_lsb = (int16_t)(reg_data[4] >> 4);
calib_data->dig_h5 = dig_h5_msb | dig_h5_lsb;

Though, I wonder how you encountered this issue, as I've checked 3 my sensors and they all have values < 0x7F in registers 0xE4-0xE6.

// 1 
[00:14:06][D][bme280.sensor:158]: 0xE4: 0x13
[00:14:06][D][bme280.sensor:159]: 0xE5: 0x26
[00:14:06][D][bme280.sensor:160]: 0xE6: 0x03
[00:14:06][D][bme280.sensor:161]: H4: 0x0136
[00:14:06][D][bme280.sensor:162]: H5: 0x0032

// 2
[00:16:10][D][bme280.sensor:158]: 0xE4: 0x12
[00:16:10][D][bme280.sensor:159]: 0xE5: 0x20
[00:16:10][D][bme280.sensor:160]: 0xE6: 0x03
[00:16:10][D][bme280.sensor:161]: H4: 0x0120
[00:16:10][D][bme280.sensor:162]: H5: 0x0032

// 3
[00:35:59][D][bme280.sensor:158]: 0xE4: 0x11
[00:35:59][D][bme280.sensor:159]: 0xE5: 0x2c
[00:35:59][D][bme280.sensor:160]: 0xE6: 0x03
[00:35:59][D][bme280.sensor:161]: H4: 0x011c
[00:35:59][D][bme280.sensor:162]: H5: 0x0032

AmedeeBulle commented 1 month ago

Though, I wonder how you encountered this issue

I had some differences switching devices from my own code to ESPHome, so I started to read and compare the implementations...

The real issue was the default oversampling of EPSHome causing the sensor to overheat, but I decided to report this sign issue anyway. Not sure of the real impact though -- most libraries have it "wrong"...

esphome / issues