Hey - I'm using a Teensy 3.0 and my program is crashing due to a while loop, which is embedded in a for loop, which is located in an interrupt using IntervalTimer (Loglows timer from here: GitHub - loglow/IntervalTimer: Timer library for Teensy 3.0).
I'm running a timer which calls the subroutine ps1_pulse1, described below. The timer and called subroutine run fine, except when I include this while statement (see end of code line). It causes the Teensy to lock up. Notice that I call basically the same thing above (ie while (start2<micros())).. If I take the while statement out of the if conditional statement, it works fine. I can leave it in the if conditional if I change the value to some fixed value (like 10,000 or something) - works fine also. I tried putting it in an if statement inside a while statement (ugly) just to see if it would work when not part of the conditional, but it failed there as well. I also checked the timing on my counters (start1, start2, start3) and they are what I would expect, so the while statement should work based on the values of micros() and start3. I also tried setting micros() as an unsigned long just for fun, but that didn't help either.
I can't seem to get this figured out... any ideas?
void ps1_pulse1() {
start1 = micros();
digitalWriteFast(ps1_meas_light, HIGH); // turn on the light
if (pulse == 0) {
if (ps1_act[cycle] == 2) {
digitalWriteFast(ps1_act_light, LOW);
// Serial.print("light off");
}
else {
digitalWriteFast(actiniclight_intensity_switch, ps1_act[cycle]);
digitalWriteFast(ps1_act_light, HIGH);
// Serial.print("light on");
}
digitalWriteFast(ps1_alt1_light, ps1_alt1[cycle]);
digitalWriteFast(ps1_alt2_light, ps1_alt2[cycle]);
digitalWriteFast(ps1_red_light, ps1_red[cycle]);
}
data1 = analogRead(detector1);
start2=start1+ps1_pulsesize;
while (micros()<start2) {} // wait 50ms
digitalWriteFast(ps1_meas_light, LOW); // then turn off the light
start3 = start1+ps1_pulsedistance[cycle][1]; // set a timepoint 3000us after start1
Serial.print(","); // these Serial.print statements is me just trying to see how big these values are, and make sure they are what I would expect them to be... and they are.
Serial.print(start1); // example value might be 10,000,000
Serial.print(",");
Serial.print(start2); // 50ms later than start1
Serial.print(",");
Serial.print(start3); // 3000ms later than start1
Serial.print(",");
Serial.print(micros());
Serial.print(",");
if (pulse == 0) {
while (start3>micros()) {} // PROBLEM STATEMENT! Even though I call basically the same thing above (ie while (start2<micros())).... this one fails. If I take it out of the while statement, it works fine. I tried putting it in an if statement inside a while statement (ugly) just to see if it would work when not part of the conditional, but it failed there as well.
// .. do more stuff here
I'm really fed up keep having to say this but POST YOUR CODE.
Read the guidelines.
Don't do serial I/O in interrupt context, listen to your mother, wear sunscreen.
Aaarrgh
start2=start1+ps1_pulsesize;
while (micros()<start2) {} // wait 50ms
you always need to test the difference of two time values to avoid wrap-around issues:
while (micros() - start1 < ps1_pulsesize) {} // wait 50ms
The subtraction is essential to the test working. Since time values are usually
unsigned long, you cannot test for negative differences either, since the difference
will inherit unsigned-ness.
Thanks Nick for the links - http://www.gammon.com.au/forum/?id=11488 is particularly good, especially the part showing exactly how much time it takes to run an interrupt and the process, as well as the "Hints for writing ISRs" section. Very useful!
and MarkT for addressing the question. That definitely makes sense.
Paul - Ok - so an ISR is not fast... that means... what exactly? It takes longer to execute a line of code? How does that impact this particular case with while and if statements? I read this AVR-like ISR() and it's very useful - thanks. I now realize that I had a major error in how I wanted to link the intervaltimer calls together based on your description which I need to fix.
I know I'm pushing the limit on what you can do in an ISR, but the job requires pretty specific timing and consistency and so far, that has seemed like the right way to go. Any links or tips, like Nick's "Hints for writing ISRs" above, specific to Teensy and ARM would be useful.
AWOL - I read the guidelines before posting. The serial statements in the code are just so I could see how far I was getting before it crashed used only while I was debugging - yes it still crashes even without the serial calls. I avoid using delays by using while loops. I understand the basic rules of interrupts. I didn't post the rest of the code because it's really long and requires a couple different libraries, and I thought it would be distracting from the point, so I included the relevant portion and the necessary details to understand the problem. I've posted the entire code (the mess that it is) as a link in google docs since it doesn't fit (even trimmed down) in the 9500 character maximum limit here. You will probably find a million errors (though the program does run the way I want it to, even if it's written poorly) - I know I have a long way to go to become as efficient a coder as possible, but you've got to start somewhere. If you're interested in the project, you can see more about it at www.photosynq.org, and the github contains older version of the code here GitHub - Photosynq/Protocols-Arduino: Arduino protocols/code for controlling the Photosynq device which I'll update when I have something more stable). I'd love some expert help.
The serial statements in the code are just so I could see how far I was getting before it crashed
You know that serial I/O is interrupt-driven...and interrupts are disabled in interrupt context?
I've posted the entire code (the mess that it is) as a link in google docs since it doesn't fit (even trimmed down) in the 9500 character maximum limit here
gbathree:
Paul - Ok - so an ISR is not fast... that means... what exactly? It takes longer to execute a line of code? How does that impact this particular case with while and if statements?
He didn't say that. He meant your ISR is supposed to get its job over quickly. The code executes at the same speed inside it.
Some things, like Serial printing, which themselves require interrupts to be on, just won't work. Plus even if they did, if the ISR takes more than 1 mS then it will cause other ISRs to miss things like timer interrupts.
A rule of thumb is to keep the ISR minimal. Either set a flag, which you test in the main loop, or do something brief, like remember the current time, eg. if you are timing when a ball passes a sensor.
If you want to time an interval (eg. the time taken for a ball to pass two sensors) you remember the first, leave the ISR, and let the second detect the other interval.
Ah ha, thanks Nick. Sorry Paul - that is clear now.
So I'm trying something new here. Prior to this iteration, my ISR looked like the code below which is much simpler. But the problem with that is I have to save all the data until the end of a measurement, which can be hundreds of ints and takes up too much memory. So instead of saving the data, I'm trying to push it out immediately when it's created, ideally via Serial. I'm also trying to increase the number of user-adjustable options (thus the if statements and such).
Anyway, thanks for the input and advice - I'll try to rethink it and stick to the rules.
Perhaps if you describe what you are trying to do rather than how you are trying to do it.
As soon as I saw your web page about this being a project to measure the behaviour of plants, I revised down my expectation that you needed to do things within a few microseconds.
And apart from what AWOL said about analogRead you have this:
start1=start1+dpulselengthon;
while (micros()<start1) {
}
Which is equivalent to while(1). The function micros() is interrupt based and as such will not count inside another ISR as the interrupt which causes it to count is disabled. In otherwords your program will never complete that while loop and so it will freeze.
I took a look at the analog read code and it uses interrupts. It wait on the conversion complete interrupt so I think it needs adding to the list of things we don't use in an ISR.
// start the conversion
sbi(ADCSRA, ADSC);
// ADSC is cleared when the conversion finishes
while (bit_is_set(ADCSRA, ADSC));
// we have to read ADCL first; doing so locks both ADCL
// and ADCH until ADCH is read. reading ADCL second would
// cause the results of each conversion to be discarded,
// as ADCL and ADCH would be locked when it completed.
low = ADCL;
high = ADCH;
It loops waiting for a bit to be cleared, that's all.
AWOL - The analog read is much shorter on the Teensy. If I change the analog read resolution, I can take a single (unaveraged) read safely in 6us. So in this case, I'm taking 3 reads and averaging them (in the ADC) in ~18us, so it's not too long. The analog read is necessary to be taken during the pulse. I've run this script many times at this point and haven't had issues with it, so while it may not be perfect, it is effective.
Also, note that if there are limits on the pulse length or other stuff (for example, on the number of analog read averages) required to ensure that the ISR works properly, I can integrate them into the user spec to ensure that they are not exceeded (or at least to absolve myself from blame :)).
Nick - Yes... I'd like to get rid of those while statements... Right now I am pulsing the light on and off all within a single ISR and a while loop in between the off and on to get the timing correct. I have considered setting two interval timers, one to pulse the light on and one to pulse it off, but there are some protocols which want this: pulse, wait 3000us, pulse, wait 6000us, repeat - in the previous setup that would require 4 interval timers. And, as always in science, everyone wants as many options as possible to do all kinds of crazy combinations.
... but hmmm as I'm writing this I realize that as long as the pulses were multiples of each other (like 3000 and 6000), then I could just set flags so that the on/off skip every 3rd cycle out of 4... right? If I did this using a 25u pulse and my 3000 delay and 6000 delay case:
1 on @ 0
1 off @ 25
2 on @ 3000
2 off @ 3025
3 off @ 6000 // use counter flag to skip this cycle
3 off @ 6025
4 on @ 9000
4 off @ 9025
This way, I can actually have as many different kinds of pulse combinations combinations that I want, so long as they are all multiples of each other (which is pretty reasonable). AWOL - if I do this, I may also be able to perform the analogRead outside of the loop too, which as you've pointed out would be better...
I'm not sure of the guts of interrupts and while loops, but I can tell you that that particular piece of code you reference works fine inside the ISR, at least on the Teensy.
start1=start1+dpulselengthon;
while (micros()<start1) {
}
Even the timing of the pulses (checked on a scope) is very accurate - the only error is +-1us when the light turns off, which isn't too bad, and because it's all on timers the error doesn't accumulate which is what I need.
