Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PID-Feature computes with outdated temperature values #17636

Closed
13 of 14 tasks
Wintespe opened this issue Jan 7, 2023 · 2 comments
Closed
13 of 14 tasks

PID-Feature computes with outdated temperature values #17636

Wintespe opened this issue Jan 7, 2023 · 2 comments
Assignees
Labels
enhancement Type - Enhancement that will be worked on fixed Result - The work on the issue has ended

Comments

@Wintespe
Copy link

Wintespe commented Jan 7, 2023

PROBLEM DESCRIPTION

A clear and concise description of what the problem is.

This problem report refers to the PID-feature implemented in xdrv_49_pid.ino.

The PID algorithm is called at the time intervals specified by the PID-command PidUpdateSecs. The temperature for the algorithm is updated by TelePeriod time slice.
If the time interval of TelePeriod is greater then the time interval from PidUpdateSecs the temperature value for the PID algorithm is outdated.

Code analysis
To run the PID algorithm, the function

void PIDEverySecond()
   ...
   PIDRun();
   ...

is called.

The temperature for the algorithm is transmitted within the function

void PIDShowSensor()
  ...
   Pid.pid.setPv(temperature, Pid.last_pv_update_secs);
   ...

If the time interval of TelePeriod is greater then the time interval from PidUpdateSecs
the temperature value for the PID algorithm is outdated.

To fix the problem change:

void PIDEverySecond() {
  static int sec_counter = 0; // overflow after 70 years, ok
  Pid.current_time_secs++;    // increment time
  //AddLog(LOG_LEVEL_INFO, PSTR("PIDEverySecond sec_counter: %d"), sec_counter );

  // run the pid algorithm if Pid.run_pid_now is true or if the right number of seconds has passed or if too long has
  // elapsed since last pv update. If too long has elapsed the the algorithm will deal with that.
  if (Pid.run_pid_now  ||  Pid.current_time_secs - Pid.last_pv_update_secs > Pid.max_interval  ||  (Pid.update_secs != 0 && sec_counter++ % Pid.update_secs  ==  0)) {
    PIDRun();
    Pid.run_pid_now = false;
  }
}

to

void PIDEverySecond() {
  static int sec_counter = 0; // overflow after 70 years, ok
  Pid.current_time_secs++;    // increment time
  //AddLog(LOG_LEVEL_INFO, PSTR("PIDEverySecond sec_counter: %d"), sec_counter );

  // run the pid algorithm if Pid.run_pid_now is true or if the right number of seconds has passed or if too long has
  // elapsed since last pv update. If too long has elapsed the the algorithm will deal with that.
  if (Pid.run_pid_now  ||  Pid.current_time_secs - Pid.last_pv_update_secs > Pid.max_interval  ||  (Pid.update_secs != 0 && sec_counter++ % Pid.update_secs  ==  0)) {
    if (Pid.run_pid_now == false) PIDShowSensor(); // set actual process value
    PIDRun();
    Pid.run_pid_now = false;
  }
}

by adding the line

if (Pid.run_pid_now == false) PIDShowSensor(); // set actual process value

That calls P IDShowSensor() if it wasn't triggered by PIDShowSensor() itself.

REQUESTED INFORMATION

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

  • Read the Contributing Guide and Policy and the Code of Conduct
  • Searched the problem in issues
  • Searched the problem in discussions
  • Searched the problem in the docs
  • Searched the problem in the chat
  • Device used (e.g., Sonoff Basic): NodeMcu
  • Tasmota binary firmware version number used: 11.0.2 and above
    • Pre-compiled
    • Self-compiled
  • Flashing tools used: Tasmitizer
  • Provide the output of command: Backlog Template; Module; GPIO 255:
  Configuration output here:
na
  • If using rules, provide the output of this command: Backlog Rule1; Rule2; Rule3:
  Rules output here:
na
  • Provide the output of this command: Status 0:
  STATUS 0 output here:
na
  • Set weblog to 4 and then, when you experience your issue, provide the output of the Console log:
  Console output here:
na

TO REPRODUCE

Steps to reproduce the behavior:
na

EXPECTED BEHAVIOUR

A clear and concise description of what you expected to happen.

The PID algorithm should not run with outdated temperature data.

SCREENSHOTS

If applicable, add screenshots to help explain your problem.

TelePeriod 300
PidUpdateSecs 1
The picture shows a time/temperature diagram of a step function (setpoint 25 ->30 °C) after the software fix.

image

ADDITIONAL CONTEXT

Add any other context about the problem here.

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

arendst added a commit that referenced this issue Jan 7, 2023
@arendst arendst self-assigned this Jan 7, 2023
@arendst arendst added enhancement Type - Enhancement that will be worked on fixed Result - The work on the issue has ended labels Jan 7, 2023
@arendst
Copy link
Owner

arendst commented Jan 7, 2023

Thx.

@Wintespe
Copy link
Author

Wintespe commented Jan 7, 2023

Thanks for the effort

@Wintespe Wintespe closed this as completed Jan 7, 2023
Wintespe referenced this issue Sep 20, 2024
#22162)

* Refactor and fix PID sensor (PID_USE_LOCAL_SENSOR) read race condition

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.

* Reduce logging of PID now that query side-effects removed

* Comment out all new logging, but leave present for next debugger
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Type - Enhancement that will be worked on fixed Result - The work on the issue has ended
Projects
None yet
Development

No branches or pull requests

2 participants