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.03k stars 4.78k forks source link

Refactor and fix PID sensor (PID_USE_LOCAL_SENSOR) read race condition #22162

Closed spacelama closed 1 week ago

spacelama commented 1 week ago

Refactor PID since it was calling pid.tick willy-nilly upon demand from MQTT and the web (PIDShowValues and PIDShowValuesWeb had fundamental side-effects!) instead of on a periodic basis (and was being called with time interval of 0 when those times lined up!). Refactor web/mqtt display because there was shared code (that code turned out to be misguided and belonged in Every_Second loop, but now we are also similar to 39 thermostat)

Logging revealed that the vast majority of the time the sensor JSON was parsed to obtain current sensor data when using PID local sensor, it was failing to parse (and it would typically only work for a second around TELE_PERIOD, but even then, not reliably). This bug almost certainly affects xdrv_39_thermostat too, but using xsns_75_prometheus.ino as a template, we are able to update PV once per second, which allows us to be a lot more responsive. There is no danger of being "too responsive" because that's the point of PID, and the PID loop already scales feedback by interval between ticks.

Fixes #22094 tested against both local sensor and MQTT set PV

Description:

Related issue (if applicable): fixes #22094

Checklist:

NOTE: The code change must pass CI tests. Your PR cannot be merged unless tests pass

eku commented 1 week ago

A lot of additional log output. Did you need them for analysis and do they add value to the release?

spacelama commented 1 week ago

I did need them for analysis, and they are at the DEBUG level. They will also help people when they're looking what to populate PID_LOCAL_SENSOR_NAME/PID_LOCAL_SENSOR_TYPE etc with.

Can probably do away with PIDShowValues now that it doesn't have side-effects, and PIDEverySecond, "Got isNum", maybe convert "not valid JSON" to error or INFO or similar (but might want it throttled, but perhaps the existence of the "PID: Local temperature sensor missing for longer than PID_MAX_INTERVAL" would prompt the user to turn on DEBUG which would then highlight this error).

I'll push a new commit...

eku commented 1 week ago

they are at the DEBUG level

Yes, but they increase the images size a lot.

sfromis commented 1 week ago

Some drivers have "additional development debug" wrapped in #ifdef (or even commented out after development finished), to avoid the code size increase.

blacknell commented 1 week ago

Good stuff @spacelama

Wintespe commented 1 week ago

Mhm, you remove line 255 in your changes of the PID controller.

PIDShowSensor(); // set actual process value

Refer to: PID-Feature computes with outdated temperature values #17636

We are back to outdated temperature values. Now missing:

Pid.pid.setPv(sensor_reading, Pid.last_pv_update_secs);

This function sets the current process data in the PID algorithm. This no longer happens in PIDEverySecond()

but in the time slice of TelePeriod. Therefore, if the outdated data,

spacelama commented 1 week ago

@Wintespe , see my reply again your comment in #22094 .