On 7/3/2012 5:48 PM, Niklas Holsti wrote:> On 12-07-03 18:01 , alb wrote: >> On 7/3/2012 1:16 PM, alb wrote: >> [...] >>>> The fifo is shared between main and the ISR. You are not preventing the >>>> ISR triggering while inserting more data into the fifo. This would >>>> result in a race condition. >>> >>> the main() is only writing to the fifo and the isr_sport() only reading >>> - except at the beginning of transmission - so why this should result in >>> a race condition? >>> >>> In the write_to_fifo() I check if the fifo is full and if not I then >>> write to it, incrementing the write pointer. In the read_from_fifo() I >>> check if the fifo is empty and if not read from it, incrementing the >>> read pointer. Maybe I'm missing something here. >>> >> >> I think I was really missing something here!!! These are the >> write_to_fifo() and read_from_fifo() functions: > > [ elided code ] > > Without analysing that code in detail, at first glance it does look as > if it needs to be protected against concurrent access from several > threads/tasks/ISRs.I realized that. Here I can only think about disabling the interrupt temporarily, since interrupts will be still latched and servicing resumed on interrupt enabling. Otherwise I would need to invent some 'locking' mechanism with possibility to postpone a write/read to/from the fifo.> >> where fifo has the following structure: >> >>> typedef struct { >>> int8 bf[FIFO_LENGTH]; >>> int16 wr; // write pointer >>> int16 rd; // read pointer >>> bool ef; // fifo empty/full flag >>> } _fifo; >> >> The race condition is on the ef flag, since both read_from_fifo() and >> write_from_fifo() will set it to either FULL or empty. > > How can a "bool" variable have the values FULL or EMPTY? That > contradicts the meaning of "bool". (Yes, I know that "bool" is just a > fiction in traditional C, but that does not make it logically correct :-))A fifo is fully described with a read pointer, a write pointer, the size and a bit to distinguish EMPTY/FULL when pointers are the same. I do not see any logical flow here.> > And what about the case when the FIFO is neither FULL, nor EMPTY, say it > is about half-full?if (wr - rd != 0) fifo is neither empty nor full.>> I suddenly realize I'm lacking of a methodology to write code which is >> interruptable, maybe some of you can point me to some good reference. > > The traditional way, with mutually exclusive critical sections: > http://en.wikipedia.org/wiki/Mutual_exclusion. > > The new way (mainly useful for multicore processors IMO), with lock-free > data structures: http://en.wikipedia.org/wiki/Lock-free. >Thanks for the pointers!> Note that both methods need a known ordering of actions on variables, so > they need "volatile", or something like that (plus, in the more complex > processors, memory barriers, etc.) If you were using an RT kernel, it > would provide some of this stuff, usually at least interrupt-safe FIFO > queues. >Certainly an RTOS will facilitate some of these stuff, but I'm afraid I will bump in several other problems, considering that there's very little activity on this type of DSP (after all is a legacy DSP).
[cross-post] nested interrupts
Started by ●July 1, 2012
Reply by ●July 4, 20122012-07-04
Reply by ●July 4, 20122012-07-04
On 7/4/2012 8:39 AM, #! wrote: [...]>>>> bool write_to_fifo(int8 d) { >>>> >>>> if ((fifo->wr != fifo->rd) || fifo->ef == EMPTY) { >>>> fifo->bf[fifo->wr] = d; >>>> fifo->wr = MOD(fifo->wr+1, DIM(fifo->bf)); >>>> if (fifo->wr == fifo->rd) fifo->ef = FULL; >>>> return TRUE; >>>> } >>>> >>>> return FALSE; >>>> } >>>> >>> >>>> bool read_from_fifo(int8 *d) { >>>> >>>> if ((fifo->wr != fifo->rd) || fifo->ef == FULL) { >>>> *d = fifo->bf[fifo->rd]; >>>> fifo->rd = MOD(fifo->rd+1, DIM(fifo->bf)); >>>> if (fifo->rd == f->wr) fifo->ef = EMPTY; >>>> return TRUE; >>>> } >>>> >>>> return FALSE; >>>> } >>>> >>> >>> where fifo has the following structure: >>> >>>> typedef struct { >>>> int8 bf[FIFO_LENGTH]; >>>> int16 wr; // write pointer >>>> int16 rd; // read pointer >>>> bool ef; // fifo empty/full flag >>>> } _fifo;[...]> Just looked at your code > > An interrupt could change the value of rd while evaluating this line > if (fifo->wr == fifo->rd) fifo->ef = FULL; > > The value of wr may be partly incremented when an interrupt tries to > evaluate this line > if (fifo->rd == f->wr) fifo->ef = EMPTY;Yep, I also noticed that, even though I do not understand how this problem can produce the effect I see. If the pointers are moved while reading or writing the flag will be wrong. But in either case if the flag is wrong the fifo will be wrongly interpreted as full while is empty or viceversa, meaning data transmitted will be wrong... but they would still be transmitted. I will nevertheless disable the interrupts and reenable inside write_to_fifo() and read_from_fifo() and see what will happen.
Reply by ●July 4, 20122012-07-04
On 2012-07-04, Meindert Sprang <ms@NOJUNKcustomORSPAMware.nl> wrote:> The code above is flawed. Your main should not write to the sport at all. If > no transmission is in progress, the TX interrupt of sport should be > disabled. If you write something to the fifo, you just enable the TX > interrupt, nothing more. At that time, the sport TX buffers are empty and an > interrupt will occur immediately, reading a byte from the fifo and writing > it to sport. When the next interrupt occurs, the next byte will be read from > the fifo and written to the sport. If the fifo is empty when an interrupt > occurs, the interrupt handler should disable the TX interrupt and exit.Exactly. And you do not need "fifo empty/full" flags. If write_pointer == read_pointer, fifo is empty. If write_pointer+1 (wrapping at end of buffer) == read_pointer, fifo is full. This way the main program only alters write_pointer (and buffer contents) and the irs code only alters read_pointer and there are no race conditions, ever. Simple. -jm
Reply by ●July 4, 20122012-07-04
On 7/4/2012 9:12 AM, Meindert Sprang wrote: [...]>> >>> int main() { >>> ... >>> while(1) { >>> write_to_fifo(data); >>> if (state != TRANSMITTING) { >>> if (read_from_fifo(&byte) == OK) { >>> write_to_sport(byte); >>> state = TRANSMITTING; >>> } >>> } >>> } >>> } >>[...]> The code above is flawed. Your main should not write to the sport at all. If > no transmission is in progress, the TX interrupt of sport should be > disabled. If you write something to the fifo, you just enable the TX > interrupt, nothing more. At that time, the sport TX buffers are empty and an > interrupt will occur immediately, reading a byte from the fifo and writing > it to sport.Uhm that is interesting, but since my interrupt is edge sensitive I do not believe that when I enable it the interrupt will be seen immediately. I may change the interrupt to level sensitive, but I'm not sure if it is beneficial. Nevertheless I don't see how this change will make me understand why the nested interrupts break my code. When the next interrupt occurs, the next byte will be read from> the fifo and written to the sport. If the fifo is empty when an interrupt > occurs, the interrupt handler should disable the TX interrupt and exit. >Your proposal is certainly valid, but it does not help me understanding what I'm doing wrong. Other people have correctly pointed out several flaws in my code but the logic of 'starting' the transmission in main() is not flawed 'per se', it is maybe wrongly implemented.
Reply by ●July 4, 20122012-07-04
On 7/4/2012 11:19 AM, Jukka Marin wrote:> On 2012-07-04, Meindert Sprang <ms@NOJUNKcustomORSPAMware.nl> wrote: >> The code above is flawed. Your main should not write to the sport at all. If >> no transmission is in progress, the TX interrupt of sport should be >> disabled. If you write something to the fifo, you just enable the TX >> interrupt, nothing more. At that time, the sport TX buffers are empty and an >> interrupt will occur immediately, reading a byte from the fifo and writing >> it to sport. When the next interrupt occurs, the next byte will be read from >> the fifo and written to the sport. If the fifo is empty when an interrupt >> occurs, the interrupt handler should disable the TX interrupt and exit. > > Exactly. And you do not need "fifo empty/full" flags. If write_pointer == > read_pointer, fifo is empty. If write_pointer+1 (wrapping at end of buffer) > == read_pointer, fifo is full. This way the main program only alters > write_pointer (and buffer contents) and the irs code only alters read_pointer > and there are no race conditions, ever.You are suggesting to remove the flag and keep one memory location of the fifo not used (if I understood correctly). This will prevent the race condition on the flag, it will still not prevent the race on 'state'. And I should remind you that my problem is why nested interrupts will break my code w.r.t. non nested interrupts. It is still true that I have to prevent fifo to be misused but this will affect only the data being wrong (fifo will be interpreted as full while is empty), not the logic on the transmission.
Reply by ●July 4, 20122012-07-04
On 2012-07-04, alb <alessandro.basili@cern.ch> wrote:>> Exactly. And you do not need "fifo empty/full" flags. If write_pointer == >> read_pointer, fifo is empty. If write_pointer+1 (wrapping at end of buffer) >> == read_pointer, fifo is full. This way the main program only alters >> write_pointer (and buffer contents) and the irs code only alters read_pointer >> and there are no race conditions, ever. > > You are suggesting to remove the flag and keep one memory location of > the fifo not used (if I understood correctly).Yes.> This will prevent the > race condition on the flag, it will still not prevent the race on 'state'.Yes, it does. You update the read and write pointers after you have read or written the buffer. No race.> And I should remind you that my problem is why nested interrupts will > break my code w.r.t. non nested interrupts. It is still true that I have > to prevent fifo to be misused but this will affect only the data being > wrong (fifo will be interpreted as full while is empty), not the logic > on the transmission.I'm not sure it's the nested interrupts that's the problem but the race problems in your fifo/isr code. (I'm also not sure if you need nested interrupts at all. ;-) -jm
Reply by ●July 4, 20122012-07-04
On 04/07/12 21:19, alb wrote:> On 7/4/2012 8:39 AM, #! wrote: > [...] >>>>> bool write_to_fifo(int8 d) { >>>>> >>>>> if ((fifo->wr != fifo->rd) || fifo->ef == EMPTY) { >>>>> fifo->bf[fifo->wr] = d; >>>>> fifo->wr = MOD(fifo->wr+1, DIM(fifo->bf)); >>>>> if (fifo->wr == fifo->rd) fifo->ef = FULL; >>>>> return TRUE; >>>>> } >>>>> >>>>> return FALSE; >>>>> } >>>>> >>>> >>>>> bool read_from_fifo(int8 *d) { >>>>> >>>>> if ((fifo->wr != fifo->rd) || fifo->ef == FULL) { >>>>> *d = fifo->bf[fifo->rd]; >>>>> fifo->rd = MOD(fifo->rd+1, DIM(fifo->bf)); >>>>> if (fifo->rd == f->wr) fifo->ef = EMPTY; >>>>> return TRUE; >>>>> } >>>>> >>>>> return FALSE; >>>>> } >>>>> >>>> >>>> where fifo has the following structure: >>>> >>>>> typedef struct { >>>>> int8 bf[FIFO_LENGTH]; >>>>> int16 wr; // write pointer >>>>> int16 rd; // read pointer >>>>> bool ef; // fifo empty/full flag >>>>> } _fifo; > [...] > >> Just looked at your code >> >> An interrupt could change the value of rd while evaluating this line >> if (fifo->wr == fifo->rd) fifo->ef = FULL; >> >> The value of wr may be partly incremented when an interrupt tries to >> evaluate this line >> if (fifo->rd == f->wr) fifo->ef = EMPTY; > > Yep, I also noticed that, even though I do not understand how this > problem can produce the effect I see. > If the pointers are moved while reading or writing the flag will be > wrong. But in either case if the flag is wrong the fifo will be wrongly > interpreted as full while is empty or viceversa, meaning data > transmitted will be wrong... but they would still be transmitted. > > I will nevertheless disable the interrupts and reenable inside > write_to_fifo() and read_from_fifo() and see what will happen. >To understand, you will need to look at the machine code and work out what happens when an interrupt occurs at every position in the critical section of code, with every possible combination of rd and wr As Niklas has suggested maybe the extra interrupts changed the timing, causing you luck to run out.
Reply by ●July 4, 20122012-07-04
"Rocky" <robertgush@gmail.com> wrote in message news:55d65d0a-6660-4cca-827e-4f23ac1cf93b@q29g2000vby.googlegroups.com... On Jul 4, 9:12 am, "Meindert Sprang" <m...@NOJUNKcustomORSPAMware.nl> wrote:> - Show quoted text - > This is not true for all processors. Some of them have edge triggered > interrupts the only interrupt when the transmitter finishes > transmission.What would be the advantage of having an edge triggered interrupt on a UART empty event? Do you have an example of this? Meindert
Reply by ●July 4, 20122012-07-04
"alb" <alessandro.basili@cern.ch> wrote in message news:a5igkjFb0oU1@mid.individual.net...> On 7/4/2012 9:12 AM, Meindert Sprang wrote: > [...] > >> > >>> int main() { > >>> ... > >>> while(1) { > >>> write_to_fifo(data); > >>> if (state != TRANSMITTING) { > >>> if (read_from_fifo(&byte) == OK) { > >>> write_to_sport(byte); > >>> state = TRANSMITTING; > >>> } > >>> } > >>> } > >>> } > >> > [...] > > > The code above is flawed. Your main should not write to the sport atall. If> > no transmission is in progress, the TX interrupt of sport should be > > disabled. If you write something to the fifo, you just enable the TX > > interrupt, nothing more. At that time, the sport TX buffers are emptyand an> > interrupt will occur immediately, reading a byte from the fifo andwriting> > it to sport. > > Uhm that is interesting, but since my interrupt is edge sensitive I do > not believe that when I enable it the interrupt will be seen immediately. > I may change the interrupt to level sensitive, but I'm not sure if it is > beneficial.Yes it is because then you can use the procedure I outlined. And this is not pure theoretical, I use this in a commercial product. And I have no race conditions and use nested interrupts.> Your proposal is certainly valid, but it does not help me understanding > what I'm doing wrong. Other people have correctly pointed out several > flaws in my code but the logic of 'starting' the transmission in main() > is not flawed 'per se', it is maybe wrongly implemented.It sure makes life a lot easier if you don't write to the same hardware in main code AND in an interrupt. Meindert
Reply by ●July 4, 20122012-07-04
On Jul 4, 1:14�pm, "Meindert Sprang" <m...@NOJUNKcustomORSPAMware.nl> wrote:> "Rocky" <robertg...@gmail.com> wrote in message > > news:55d65d0a-6660-4cca-827e-4f23ac1cf93b@q29g2000vby.googlegroups.com... > On Jul 4, 9:12 am, "Meindert Sprang" <m...@NOJUNKcustomORSPAMware.nl> > wrote: > > > - Show quoted text - > > This is not true for all processors. Some of them have edge triggered > > interrupts the only interrupt when the transmitter finishes > > transmission. > > What would be the advantage of having an edge triggered interrupt on a UART > empty event? Do you have an example of this? > > Meindert8032. Bit of a pain, but once the code is in place it is OK. IIRC the Z80 SIO had a similar issue. The scheme in the PIC for example of disabling the specific interrupt is much easier to use because you just enable the interrupt after adding data to the circular buffer. It doesn't matter if was enabled or not when you re-enable. Worst case is a gratuitous interrupt.
