Refactor and fix PID sensor (PID_USE_LOCAL_SENSOR) read race condition #22162
Conversation
Refactor PID since it was calling pid.tick willy-nilly upon demand from MQTT and the web 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.
|
A lot of additional log output. Did you need them for analysis and do they add value to the release? |
|
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... |
Yes, but they increase the images size a lot. |
|
Some drivers have "additional development debug" wrapped in |
| @@ -276,12 +292,14 @@ void PIDShowSensor() { | |||
| JsonParserToken value_token = root[PID_LOCAL_SENSOR_NAME].getObject()[PSTR(PID_LOCAL_SENSOR_TYPE)]; | |||
| if (value_token.isNum()) { | |||
| sensor_reading = value_token.getFloat(); | |||
| //AddLog(LOG_LEVEL_DEBUG_MORE, PSTR("PID: PIDProcessSensor: Got isNum: %s"), buf.c_str()); | |||
There was a problem hiding this comment.
No need to. Always usefull when debugging and as commented take up no code space.
|
Good stuff @spacelama |
|
Mhm, you remove line 255 in your changes of the PID controller.
Refer to: We are back to outdated temperature values.
This function sets the current process data in the PID algorithm. This no longer happens in but in the time slice of TelePeriod. Therefore, if the outdated data, |
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