arendst / Tasmota

Alternative firmware for ESP8266 and ESP32 based devices with easy configuration using webUI, OTA updates, automation using timers or rules, expandability and entirely local control over MQTT, HTTP, Serial or KNX. Full documentation at
https://tasmota.github.io/docs
GNU General Public License v3.0
22.26k stars 4.81k forks source link

TasmotaGlobal.temperature_celsius includes ESP32 core temperature --> wrong compensation for Sensores #15834

Closed Links2004 closed 2 years ago

Links2004 commented 2 years ago

PROBLEM DESCRIPTION

TasmotaGlobal.temperature_celsius includes ESP32 Temperature resulting in wrong compensation for Sensores.

for example the Absolute Humidity calculated here:

https://github.com/arendst/Tasmota/blob/c08561f67c722556655da7b8cbe1482d98aecd90/tasmota/tasmota_xsns_sensor/xsns_21_sgp30.ino#L86-L90

jumps from 8.8 g/m3 to 32.9 g/m3 and back, based on if the ESP32 Temperature where used or the connected Temperature sensor. since there is a big difference in Temperature.

ESP32-DevKit
Air Sensor
MQ-2    1.02 ppm
MQ-3    null ppm
MQ-6    876.90 ppm
MQ-7    0.18 ppm
BME280 Temperature  24.0 °C
BME280 Humidity 36.6 %
BME280 Dew point    8.2 °C
BME280 Pressure 994.8 hPa
MHZ19B Carbon dioxide   410 ppm
MHZ19B Temperature  26.0 °C
SGP30 eCO₂  400 ppm
SGP30 TVOC  0 ppb
SGP30 Abs Humidity  35.2803 g/m3
ESP32 Temperature   52.8 °C

I found https://github.com/arendst/Tasmota/issues/12630 but my firmware is form this year.

tested with

Program Version 11.1.0.3(tasmota)
Build Date & Time   2022-05-31T15:54:53

and

Program Version 12.0.1.2(tasmota)
Build Date & Time   2022-06-20T13:26:23

REQUESTED INFORMATION

Make sure your have performed every step and checked the applicable boxes before submitting your issue. Thank you!

- [x] If using rules, provide the output of this command: `Backlog Rule1; Rule2; Rule3`:
```lua
  Rules output here:
12:34:41.472 CMD: Backlog Rule1; Rule2; Rule3
12:34:41.516 MQT: stat/tasmota_14FBDC/RESULT = {"Rule1":{"State":"OFF","Once":"OFF","StopOnError":"OFF","Length":0,"Free":511,"Rules":""}}
12:34:41.722 MQT: stat/tasmota_14FBDC/RESULT = {"Rule2":{"State":"OFF","Once":"OFF","StopOnError":"OFF","Length":0,"Free":511,"Rules":""}}
12:34:41.928 MQT: stat/tasmota_14FBDC/RESULT = {"Rule3":{"State":"OFF","Once":"OFF","StopOnError":"OFF","Length":0,"Free":511,"Rules":""}}
- [ ] Set `weblog` to 4 and then, when you experience your issue, provide the output of the Console log:
```lua
  Console output here:

TO REPRODUCE

use anything that uses TasmotaGlobal.temperature_celsius with the ESP32

EXPECTED BEHAVIOUR

ESP32 temperature is not included in TasmotaGlobal.temperature_celsius

SCREENSHOTS

image

(Please, remember to close the issue when the problem has been addressed)

arendst commented 2 years ago

Disable ESP32 temperature with command setsensor127 0

Links2004 commented 2 years ago

yes this works a a workaround. But not everyone will notice this so excluding it from TasmotaGlobal.temperature_celsius sounds like a good idea.

arendst commented 2 years ago

What works for you doesn't work for energy monitoring devices using the single present temp sensor to monitor overtemp.

So just execute the above command and you're good to go.

sfromis commented 2 years ago

Anyway, that ESP32 Temperature cannot generally be expected to have any relationship to reality, like actual chip temperature. May depend on chip variant.

And using SGP30 with more than one temperature source works badly, even if one of them is not an useless ESP32 Temperature. Since your BME280 and MHZ19B are not in agreement already, expect unstable and "less usable" numbers for absolute humidity.

Links2004 commented 2 years ago

yes the MHZ19B shows to high temperatures too. Resulting in the same problem with the ESP32 but less extrem in the Abs Humidity calculation. the bad part is that is practically random which temperature is stored in TasmotaGlobal. I ended up adding some defines to disable the Global update. if you like I can open a PR.

It where unexpected to find that the Convert functions are updating the TasmotaGlobal, but I added a optional parameter to disable the update for ConvertTemp.

diff --git a/tasmota/tasmota_support/support.ino b/tasmota/tasmota_support/support.ino
index 0cb2d6af2..a1d5dac64 100755
--- a/tasmota/tasmota_support/support.ino
+++ b/tasmota/tasmota_support/support.ino
@@ -756,8 +756,10 @@ void UpdateGlobalTemperature(float c) {
   TasmotaGlobal.temperature_celsius = c;
 }

-float ConvertTemp(float c) {
-  UpdateGlobalTemperature(c);
+float ConvertTemp(float c, bool update = true) {
+  if (update) {
+    UpdateGlobalTemperature(c);
+  }

   return ConvertTempToFahrenheit(c);
 }
diff --git a/tasmota/tasmota_xsns_sensor/xsns_127_esp32_sensors.ino b/tasmota/tasmota_xsns_sensor/xsns_127_esp32_sensors.ino
index 8ac2ee7f6..b7559307b 100644
--- a/tasmota/tasmota_xsns_sensor/xsns_127_esp32_sensors.ino
+++ b/tasmota/tasmota_xsns_sensor/xsns_127_esp32_sensors.ino
@@ -61,9 +61,11 @@ void Esp32SensorShow(bool json) {
     add_global_temp = !ResponseContains_P(PSTR(D_JSON_TEMPERATURE));
   }
   float c = CpuTemperature();  // in Celsius
+#ifndef ESP32_SENSORS_TEMP_NO_GLOBAL
   if (add_global_temp) {
     UpdateGlobalTemperature(c);
   }
+#endif
   float t = ConvertTempToFahrenheit(c);

 #if CONFIG_IDF_TARGET_ESP32
diff --git a/tasmota/tasmota_xsns_sensor/xsns_15_mhz19.ino b/tasmota/tasmota_xsns_sensor/xsns_15_mhz19.ino
index 67745ddb4..2d61cd9fd 100644
--- a/tasmota/tasmota_xsns_sensor/xsns_15_mhz19.ino
+++ b/tasmota/tasmota_xsns_sensor/xsns_15_mhz19.ino
@@ -225,7 +226,11 @@ void MhzEverySecond(void)
       }
     } else {
       uint16_t ppm = (mhz_response[2] << 8) | mhz_response[3];
+#ifdef USE_MHZ19_TEMP_NO_GLOBAL
+      mhz_temperature = ConvertTemp((float)mhz_response[4] - 40, false);
+#else
       mhz_temperature = ConvertTemp((float)mhz_response[4] - 40);
+#endif
       uint8_t s = mhz_response[5];
       mhz_type = (s) ? 1 : 2;
       if (MhzCheckAndApplyFilter(ppm, s)) {
arendst commented 2 years ago

Thx for you input.

Now consider the fact that there are many more calls to ConvertTemp. I challenge you to come up with a solution workable for all of them, not just the ones you feel you need, and small code change without many #defines but on the fly control.

sfromis commented 2 years ago

One feasible strategy could be to allow ConvertTemp to just not set global temperature (like another SetOption :grin: ), but have the user create a rule to use the GlobalTemp (and GlobalHum) command for the sensor of choice.

Links2004 commented 2 years ago

If I think big with full config options, I propose:

  1. remove the global update from all the Convert functions (its misleading naming in my opinion)
  2. add a new function to do the global update that includes the ID of the sensor
  3. add settings for all the different global values to set which sensor ID to use (and use this in combination with the ID parameter of the function)

May have the old behavior (no filtering) as default aka setting ID as 0 and if ID setting > 0 then only allow this sensor for this global parameter.

with this this will be flexible for any use case, and the User has full control during runtime. but i am currently not familiar enough to say how to do this in the code base or how easy it is.

arendst commented 2 years ago

I'll sit and think about a solution for user selection of global temperature/humidity/pressure selection.

arendst commented 2 years ago

Suppose I remove the current global temp/hum/pressure code and change it to something based on the JSON data like

14:44:18.960 MQT: tele/esp32d/SENSOR = {"Time":"2022-06-22T14:44:18","BME680":{"Temperature":26.4,"Humidity":48.0,"DewPoint":14.5,"Pressure":1014.4,"Gas":1020.41},"ESP32":{"Temperature":51.7},"PressureUnit":"hPa","TempUnit":"C"}

In this case there are two temperatures. Suggesting to let user select between the first or the second one to become global with a new command GlobalTempSelect <number>. If there is only one temp/hum/pressure it becomes the global one automagically. Is that usable for you?

This results in global values updates only at Teleperiod time (default 300 secs). As an alternative I could add a "behind the scenes" update every minute.

The goal of this change is to use as less code as possible while obtaining max user control. Your initial thoughts costs too much code space and would need an external list for users to scan through to find the correct sensor number (as temp is set by user, drivers and sensors).

Links2004 commented 2 years ago

with two assumptions this sounds fin for my use case

but the 300 sec Teleperiod default sounds to high for the over temperature use case we had in a earlier discussion.

arendst commented 2 years ago

I'll implement this as an option to the current situation and see what the future brings.

Thx so far.

Jason2866 commented 2 years ago

@Links2004 Please test and close the issue if the changes are working as expected.

Links2004 commented 2 years ago

Firmware updated and used:

GlobalTemp2 1
GlobalHum2 1
GlobalPress2 1

first look at the web interface is good, no jumping there. will let it run a bit and check my Grafana to see it over time.

Links2004 commented 2 years ago

looks like that the current implementation has some race condition, update around 18:35 image

Links2004 commented 2 years ago

did some dissecting and the breaking change is the 10sec update, but no idea how to fix it.

diff --git a/tasmota/tasmota_support/support_tasmota.ino b/tasmota/tasmota_support/support_tasmota.ino
index 9c70c4890..18694c0e9 100644
--- a/tasmota/tasmota_support/support_tasmota.ino
+++ b/tasmota/tasmota_support/support_tasmota.ino
@@ -1086,7 +1086,7 @@ void PerformEverySecond(void)
   if (!(TasmotaGlobal.uptime % 10)) {
     for (uint32_t type = 0; type < 3; type++) {
       if (!Settings->global_sensor_index[type] || TasmotaGlobal.user_globals[type]) { continue; }
-      GetSensorValues();
+      //GetSensorValues();
       break;
     }
   }
arendst commented 2 years ago

I'm out the next days.

Now that you have commented out the 10 minute update global values will only be updated at teleperiod time. From your status 0 output it is set to 30 seconds which is ok as then it will use the global values from 30 seconds ago for the SGP calculation.

When I'm back I'll try to think of a more teleperiod synced approach but in your case (having already a small teleperiod set) it wouldn't make it must better.

Links2004 commented 2 years ago

yes for my configuration the teleperiod update all 30sec is fin, since its only monitoring.

Jason2866 commented 2 years ago

Closing, feature has been added