EasyG0ing1 / BlockNot

BlockNot is a library that creates non-blocking timers with simplicity.
MIT License
40 stars 4 forks source link

trigerred () after firstTrigger() return true #17

Open guillaumechardin opened 1 year ago

guillaumechardin commented 1 year ago

Hello,

It seems that triggered() return true after firstTrigger() was called before. It seems this behaviour not correct, since code can be executed many times.

Consider this code that test firstTrigger().

void loop() {
  BlockNot Mytimer = BlockNot(100);
  int i = 1;
  unsigned int count = 0;

   while(1)
  {
    Serial.printf("Iteration %d ", i);
    Serial.printf("Next trigger occur in %d ms\n", Mytimer.getTimeUntilTrigger());
    if(Mytimer.firstTrigger() )
    {
      Serial.println("  *FIRST trigger occur");
      i++;
    }
    else if(Mytimer.triggered() )
    {
      Serial.println("  *STANDARD trigger occur");
      i++;
    }
    else
    {
      Serial.println("  *NO trigger occur");
    }
    delay(30);

    if (i > 5)
    {
      Serial.println("  *RESET TIMER , next line should be FIRST");
      Mytimer.RESET;
      i=1;
    }
  }
}

See notes inside the Serial transcript bellow

Iteration 1 Next trigger occur in 100 ms
  *NO trigger occur
Iteration 1 Next trigger occur in 70 ms
  *NO trigger occur
Iteration 1 Next trigger occur in 40 ms
  *NO trigger occur
Iteration 1 Next trigger occur in 10 ms
  *NO trigger occur
Iteration 1 Next trigger occur in 0 ms
  *FIRST trigger occur         -----> this is OK
Iteration 2 Next trigger occur in 0 ms
  *STANDARD trigger occur  ---> this should not occur since it have already been checked a moment ago.
Iteration 3 Next trigger occur in 70 ms
  *NO trigger occur
Iteration 3 Next trigger occur in 40 ms
  *NO trigger occur
Iteration 3 Next trigger occur in 10 ms
  *NO trigger occur
Iteration 3 Next trigger occur in 0 ms
  *FIRST trigger occur
Iteration 4 Next trigger occur in 0 ms
  *STANDARD trigger occur              -----> this is OK
Iteration 5 Next trigger occur in 70 ms
  *NO trigger occur
Iteration 5 Next trigger occur in 40 ms
  *NO trigger occur
Iteration 5 Next trigger occur in 10 ms
  *NO trigger occur
Iteration 5 Next trigger occur in 0 ms
  *FIRST trigger occur                 ----> why FIRST Trigger here, it has not been reseted
  *RESET TIMER , next line should be FIRST   --> here we reset timer
Iteration 1 Next trigger occur in 100 ms
  *NO trigger occur
Iteration 1 Next trigger occur in 70 ms
  *NO trigger occur
Iteration 1 Next trigger occur in 40 ms
  *NO trigger occur
Iteration 1 Next trigger occur in 10 ms
  *NO trigger occur
Iteration 1 Next trigger occur in 0 ms
  *FIRST trigger occur    ---> another first, but this one should be ok.
Iteration 2 Next trigger occur in 0 ms
  *STANDARD trigger occur
Iteration 3 Next trigger occur in 70 ms
  *NO trigger occur
Iteration 3 Next trigger occur in 40 ms
  *NO trigger occur
Iteration 3 Next trigger occur in 10 ms
  *NO trigger occur
Iteration 3 Next trigger occur in 0 ms
  *FIRST trigger occur
Iteration 4 Next trigger occur in 0 ms
  *STANDARD trigger occur
Iteration 5 Next trigger occur in 70 ms
  *NO trigger occur
Iteration 5 Next trigger occur in 40 ms
  *NO trigger occur
Iteration 5 Next trigger occur in 10 ms
  *NO trigger occur
Iteration 5 Next trigger occur in 0 ms
  *FIRST trigger occur
  *RESET TIMER , next line should be FIRST
Iteration 1 Next trigger occur in 100 ms
  *NO trigger occur
Iteration 1 Next trigger occur in 70 ms
  *NO trigger occur
Iteration 1 Next trigger occur in 40 ms
  *NO trigger occur
Iteration 1 Next trigger occur in 10 ms
  *NO trigger occur
Iteration 1 Next trigger occur in 0 ms
  *FIRST trigger occur
Iteration 2 Next trigger occur in 0 ms

What i missed in the firstTrigger() behaviour. What i am doing wrong. Is this a bug ?

Guillaume

EasyG0ing1 commented 1 year ago

@guillaumechardin I'll have a look at it and get back to you.

EasyG0ing1 commented 1 year ago

@guillaumechardin OK, so what is happening is that you're calling firstTrigger() then immediately after you're calling triggered(). When firstTrigger() is called, the timer is not reset (and that is intentional) so it makes sense that triggered() would also return true since the timer had not been reset.

Lets break it down:

Timer is reset, at that moment, the value of millis() is recorded into a variable, let's call that startTime

... 100ms has passed

call to firstTrigger() returns true because it calculates the current millis() against startTime and the difference is 100ms or more so it returns true. That method DOES NOT reset the startTime value to the current millis() (and I'll explain why in a moment)

Next, you call triggered() and because the value of startTime has not been changed, it will also respond with true. HOWEVER, that method will also reset startTime to be the current value of millis().

A subsequent call to firstTrigger() after another 100ms has passed will return true because startTime was reset when you called triggered()

firstTrigger() was a method that I included in the library for a specific purpose. So that you can have a timer that only triggers once and will not trigger again until you manually reset the timer. However, if you end up calling triggered(), then the timer will reset which will cause firstTrigger() to trigger again once the duration of the timer has passed.

If you need to use a firstTrigger() scenario in conjunction with triggered() then you should create two timers and use one for firstTrigger() and the other for triggered().

Also, if you need to know after calling firstTrigger() if the timer has triggered, then you can call timer.HAS_TRIGGERED or triggered(NO_RESET)

The whole point of a trigger in the first place, is to find out if the timers duration has passed or not. And the duration is calculated by subtracting the value of millis() against the recorded value of millis() when the timer was reset.

If the timer is a microsecond timer than the micros() method is used instead of millis() to keep track of time and duration etc.

EasyG0ing1 commented 1 year ago

@guillaumechardin And just for clarity, the way the firstTrigger() method works is like this:

If the duration of the timer has passed, it checks a flag to see if it has already responded with true. If it hasn't, then it sets that flag and responds true. When you call that method again, it checks the flag to see if it has already responded with true and if it has, then it responds with false. Every call to firstTrigger() will return false until that flag is reset, and that flag gets reset when the RESET method is called and the triggered() method ALWAYS calls RESET unless you pass in the NO_RESET constant triggered(NO_RESET).

guillaumechardin commented 1 year ago

Hello @EasyG0ing1 thanks for your explanations. Its clear for me right now. I understand the culprit and why I need to use two timers (and for now it not a solution for me :) )... Or, another solution I have is to edit the code to add another persitant flag that does not rely somehow on time comparison like in hasTrigerred() or firstTrigger() (this last relying on hastriggered() ) and at least reset() too. rewriting firstTrigger():

bool BlockNot::firstTrigger() {
    if(onceTriggered) {
        return false;
    }
    else if (hasTriggered()) {
        onceTriggered = true;
        onceTriggeredPersistant = true;
        return timerState == RUNNING;
    }
    return false;
}

in my code

if (myTimer.onceTriggeredPersistant  || myTimer.firstTrigger() )
{
   myTimer.trigerred(); //to reset timer for subsequent elseif tests
   //more custom code
}
else if (myTimer.triggered() )
{
    //my code
}
[...]

What do you think about that ?

EasyG0ing1 commented 1 year ago

@guillaumechardin

So firstTrigger is intended to only return true one time until the timer is reset. triggered() returns true and resets the timer. UNLESS you call triggered(NO_RESET) but then it will respond true every time you call it after the timers duration has passed.

There is also the timer.TRIGGERED_ON_MARK option which is a slight twist to triggering as it essentially ensures that triggers happen consistently at every duration point ... so lets say you have a timer for 1000 ms ... and 1300 ms have passed since it was last triggered ... TRIGGERED_ON_MARK will return true when you call at at 1300 ms but it will also return true in another 700 ms because that's the mark point (every 1000 ms) ... its kind of intended to provide a steady pulse of triggers. There's another flavor of that method that keeps track of missed triggers and gives you the option of making those up or not.

Instantiating a second timer is not very resource intensive as it really just declares a couple of variables where all the magic happens when you invoke the methods of the class...

What is your specific use case? Maybe I can help either with a recommended code mod for this library or maybe some other idea ...

EasyG0ing1 commented 1 year ago

@guillaumechardin

in my code

if (myTimer.onceTriggeredPersistant  || myTimer.firstTrigger() )
{
myTimer.trigerred(); //to reset timer for subsequent elseif tests
//more custom code
}
else if (myTimer.triggered() )
{
//my code
}
[...]

What do you think about that ?

So not being sure what you're trying to accomplish ... the timer is going to reset after every trigger event. What you've shown here is pretty much no different than just using triggered(). And I don't know where you set onceTriggeredPersistant back to false if ever and if not, then that first test condition will always execute every loop after the timer triggers and will continue to execute every loop even after the timer is reset.

If you want that first test conditions code block to execute every loop after the timer has triggered, then you can just use triggered(NO_RESET).

Also, I would change this line:

   myTimer.trigerred(); //to reset timer for subsequent elseif tests

To this

   myTimer.RESET; //to reset timer for subsequent elseif tests

And you can you make your code easier to read and write ... here is your exact same code

if (myTimer.onceTriggeredPersistant  || myTimer.FIRST_TRIGGER )
{
   myTimer.RESET; //to reset timer for subsequent elseif tests
   //more custom code
}
else if (myTimer.TRIGGERED )
{
    //my code
}

I put the constants in the library to make things read and write easier ... more simple... They're actually called macros (not constants in this context), but the way I'm using the C++ macro makes these more akin to a method alias because the "macro" doesn't actually do any code processing.

But here is how I would actually handle the scenario you seem to be going after in your code:

loop() {
    static bool timerTriggered = false;

    if (timerTriggered || myTimer.FIRST_TRIGGER ) {
    //CODE BLOCK 1
        timerTriggered = true;
        myTimer.RESET;
    }   
    else if(myTimer.TRIGGERED) {
    //CODE BLOCK 2
        //my code
    }
}

NOW, the TRIGGERED call for code block 2 is essentially never going to happen because if the timers duration has passed then the test for FIRST_TRIGGER will keep the TRIGGERED event from happening because you reset the timer in code block 1. Also if you never set the timerTriggered variable to false, then code block 1 will execute every loop after a timer first triggers regardless of whether or not the timer was reset.

guillaumechardin commented 1 year ago

Hello,

it seems that your idea get the job done and is more elegant than rewriting the library code 👍 loop() {

loop() {
    static bool timerTriggered = false;

    if (timerTriggered || myTimer.FIRST_TRIGGER ) {
    //CODE BLOCK 1
        timerTriggered = true;
        myTimer.RESET;
    }   
    else if(myTimer.TRIGGERED) {
    //CODE BLOCK 2
        //my code
    }
}

What is your specific use case? Maybe I can help either with a recommended code mod for this library or maybe some other idea ... My goal is to use the lib to "debounce" some button on capacitive touch sensor.

Thanks.

EasyG0ing1 commented 11 months ago

@guillaumechardin

I always found some of the debouncing discussions to be rather interesting, where there is a method that seems to be considered the best way to do it (though I can't remember which one that is at the moment). In practice, I generally found that (depending on the button of course) - 150ms was usually enough buffer time to debounce effectively.

This method, however, is a backwards way of handling it, though it would work quite well...

So lets say that you define the longest time that a person might hold down a button would be less than one second. If you define a one second timer, then make the button press dependent on that timer triggering, it would execute the code in the if condition, but then that condition would not be able to execute again until the duration of the timer has passed, effectively debouncing the button and ensure that the button code doesn't run repeatedly:

BlockNot btnOneTimer(1, SECOND);

#define BUTTON_ONE_PRESSED digitalRead(btnPin1) == 0

loop {

    if(BUTTON_ONE_PRESSED && btnOneTimer.TRIGGERED) {
        //Code to run when button is pressed.
    }
}

OR, you could do something like this which guarantees that the button code only happens one time no matter how long they hold the button down, where it wont run that code again until they release then press the button again:

BlockNot btnOneTimer(1, SECOND);
boolean btnOnePressed = false;

#define BUTTON_ONE_PRESSED digitalRead(btnPin1) == 0
#define BUTTON_ONE_RELEASED digitalRead(btnPin1) == 1

loop {

    if(BUTTON_ONE_PRESSED && btnOneTimer.TRIGGERED && !btnOnePressed) {
        btnOnePressed = true;
        //Code to run when button is pressed.
    }

    if(BUTTON_ONE_RELEASED && btnOneTimer.HAS_TRIGGERED && btnOnePressed) {
        btnOnePressed = false;
    }
}

The timer is still necessary to prevent false triggers during debouncing... the HAS_TRIGGERED call won't reset the timer which means that if they release the button but then press it again in less than a second, the first if clause will still trigger.