MODSERIAL race condition in receive?

03 Dec 2011

I've run across a problem in MODSERIAL, where there's a backlog of characters (I think they're stuck in the device fifo). My processing loop goes like this:

char* CommandProcessor::GetCommand()
 {
    int c;
   
    while ((c = serial.getcNb()) != -1)
    {
        // serial.putc(c);
        _buf[_bufptr++] = c;
        if ((c == '\n') || (_bufptr == 127))
        {
            _buf[_bufptr] = 0;
            _bufptr = 0;
            return _buf;
        }
    }
    return NULL;
 }

What I find is that eventually I send off a command (say "v\n") and it processes a character that was in the buffer earlier. After about 3 or 4 additional characters I finally receive my command.

I think the problem is caused by line 59 of getc.cpp

buffer_count[RxIrq]--;

which isn't protected from the overwrite in line 46 of isrrx.cpp

                buffer_count[RxIrq]++; 

Are ++ and -- atomic?

03 Dec 2011

It appears that protecting line 59 of getc.cpp as follows gets rid of the problem.

__disable_irq();
buffer_count[RxIrq]--;
__enable_irq();

A similar fix is required on line 68 of putc.cpp

__disable_irq();
buffer_count[TxIrq]++;
__enable_irq();

while the variables are marked as volatile, this does not appear to ensure atomic updates.

19 Apr 2012

Hey! You are my hero! I had the same problem. I am using XBee Transceiver Modules and an own transfer protocoll for uploading new firmware wireless. There was the same stuck of bytes anywhere buffered. Now all is working correctly. Thank you!

Here is my modded MODSERIAL Library: http://mbed.org/users/BlazeX/libraries/MODSERIAL_mod0/

19 Apr 2012

@BlazeX, thanks for the alert.

@Anthony, thanks for the fix.

Republished here:- http://mbed.org/users/AjK/libraries/MODSERIAL/m8qbxa

18 Dec 2012

I seem to be having this same error in my program. I am using the latest greatest MODSERIAL library modified by Erik for the new mbed libraries. I have confirmed that the protection is in the getc and putc files as above. When I connect with hyperterminal my program gets a character and then printf's a response. After a while it gets out of sync and my printf statements dont match my inputs but it fires a response because it saw the autodetect character of '\n'. This is preventing me from using this otherwise awesome library.

Anything I should check or do to try and sort this out?

I dont know if there is a flush I should be doing here or what?

        if (sFlag==1) {
            NVIC_DisableIRQ(UART0_IRQn);   
            int nNum = pc.move(buf,512);
            buf[nNum] = '\0';

            parse();
            NVIC_EnableIRQ(UART0_IRQn);     
        }

the code above is how I copy the buffer from the rx buffer to my string. I chop off the '\n' and add a '\0' to terminate the string which is used in the parsing routine.

Thanks

18 Dec 2012

I looked a bit at it, and maybe got a possibility, but that is only if your code can also be really busy with other stuff.

Lets assume you send two commands, one is A, other one is B. So you send from serial: "A\nB\n". After the first '\n' MODSERIAL probably calls your autodetect function, which sets sFlag = 1. However lets assume your code is meanwhile doing other stuff, such as calculating pi to 2 million digits, so meanwhile also 'B' is received, and is placed in the input buffer since interrupts are still enabled. Now pi is calculated, and it runs the routine you printed above. There it moves 3 bytes, and replaced the last one, so you get in your buffer: "A\n\0", and the next time you only receive '\n'.

So the problem would be you only disable RX interrupts when your function gets to it, not immediatly when the MODSERIAL interrupt is called. I can imagine several solutions, but easiest would be adding your autodetect char to your move command.

18 Dec 2012

Erik I agree with your analysis and I am heading a similar path.

In my autodetect interrupt function it just sets the sFlag=1; but in the main program it has a parse() function that does a lot of printf strings so might be slow. In the first line of the parse function I reset the sFlag to 0. If another character comes in while I am in the parsing it again sets the sFlag=1 in the interrupt and after the parsing it runs through the parsing main function again.

I have tried shifting some things and will report back but its really strange behavior as if its getting characters in addition to what was sent.

18 Dec 2012

Erik - wrote:

I looked a bit at it, and maybe got a possibility, but that is only if your code can also be really busy with other stuff.

Lets assume you send two commands, one is A, other one is B. So you send from serial: "A\nB\n". After the first '\n' MODSERIAL probably calls your autodetect function, which sets sFlag = 1. However lets assume your code is meanwhile doing other stuff, such as calculating pi to 2 million digits, so meanwhile also 'B' is received, and is placed in the input buffer since interrupts are still enabled. Now pi is calculated, and it runs the routine you printed above. There it moves 3 bytes, and replaced the last one, so you get in your buffer: "A\n\0", and the next time you only receive '\n'.

So the problem would be you only disable RX interrupts when your function gets to it, not immediatly when the MODSERIAL interrupt is called. I can imagine several solutions, but easiest would be adding your autodetect char to your move command.

Can you explain what you mean with your last sentence. I dont understand what you mean by adding the autodetect char to the move command?

18 Dec 2012

My bad, I thought move would move all data it had in its buffers, but if you don't have an end char defined in your move command it will automatically use the autodetect char. So that makes it weird, move should only move up to the \n, even if there is more data available.

18 Dec 2012

Okay originally i mentioned my autodetect was '\n' but in fact its the return character '\r' if that makes a difference.

My understanding was that once the rx interrupt sees that character it will fire my serial interrupt which sets sFlag=1. Then in my main loop it will do the move which should only move up to the '\r' character which I was then stripping off and appending a string termination to make it a proper string for the parsing function.

I have since found the problem only occurs when in hyperterminal i HOLD DOWN the return key, essentially flooding the firmware with '\r' characters very fast. My parsing routine probably cannot run fast enough to keep up with that so it probably builds up several return chars in the buffer, ie buffer contents = {'\r', '\r', '\r'........}

It seems like I need a way to flush all the returns out of the buffer or something.

18 Dec 2012

That actually also makes some sense. If you flood it with autodetect chars it can set sFlag=1 20 times, but if it only gets to processing it after that, it will only move a single command and keep the rest in the buffer. That way you get a backlog you won't be getting rid off.

Possible idea: change your interrupt handler to sFlag++ instead of sFlag = 1. Then when you get to moving to your buffer, add the sFlag- - (space is only for forum code) inside it and keep running it until sFlag = 0, just make sure you don't get the original issue from this topic :P (btw if your parse function takes long I dont know if your serial interrupt should be diabled all the time).

18 Dec 2012

Erik - wrote:

That actually also makes some sense. If you flood it with autodetect chars it can set sFlag=1 20 times, but if it only gets to processing it after that, it will only move a single command and keep the rest in the buffer. That way you get a backlog you won't be getting rid off.

Possible idea: change your interrupt handler to sFlag++ instead of sFlag = 1. Then when you get to moving to your buffer, add the sFlag- - (space is only for forum code) inside it and keep running it until sFlag = 0, just make sure you don't get the original issue from this topic :P (btw if your parse function takes long I dont know if your serial interrupt should be diabled all the time).

I actually had it only disabled only on the printf in the parse function but that seemed to cause issues with my own circular buffer implementation. I dont know if its an issue with MODSERIAL or not.

19 Dec 2012

Erik

Here is a sample program I created that shows the problem I am having. If you run this on your mbed and in the terminal hold down Enter for a few seconds it will build up alot of characters in the backlog and if you then type a valid character like 'T' and press enter it will not respond for several presses of the enter key.

http://mbed.org/users/crazynanoman/code/MODSERIAL_TEST/

If you have suggestions for how to prevent this condition please let me know.

It seems that my windows program which sends commands possibly fast via VB.NET is also causing this situation because it gets behind on the display of current characters as well.

19 Dec 2012

Had a look at it, and indeed it only goes wrong when the interrupt is called, before the last one is handled (or more accurately, before the sFlag == 1 is handled). So then it only detects it once, and its buffer keeps filling.

Easiest solution, remove the wait(0.1) from your main loop, and you got a good chance you cant send chars fast enough anymore to have it happen. But if you got other code there that wont work of course, and it isnt very nice.

Plan B what I wrote in my previous post also seems to work: change interrupt handler to sFlag++, and your main loop code to:

        if (sFlag>=1) {
            
            int nNum = pc.move(buf,512);
            buf[nNum] = '\0';
            pc.printf("\n\r%s\n\r",buf);
            parse();
            NVIC_DisableIRQ(UART0_IRQn);
            sFlag--;
            NVIC_EnableIRQ(UART0_IRQn);
        }

Small edit: Depending on your requirements you might as want to change the if to a while. Same story as previously, but then it will first finish all its serial stuff before again continuing with other code in the main loop.

28 Mar 2014

I'm having continuing problems with this, and think I've found why. I'm running my serial port at 921600, so things are happening pretty quickly.

It appears that in the Macros.h file the registers are being treated as variables. Shouldn't they all be marked as volatile starting on line 54:

  1. define _RBR *((char *)_base+MODSERIAL_RBR)
  2. define _THR *((char *)_base+MODSERIAL_THR)
  3. define _IIR *((char *)_base+MODSERIAL_IIR)
  4. define _IER *((char *)_base+MODSERIAL_IER)
  5. define _LSR *((char *)_base+MODSERIAL_LSR)
  6. define _FCR *((char *)_base+MODSERIAL_FCR)

should be

  1. define _RBR *((volatile char *)_base+MODSERIAL_RBR) etc

Otherwise, the compiler is free to cache the value, isn't it?

28 Mar 2014

You can also try using https://mbed.org/users/Sissors/code/MODSERIAL/. Fork of MODSERIAL, and among others it uses directly the register definitions. Or this one: https://mbed.org/users/sam_grove/code/BufferedSerial/