views:

1038

answers:

10

Hello,

This question is about programming small microcontrollers without an OS. In particular, I'm interested in PICs at the moment, but the question is general.

I've seen several times the following pattern for keeping time:

Timer interrupt code (say the timer fires every second):

...
if (sec_counter > 0)
  sec_counter--;
...

Mainline code (non-interrupt):

sec_counter = 500; // 500 seconds

while (sec_counter)
{
  // .. do stuff
}

The mainline code may repeat, set the counter to various values (not just seconds) and so on.

It seems to me there's a race condition here when the assignment to sec_counter in the mainline code isn't atomic. For example, in PIC18 the assignment is translated to 4 ASM statements (loading each byte at the time and selecting the right byte from the memory bank before that). If the interrupt code comes in the middle of this, the final value may be corrupted.

Curiously, if the value assigned is less than 256, the assignment is atomic, so there's no problem.

Am I right about this problem? What patterns do you use to implement such behavior correctly? I see several options:

  • Disable interrupts before each assignment to sec_counter and enable after - this isn't pretty
  • Don't use an interrupt, but a separate timer which is started and then polled. This is clean, but uses up a whole timer (in the previous case the 1-sec firing timer can be used for other purposes as well).

Any other ideas?

+1  A: 

Write the value then check that it is the value required would seem to be the simplest alternative.

do {
 sec_counter = value;
} while (sec_counter != value);

BTW you should make the variable volatile if using C.

If you need to read the value then you can read it twice.

do {
    value = sec_counter;
} while (value != sec_counter);
Dipstick
Interesting approach, though doubtedly cleaner than disabling interrupts. I'm not sure I need volatile since compiler optimizations are disabled anyway.
Eli Bendersky
I'd hope that this techique is commented when you use it because its going to look awfully weird to someone who doesn't immediately realises that its guarding agaist interrupts changing the value within one "C instruction".
Martin
It also works when reading from multiple memory mapped registers where disabling interrupts does not help.
Dipstick
You definitely need volatile unless you create one function to change the value of the sec_counter.
Stephen Friederichs
I'd wrap this in macro like this#define safesettimer(timervar, val) do (timervar) = (val); while ((timervar)!=(val))but the disadvantage over disabling interrupts is that this is 11 instructions, compared to the original 5 it took just to set the 16 bit value, and 7 if you disable then re-enable the interrupts.
Martin
I wouldn't actually use this mechanism in the circumstances described but have used similar in multi-processor environments for reading/writing data in shared memory. eliben did ask "Any other ideas?" - this is one I thought worth knowing. I am not familiar with the PIC18, but it is probably worth noting that some CPUs need to stall for several cycles - or you need to add several no-ops - after disabling interrupts before there is any certainty that an interrupt will not occur. Therefore instruction counting can be a bit simplistic.
Dipstick
A: 

Well, what does the comparison assembly code look like?

Taken to account that it counts downwards, and that it's just a zero compare, it should be safe if it first checks the MSB, then the LSB. There could be corruption, but it doesn't really matter if it comes in the middle between 0x100 and 0xff and the corrupted compare value is 0x1ff.

kotlinski
the problem is not with the compare, I think. it's that both the mainline code and the ISR write into the same variable
Eli Bendersky
+1  A: 

You definitely need to disable the interrupt before setting the counter. Ugly as it may be, it is necessary. It is a good practice to ALWAYS disable the interrupt before configuring hardware registers or software variables affecting the ISR method. If you are writing in C, you should consider all operations as non-atomic. If you find that you have to look at the generated assembly too many times, then it may be better to abandon C and program in assembly. In my experience, this is rarely the case.

Regarding the issue discussed, this is what I suggest:

ISR:
if (countDownFlag)
{
   sec_counter--;
}

and setting the counter:

// make sure the countdown isn't running
sec_counter = 500;
countDownFlag = true;

...
// Countdown finished
countDownFlag = false;

You need an extra variable and is better to wrap everything in a function:

void startCountDown(int startValue)
{
    sec_counter = 500;
    countDownFlag = true;
}

This way you abstract the starting method (and hide ugliness if needed). For example you can easily change it to start a hardware timer without affecting the callers of the method.

kgiannakakis
why do I need both the flag and the disable of interrupts? or is this an alternative?
Eli Bendersky
You don't need both the flag and disable the interrupts. One of them is enough. If you use the flag, make sure that countDownFlag == false, before setting the counter. Basically this is what you need to do: 1) stop the countdown, 2) set the counter, 3) start the countdown
kgiannakakis
however, as this is a clock, it will loose ISR down-ticks... in a non deterministic fashion... ugh
jpinto3912
The count down starts when you instruct it to do so. It doesn't run all the time (at least this is how I understand the question). Using the flag will allow you to run the ISR (perhaps shared with some other job), while still not allowing the count down.
kgiannakakis
A: 

The way you are using your timer now, it won't count whole seconds anyway, because you might change it in the middle of a cycle. So, if you don't care about it. The best way, in my opinion, would be to read the value, and then just compare the difference. It takes a couple of OPs more, but has no multi-threading problems.(Since the timer has priority)

If you are more strict about the time value, I would automatically disable the timer once it counts down to 0, and clear the internal counter of the timer and activate once you need it.

SurDin
sorry, I'm not sure I follow your answer. I have no multi-threading and multi-core here, just a small PIC18 running bare-on-metal code. Also, timing *is* strict to a reasonable level.
Eli Bendersky
1. Using interrupts is a simple version of having 2 threads, you also use the same terminology in your question. You have the mainloop thread, and you have the interrupt thread, you need to context switch when you go to the interrupt(saving bank registers, and W value, etc.). You have a problem of moving to the interrupt, but if you avoid writing to the counter in the main program, the problem disappears.2. It is correct up to 2*{timer length}. If you need more you need to reset the timer internal count.
SurDin
+2  A: 

The PIC architecture is as atomic as it gets. It ensures that all read-modify-write operations to a memory file are 'atomic'. Although it takes 4-clocks to perform the entire read-modify-write, all 4-clocks are consumed in a single instruction and the next instruction uses the next 4-clock cycle. It is the way that the pipeline works. In 8-clocks, two instructions are in the pipeline.

If the value is larger than 8-bit, it becomes an issue as the PIC is an 8-bit machine and larger operands are handled in multiple instructions. That will introduce atomic issues.

sybreon
That's not what was said. He was talking about the fact that it has 2 bytes, and hence takes 4 operations:movlwmovwfmovlwmovwf
SurDin
This aligns with my experience that no problems result when 8-bit values are written into the counter. The problems begin with 16-bit values, because those take two separate write cycles - one to each of the HI and LO parts of the word.
Eli Bendersky
Since this is PIC specific - ask it on the piclist. I'm sure that you'll get the answer you want.
sybreon
A: 

Move the code portion that would be on the main() to a proper function, and have it conditionally called by the ISR.

Also, to avoid any sort of delaying or missing ticks, choose this timer ISR to be a high-prio interrupt (the PIC18 has two levels).

jpinto3912
+1  A: 

Because accesses to the sec_counter variable are not atomic, there's really no way to avoid disabling interrupts before accessing this variable in your mainline code and restoring interrupt state after the access if you want deterministic behavior. This would probably be a better choice than dedicating a HW timer for this task (unless you have a surplus of timers, in which case you might as well use one).

Lance Richardson
+1  A: 

If you download Microchip's free TCP/IP Stack there are routines in there that use a timer overflow to keep track of elapsed time. Specifically "tick.c" and "tick.h". Just copy those files over to your project.

Inside those files you can see how they do it.

Robert
yep, they basically disable the interrupt and reenable it, when accessing data the ISR can change
Eli Bendersky
+1  A: 

It's not so curious about the less than 256 moves being atomic - moving an 8 bit value is one opcode so that's as atomic as you get.

The best solution on such a microcontroller as the PIC is to disable interrupts before you change the timer value. You can even check the value of the interrupt flag when you change the variable in the main loop and handle it if you want. Make it a function that changes the value of the variable and you could even call it from the ISR as well.

Stephen Friederichs
The best solution is to take a PIC with RTC. I don't know if there are any in the 8-bit series, but there are in the 16-bit series.
Marco van de Voort
Are there any PICs with RTC chips that don't require one to do silly BCD conversions and other nonsense to read the time?
supercat
A: 

One approach is to have an interrupt keep a byte variable, and have something else which gets called at least once every 256 times the counter is hit; do something like:

// ub==unsigned char; ui==unsigned int; ul==unsigned long
ub now_ctr; // This one is hit by the interrupt
ub prev_ctr;
ul big_ctr;

void poll_counter(void)
{
  ub delta_ctr;

  delta_ctr = (ub)(now_ctr-prev_ctr);
  big_ctr += delta_ctr;
  prev_ctr += delta_ctr;
}

A slight variation, if you don't mind forcing the interrupt's counter to stay in sync with the LSB of your big counter:

ul big_ctr;
void poll_counter(void)
{
  big_ctr += (ub)(now_ctr - big_ctr);
}
supercat