ISR routine does not appear to be passing back boolean?

Status
Not open for further replies.

owhite

Active member
Folks,

I've using TeensyTimerTool and it is showing odd behavior. If I put a scope on the LED_PIN, in the code shown below, if I use loop(), it works fine. I get a very nice 8ms change in the LED_PIN. But in that version of loop() then I'm changing the volatile from the ISR which I thought was a no-no.

If I change the second function loop1(), to loop() (and remove the original loop, then there is no pause, and the LED_PIN changes as rapidly as loop can er, loop.

where am i going wrong?

owen
Code:
#include "TeensyTimerTool.h"

#define LED_PIN       13

using namespace TeensyTimerTool;
Timer timer1(TMR1);
Timer timer2(GPT1); // First channel on TMR1 aka QUAD timer module. (TMR1 - TMR4, four 16bit channels each)

// loop stuff
volatile boolean _pause = false;
boolean pause = false;

// LED 
boolean blinkOn = false;

void setup() {
  pinMode(LED_PIN, OUTPUT); 

  timer1.beginPeriodic(tickLoop, 1); 
  timer2.beginPeriodic(foo, 8000);
  pause = false;
  _pause = false;
}

// this makes a nice 8 ms square wave
void loop() {
  noInterrupts();
  pause = _pause;
  interrupts();

  while(_pause) {}
  _pause = true;

  digitalWrite(LED_PIN, blinkOn); 
  blinkOn = !blinkOn;
}

// this makes dogmeat
void loop1() {   
  noInterrupts();
  pause = _pause;
  interrupts();

  while(pause) {}
  pause = true;

  digitalWrite(LED_PIN, blinkOn); 
  blinkOn = !blinkOn;
}

void foo() { 
  _pause = false;
}

void tickLoop() {
  // does lost of other things
}
 
But in that version of loop() then I'm changing the volatile from the ISR which I thought was a no-no.

That's the purpose of volatile. It forces code to actually read the variable, so you *can* change it in the ISR and have your program detect the change.

Without volatile the compiler makes a lot of assumptions about memory as it compiles each function. Data in memory doesn't just spontaneously change. So when you write something like "while (pause) {}", the compiler tries very hard to be smart. It knows "pause" can't change on its own, so it will eliminate that loop. It's trying to be smart and avoid unnecessary work, and why would it bother having the loop at all when it knows the result will always be the same.

Using volatile tells the compiler that the data in memory *can* change spontaneously. It also tells the compiler that anything you write needs to be actually written out to memory ASAP (otherwise the compiler tries to be smart and may never actually do the write if it believes nothing else ever reads it). That's why you use it with variables shared with interrupts.
 
While Paul's comments are certainly true, in this special case the problem is different (the code assigns the volatile _pause to pause, so it wont be optimized away).

See my comments in the code for an explanation why it doesn't work.

Code:
// this makes dogmeat
void loop()
{
    noInterrupts();   // you never assign _pause other than in setup and the ISR...
    pause = _pause;   // ...since the ISR always sets _pause to false, pause will _always_ be set to false here
    interrupts();    

    while (pause) {}  // pause is always false -> never waits
    pause = true;     // will be overwritten to false in the next loop

    digitalWrite(LED_PIN, blinkOn);
    blinkOn = !blinkOn;
}

A few general remarks to your code:

  • The 'Timer' interface is depreciated and will be removed in one of the next versions of the library. Better to use 'PeriodicTimer' and 'OneShotTimer' instead. (library docu: https://github.com/luni64/TeensyTimerTool/wiki)
  • Since pause/_pause is boolean, assignment is atomic and there is no need to disable interrupts
  • You set up timer1 to call tickloop at 1MHz. A comment in tickloop says "does lost of other things". This is calling for trouble. Obviously the other things should be ready in less than 1µs. Additionally the interrupt system is not very efficient. So, it will probably overload if you spend more than a couple of 100ns in the ISR.

As an example, this is how I would code this:
Code:
#include "TeensyTimerTool.h"
using namespace TeensyTimerTool;

PeriodicTimer timer1(TMR1); // First channel on TMR1 aka QUAD timer module. (TMR1 - TMR4, four 16bit channels each)
PeriodicTimer timer2(GPT1); 

void setup()
{
    pinMode(LED_BUILTIN, OUTPUT);

    timer1.begin(tickLoop, 1);
    timer2.begin(foo, 8000);
}


volatile boolean waitFlag;

void loop()
{
    waitFlag = true;
    while (waitFlag){}

    digitalToggle(LED_BUILTIN);
}

void foo()
{
    waitFlag = false;
}

void tickLoop()
{
    // does lost of other things
}
 
Last edited:
thanks. digitalToggle() was a new one for me. I updated the the teens4 library and there it was.

I'll look at the timing lengths of tickLoop() that may be what got me in trouble in the first place.

preciate it.
 
Status
Not open for further replies.
Back
Top