Problem with interrupt and interrupt counter

[sebasira]

Hello everybody!

Well, I didn't know how to name this topic and I guess this one fit the most.

I've got a program wich receive data from SCI in interrupt mode. Everytime it receive a byte, a counter is increased (interrupt counter). This counter helps me track down how many bytes I've received and were not processed yet.

Then, in the main loop, if the counter is greater than zero, it means that a new byte (or bytes) need to be processed. So after I've done with it, I decrease the counter. The problem I was having (the symptom) was that there were bytes left to be processed and the counter has already reach zero. I was burning my head until I figured out what was happening.

I look at the asm generated code for the 'counter--' instruction, and it was something like this:

word counter;

counter--;

MOVE.W     1902(A5), D0         gets counter value and store it in D0

SUBQ.L     #1, D0               decrease D0 (counter value)

MOVE.W     D0,1902(A5)          update counter value

Well, I think the problem arise when I receive a byte after the counter value is copied in D0, so the counter is increased in the IRQ, but when it's updated from D0 it loses the received byte count.

I've been working with this approach for several years and never had a problem. But I always use it in 16bits processors, and now I'm working with a 32bits one (MCF51QE). Maybe that's the problem and counter should be 32bits long so asm operation is done in a single instruction? (I haven't tryed this yet and I don't know the answer for this question for sure, so please could you answer it?)Or should I 'do' anything else so the operation is done in an atomic instruction?

I know I can use another approach such as counting how many bytes I've received and how many bytes I've processed independently and if the difference is not zero then some bytes need to be taken care of. But if possible, I'd like to continue with the first approach in order to reduce code update.

Thanks in advance!

Best Regards,

SebaS

Mensaje editado por: Sebastian M. Irazabal  Well, I've declared counter as 32 bits long and now the decrease instruction takes place on a single instruction

[JimDon]

Are interrupts disabled when you do the above code?

If not you have a problem. If the interrupt happens after you have decrement but before the save, you will over write the increment done in the interrupt.

[sebasira]

Yes, that was the problem, now that I've change counter to be 32bits long, it doesn't anymore, because decrement is done on a single asm-step.

My question was, if there is another way to overcome this without changing the size of counter and using the same approach. Disabling interrupts is not an option.

[TomE]

You're using that counter as a counting semaphore. This is something that all code using interrupts has to get right. Real basic stuff. The following is a "general introduction" to the problem:

Semaphore (programming) - Wikipedia, the free encyclopedia

The following section says that disabling interrupts is a standard practice (I'll get back to this):

Real-time operating system - Wikipedia, the free encyclopedia

The different threads (mainline and interrupt in this case) need exclusive access to that counter/semaphore.

That can either be provided by the hardware if it allows "atomic access" (in this case, atomic decrement). The ColdFire chip's SUB and SUBQ will both work with a memory location as the destination, so can support this (unlike chips such as the Power ones, which can only load and store memory).

As you've found, this will work as long as the compiler selects one of these instructions, but there's no guarantee of that. The compiler is free to keep that counter in a register for a while if the optimiser wants to. You may be able to wave a "volatile" at the compiler, but that doesn't really guarantee anything. "counter--" may turn into a load/subtract/store, just like it did when you declared it as 16 bits.

The only safe ways to write this code (so it works and keeps working) are:

1 - Surround the mainline decrement with interrupt disable and enables. That is the standard/normal approach.

2 - Perform the subtraction with inline assembly to force the atomic form of the instruction.

3 - Perform the subtraction in a subroutine written in assembly that uses the right instruction.

Another option is to read up on the "TAS" instruction, which is specifically meant for implementing semaphores, but that's overkill in this case, and would probably cause other problems. The 68000 (which the ColdFire derives from) had CAS and CAS2, which were better suited for this, but the ColdFire didn't get these instructions. Anyway, in your case the SUB does what you need.

> Disabling interrupts is not an option.

Why? Is your serial port code running in "user mode" instead of "kernel mode" or something? You're running on a MCF51QE, so it isn't like you're running Linux or anything. The "Disable/Decrement/Enable" sequence is extremely quick. Why can't you use this, or why don't you think you can use this?

Tom

[sebasira]

Thanks Tom!

You've opened my mind! Well, about disable interrupt, you're right... The time spent is insignificant, and no data is lost. And as you state, is the only safe way.

I'd like to solve this as you suggested. But i'm not familiar with ColdFire set of instructions and I'm lost here. I'd like the counter to remain 16bits long (it could even be 8bits). How would I do the following with ColdFire instruction set? This is how I guess it would be, but I don't know how to save/restore registers.

DisableInterrupts;

asm{

     ¿?Push D0 (or any other)

     MOVE.W          counter, D0               Load counter on that register

     SUBQ.L              #1, D0                    Decrement D0

     MOVE.W          D0, counter               Update counter

     ¿? Pull D0

};

EnableInterrupts;

Or maybe SUBQ.L  #1, counter but as I said before, I don't get well with this instruction set.

While I was doing some test, I noticed that sometime the problem gets worst, because I did:

if (counter > 0){

     counter--;

}

And in ASM, it first load counter into D0 and 10 instructions after it decrements!!! So the chances that an interrupt occurs between those instructions was even bigger. That was what encourage me to resolve it in the way you expressed.

SebaS

Mensaje editado por: Sebastian M. Irazabal:

This is how I solve it. Please correct me if I'm wrong

DisableInterrupts;

asm{

      SUBQ.L              #1, counter

};

EnableInterrupts;

[TomE · Accepted solution]

> This is how I solve it. Please correct me if I'm wrong

You've actually fixed it twice in that example!

Either:

DisableInterrupts;

counter--;

EnableInterrupts;

or

asm ( SUBQ.L  #1, counter };

will work. The first one allows you to have a 16 bit counter, the latter requires 32, but 16 bit arithmetic is slower on this CPU. It takes more instructions than you're saving in storage to use 16 bits. The first one is more "standard". You should avoid inline assembly unless absolutely necessary.

> You've opened my mind!

Here's another important point. The older CPUs you're familiar with have Enable and Disable interrupt instructions. This CPU has seven different interrupt levels (so high priority interrupts can interrupt lower priority ones). It is also very good practice to write code and functions that can be called either from mainline or from interrupt routines without causing problems. If you disable then enable interrupts by accident within an interrupt service routine, it all goes bad very fast. Everyone gets this wrong the first time - that's how you learn about how to do it properly.

What you want to do is have functions that disable interrupts and then RESTORE them to what they were before you disabled them. You also want to be able to call functions with interrupts disabled, where those functions then disable interrupts, but don't enable them on exit. You want the disable/enable calls to nest or stack properly. Easy.

We're using GCC and the library and headers that came with it (from Freescale) provided the function "asm_set_ipl()". You should check your toolchain to see if it provides this function or an equivalent with a different name.

First match in Google for "mcf5xxx.S" finds:

http://powerlinkstm32.googlecode.com/svn/trunk/MN/openPOWERLINK_v1.6/Target/PLCcore-CF54/no_os/gnu/c...

Read the above and find asm_set_ipl() and read the comments. The way to use this is:

uint32_t old_ipl = asm_set_ipl(7);  /* Disable and save current level */

... Do whatever needs to be done with interrupts disabled ...

asm_set_ipl(old_ipl);  /* Restore previous level */

Search these forums for "asm_set_ipl()" and see if any of them relate to the what you're using (I assume CodeWarrior), like:

Support files for MCF5225X and Connector for TWR system

Tom

[sebasira]

Thanks Tom, for sharing your knowledge!

> You've actually fixed it twice in that example!

Actually, I needed to do this way, because prior to counter-- there's a comparision and when I decrement the counter, it doesn't touch the counter itself, but the copied value in D0:

if (counter > 0){

     DisableInterrupts;

     counter--;

     EnableInterrupts;

}

I don't remember exactly the ASM generated code, but it was something like:

MOVE.L     counter, D0

TST.L          D0

BNE            xxxxxxxxxx

DisableInterrupts

SUBQ.L      #1, D0

MOVE.L     D0, counter

EnableInterrupts

So even if I disable/enable interrupts, counter value was previously loaded into D0 and modified from there, so there's a chance a new byte arrives just before disabling interrupts and I'll end up in the same place I was before. That's why I do it this way. Although now that the decrement is in only one instruction (SUBQ.L  #1, counter), there's no need of disabling interrupts.

> Here's another important point. The older CPUs you're familiar with have Enable and Disable interrupt instructions. This CPU has seven different interrupt levels (so high priority interrupts can interrupt lower priority ones). It is also very good practice to write code and functions that can be called either from mainline or from interrupt routines without causing problems. If you disable then enable interrupts by accident within an interrupt service routine, it all goes bad very fast. Everyone gets this wrong the first time - that's how you learn about how to do it properly.

Indeed, I didn't know about this. Moreover, I was looking for the I bit to disable interrupts as in HCS12 and couldn't find them. I can see that there was an Int Mask value but didn't know that this value meant priorities.

If I understood correctly, what you're saying is that if I disable/enable this way I'm disabling/enabling ALL interrupts and maybe, I don't want that to happen, because I could be working with priorities and have only enabled HIGH priority interrupts and after that, I'll have LOW priority un-masked again, right?

asm_set_ipl() routine seems very easy to use, just get previous value, and on exit reestablish that value.

Thanks again. Have a nice day!

SebaS

[sebasira]

Tom, et me ask you one more question....

About the asm_set_ipl routine.... How should I add it to my porject?

The file you link is .s and I don't know anything about them

Thanks!

[TomE]

> How should I add it to my porject?

I don't know. I use CodeSourcery, gcc and "make".

See if there's any documentation saying where you should add your own assembly code routines. CodeWarrior may "prefer" to use inline assembly.

It might be better to ask this question in a CodeWarrior Forum, after searching to see if this has been answered. I've just done this and found:

https://community.freescale.com/docs/DOC-93314

CodeWarrior should already include an equivalent to "asm_set_ipl()" but it might be called something different. It isn't impossible to write reliable code without this function, but I've never seen it done and have seen lots of very bad code written without this function.

Tom

.