Wait for target

[Micilera]

v4.3.9
Please look at the method Heater::wait_for_target().
I can't understand the logic.
In the following there are some scenarios:
0) Setup is: TEMP_RESIDENCY_TIME = 10, TEMP_WINDOW = 1, HOTEND_HYSTERESIS 2

1. A) Very First Loop, target_temperature = 250, current_temperature = 249.1,
   B) residency_start_ms is zero
   C) So the temperature error temp_diff 0.9 is less than TEMP_WINDOW
   D) The timer is set to: residency_start_ms = now+10s because is the very first time
   E) The second loop, target_temperature = 250, current_temperature = 249.0,
   F) The temperature error is inside the histeresis range, so the timer is not touch
   G) The exit condition of the loop is relative to now vs. (residency_start_ms + 10)
   H) So the timeout is 20s, because residency_start_ms was set 10s ahead.
2. A) Very First Loop, target_temperature = 250, current_temperature = 248.9,
   B) residency_start_ms is zero
   C) So the temperature error temp_diff is NOT less than TEMP_WINDOW
   D) The timer is not set
   E) The second loop, , target_temperature = 250, current_temperature = 249.1,
   F) residency_start_ms is zero
   G) So the temperature error temp_diff 0.9 is less than TEMP_WINDOW
   H) The timer is set to: residency_start_ms = now because it is no more the very first time
   I) Suppose that the temperature will not change
   L) So the timeout is 10s.
   M) Why two different timeouts just because the very first time the temperature are a little bit different?
3. Same of scenario 1
   E) The second loop, target_temperature = 250, current_temperature = 247.9,
   F) The temperature error is outside the histeresis range, so the timer is set residency_start_ms = now
   G) The third loop, target_temperature = 250, current_temperature = 249.0 and will not change
   H) The temperature error is inside the histeresis range, so the timer is not touch
   I) The exit condition of the loop is relative to now vs. (residency_start_ms + 10)
   L) So the timeout is 10s. The timeout is less that the timeout of scenario 1
   where the temperature are much closer to the target.

The above are only some undesiderable behaviours of this logic. Could you please explain?

[iosonopersia]

Hi @Micilera , I'm now looking into this trying to find an explanation for that logic.
To me it seems that the trouble is caused only by the following line:

if (first_loop) residency_start_ms += (TEMP_RESIDENCY_TIME) * 1000UL;

Without it, everything should work as expected, am I right?
By the way, we should be very careful here, if someone wrote that line it's surely to fix a problem that we may not be aware of.
As you can see, Marlin has the same identical logic: have a look here (Marlin 2.0.x branch).

[MagoKimbra]

The first loop, when setting the temperature, stretches to allow time for the thermal resistance to accumulate heat before checking whether or not there is a rise in temperature...

[iosonopersia]

@Micilera , you can have a look here: it's the commit that introduced that logic in Marlin a year ago.

Commit message says:

Skip hysteresis check when temp is already close to target
To eliminate a long delay during pause, park, and filament change

I hope this can help you understand better...

[Micilera]

Sirs,
Thank You very much for the explanation. Very appreciated.
I have made more thoughts about this function and in the following I explain my results in order to be positive and improving the quality of the project:

1. Even if the logic comes from a great project, does not mean it is correct. There is always something to improve.
2. For sure, this 'first loop' logic has no sense implemented in this way. See the first message I sent.
3. This logic to double the initial timeout has no sense either. This is a class heater method
   and the physical time costant of the devices are very different whilts the timings (#define) are common to all.
   This implementation has not a physical meaning. As it is, it is a very raw approch.
4. the output of isCooling() could become critical. If for one sample only, the current temperature (ie: 240.01) is greater than the target temperature (ie:240),
   due to an oscillation around the target temperature, it exits immediately from the loop, despite of the timer logic and all the other thresholds. Has this a meaning?
5. If for any reasons, the temperature never enters in the TEMP_WINDOW threshold, we are waiting forever;
   this is true for all the heaters; there is not a global timeout for heating. May be there is something for cooling but also about
   that I would have something to say.
6. Do not call this method while in idle state because it checks/uses the target temperature only.
7. In v4.4.0 the timer class in src\lib\timer.h has an undesiderable feature. In order to prove that, I made a PC console test
   using your class timer and some glue functions. The time elapsed is simulated by a counter:

static uint32_t millis_counter = 100000; // Starting value. Any value is ok
uint32_t millis(void) {
  return millis_counter;
}

uint32_t tick(void) {
  return ++millis_counter;
}

void Heater::test_timer()
{
  long_timer_t residency_start_timer;
  uint32_t     t_before, t_after;
  bool  b_running, b_pending;
  tick();
  residency_start_timer.start((TEMP_RESIDENCY_TIME) * 1000UL);
  t_before = millis();
  do
  {
    tick();
    b_running = residency_start_timer.isRunning();
    b_pending = residency_start_timer.pending((TEMP_RESIDENCY_TIME) * 1000);
  } while ((b_running == false) || (b_pending == true));
  t_after = millis();
  printf("Time elapsed: %lu b_running %d b_pending %d\n", t_after-t_before, b_running, b_pending);
}

...and the output is:

Time elapsed: 1 b_running 1 b_pending 0
              ^
              |
              +++++ Shouldn't it be 20000?

Please made a look at your class timer:

      if (ms <= ms + period_ms) {  // If not an overflow, this test is always true, as all unsigned numbers.
        if ((now >= ms + period_ms) || (now < ms)) expired = true;  // Second condition ?????
      }
      else if ((now >= ms + period_ms) && (now < ms)) expired = true;

and please think a little bit about this logic.

Hope that all of the above could be helpful.
Best Regards.

[Micilera]

Sorry I didn't want to put the text in bold.

[MagoKimbra]

The timer class can be called short or long, this is to save ram space for some timers.
When the class is defined short, uint16 can go from 0 to 65535.

bool expired(const T period_ms, const bool renew=true) {
      if (!running || !period_ms) return false;
      bool expired = false;
      const T now = millis();
      if (ms <= ms + period_ms) {
        if ((now >= ms + period_ms) || (now < ms)) expired = true;
      }
      else if ((now >= ms + period_ms) && (now < ms)) expired = true;
      if (expired) {
        renew ? ms = now : running = false;
      }
      return expired;
    }

Now when timer is short, ms and period_ms is short.
For example: ms = 60000 and period_ms = 15000.
First check if ( ms<= ms + period_ms) = if (60000 <= 60000 + 15000) = if (60000 <= 9465)
Because uint16 when is overflaw restart from 0, then 60000 + 15000 is equal to 9465.
The first check is false.
Second check
else if ((now >= ms + period_ms) && (now < ms)) expired = true;

now is same uint16 and it's equal to millis(), for example millis() = 120000, now is equal 54565

if ((54565 >= 60000 + 15000) && (54565 < 60000) expired= true
if ((54565 >= 9465) && ( true) expired = true
if (true && true) expired = true

Sorry for my bad english.
The two controls as they are seem strange, but they are used precisely because the variable can be 16 bit, therefore resetting each time when it reaches 65535 you have to double check to verify that the time has passed.
If the timer class had always been 32 bit the problem was simpler ..

The short timer class is used to see if a maximum of 65535 milliseconds then 65 seconds have passed.
For longer times it takes the 32-bit long class.

[Micilera]

Sirs,
Thank You again for the quick answer and for your time.

1. What about all the other items?
2. Due to your explanation about the range of the class timer data,
   I run again my test that, do not forget, is using your class.
   I have changed only the initial value of the counter and the data type
   of the timer:
   ...
   static uint32_t millis_counter = 10000;
   ...
   short_timer_t residency_start_timer;

and this is the answer:

Time elapsed: 1 b_running 1 b_pending 0

=================================================
So you can say whatever you like, but is the software that says what is correct
and what is not correct.
The test made with your SW is saying: it is not correct.
Best Regards.

[iosonopersia]

@Micilera just a quick message to tell you I fixed your comment above 👍

[Micilera]

Sirs,
v4.4.0
I really appreciate the changes you made to your 'timer' class.
You did almost the same I made. I would propose you to define a local variable:
const T now = T(millis());
const T d = T(ms + period_ms);
so that, you calculate the value once. But this is a little improvement.

Now it is time for you to remove the flag 'first loop' from the WaitToTarget() for the following reasons:

1. Used in this way, has no meaning.
2. Used in this way, your timer class does not work. If you search carefully, this is the only time you use
   the start() method in the following way:
   void start(const T period_ms) { ms = millis() + period_ms; running = true; }
   ie you set the timer start value in the future
   and the expired() method does not handle that. Please, think about that. This is true also for the v4.3.9.
   Best Regards,
   Micilera