Accessing structures concurrently modified by ISR

Question

I have the the following situation: An STM32L476RG (ARM Cortex M4F @ 80MHz, 1MB Flash, 128kB RAM) parses some string data coming in over UART. The project uses mbed-os. The parsing of the data is done in an interrupt-based manner, meaning an ISR is in place which reads the just received character and has some stateful parsing logic to extract data from it. While doing so, it modifies an working-object which gradually gets filled up with the parsed values.

When the parsing process is done (all values filled), the working copy is saved into another global object of the same type. This is so that the application has an object with valid values and doesn't have to access the working copy of the ISR-based parser, which might change anytime. The structure in question is about 204 bytes big.

Code looks like (high level)

struct MeterState {
    double valueA;
    double valueB;
    double valueC;
    char id[64];
    //more..
}

volatile MeterState isrWorkingCopy;
volatile MeterState lastValue;

/* called in an ISR context by mbed-os whenever something is received on the serial */
static void SerialParserHandler() {
    uint8_t data = (uint8_t) meterSerial.getc();
    parse_data(data, &isrWorkingCopy);
    //check parsing finished..
    if(parsingFinished) {
        //save finished object
        lastValue = isrWorkingCopy;
    }
}

/* called by application to get last valid data */
void MeterState GetLastValue() {
    return lastValue; //copy structure to as return value to stack
}

However, here comes the crux of the problem. Due to length of the object and given some certain timing, it may be possible that:

1. Appcode calls GetLastValue()
2. Function starts copying the huge structure to the stack of the caller.
3. Mid-term gets interrupted by the ISR
4. ISR overwrites lastValue with new data when parsing is finished
5. GetLastValue() is resumed after ISR
6. Resulting returned value is half the old half the new value

See e.g. compiler output for GetLastValue(), it is implemented by a memcpy():

--
08015f3c <_ZN13ElectricMeter16GetLastValueEv>:
 8015f3c:       b508            push    {r3, lr}
 8015f3e:       22d0            movs    r2, #208        ; 0xd0
 8015f40:       4901            ldr     r1, [pc, #4]    ; (8015f48 <_ZN13ElectricMeter16GetLastValueEv+0xc>)
 8015f42:       f004 fa5f       bl      801a404 <memcpy>
 8015f46:       bd08            pop     {r3, pc}
 8015f48:       20001d38        .word   0x20001d38

Basically overwriting and getting the value is non-atomic. I'm not quiet seeing how it's possible to make it atomic so that the above scenario doesn't occur anymore.

I cannot just use a Mutex-based locking mechanism to atomatically exchange the lastValue because I cannot lock a mutex inside an ISR. The ISR must be non-blocking.

I also thought about temporarily disabling interrupts around the getter method as such

MeterState GetLastValueSafe() {
    __disable_irq();
    MeterState s = GetLastValue();
    __enable_irq();
    return s;
}

However, doesn't this have the possibility (as small as it may be) that the parser misses a character (which might screw up parsing and constitute a lost data value)? If the IRQs are disabled and the handler doesn't get invoked, the character is basically lost, as I understand.

Is there any sane way to solve this general problem of ISR concurrency with large structures?

Answer 1

Assuming that your parsing routine is fast (you really should minimize time spend inside an interrupt), I'd consider modifying your code, as follows.

struct MeterState {
    double valueA;
    double valueB;
    double valueC;
    char id[64];
    //more..
}

MeterState MeterBuffer[2];
MeterState *filled = &MeterBuffer[0];
volatile MeterState *unfilled = &MeterBuffer[0];
#define Increment(a) do { MeterState *z= (a); if ( (z += 1) == &MeterBuffer[2] ) z= &MeterBuffer[0]; (a)= z; } while (0)

/* called in an ISR context by mbed-os whenever something is received on the serial */
static void SerialParserHandler() {
    uint8_t data = (uint8_t) meterSerial.getc();
    parse_data(data, unfilled);
    //check parsing finished..
    if(parsingFinished) {
        Increment(unfilled);
        /* Problem here if both buffers become filled before GetNextValue is called */
    }
}

/* called by application to get last valid data */
void MeterState GetLastValue() {
    while ( unfilled == filled ) ;
    MeterState *result= filled;
    Increment(filled);
    return result;
}

The above technique only declares one pointer as volatile; the one that can be modified by the interrupt routine. There's no need to declare anything else in that way.

The above code also makes the interrupt routine solely responsible for updating the unfilled variable. No one else has any right to modify it. They are only allowed to observe it. That's all. (It's volatile, of course, since the interrupt might occur at any time and update it.)

It also makes the GetLastValue function solely responsible for updating the filled variable. No one else (not even the interrupt code) has any right to modify it. Others may only observe it. There's no need for it to be volatile as any modification by GetLastValue is done outside of any interrupt events (unless you do something to hook it into one, I suppose.)

By separating ownership of these pointers, there's never a problem with processing and updating their values.

I added an Increment() macro for clarity. You can expand it inline if you want.

There is a potential problem in the above code, highlighted by my comment. If the interrupt routine executes sends out two filled buffers before GetNextValue() has a chance to fetch one, then the double-buffer is insufficient and there really isn't a good way to recover from that. Which is why I suggested a circular buffer (of larger size.) You can modify the above code easily by increasing the number of MeterBuffer's and reduce (but never eliminate) the risks here. Only you can work out what you want to do if this problem happens. So I leave that to you to worry about.

Answer 2

Use the DMA for the UART to move the data into a memory buffer and parse from there, and check that you don't outrun the DMA while parsing. I would not parse in an interrupt.

Then have two copies of the object and three flags: one to indicate most updated copy which the application should read from on its next read, and two to indicate that the application is currently reading from a copy and which copy it is reading from.

That way if the parser is done updating the shadow copy, it won't proceed to update and modify the other (now old) copy if it is currently in the middle of being read by the application. Combined with the UART DMA, you won't lose incoming data, so the parser can just wait for the application to finish its read and release the now old copy before trying to update it. You could parse in an interrupt in this way, but now there is also no reason to (at least for not missing incoming data) due to the DMA.

The DMA could be substituted with interrupts to receive data and write to a memory buffer, but not parse.

Answer 3

There's many design problems here.

• Unless you have a hard real-time requirement that your system must be extremely fast responding (microsecond real-time) to incoming UART data on the fly, there is no need to parse anything inside an ISR. Since UART is asynchronous and unsuitable for such real-time requirements, this seems highly unlikely. Rather, in some 99% of all embedded systems applications, a UART rx ISR should only store the incoming data in a buffer and do nothing else.
• The first thing you should consider here however, is to use DMA/pick a MCU which supports DMA from UART. Then there will be no need for an ISR at all, so this is the best option.
• If DMA isn't possible for whatever reason, the second best option is to have the ISR do nothing but to fill up a ring buffer ADT. This ring buffer will preferably handle re-entrancy internally. This keeps the ISR reasonably fast. Doing things like protocol parsing and floating point arithmetic inside an ISR is plain bad design - leave that to the main application.

• As for the buffer itself, be it a ring buffer or some complete protocol struct, you simply don't hardcopy it around, ever. Particularly not while holding on to a mutex/interrupt disable.

  The buffer swaps should instead be done by swapping a pointer between multiple buffers. One reasonable design is to have one in-coming buffer that the UART is currently filling, one buffer containing the last complete protocol, and one buffer that the application is currently using. That way both the ISR and the application will always have a buffer to work with, without the need to wait for each other.

• The ISR can by its nature never be non-blocking. You need to either disable interrupts, implement a mutex-like mechanism or guarantee atomic access. Obviously you don't disable the global interrupt mask, but the UART-specific interrupt only. Assuming that the UART is running on reasonably slow baudrates, this is probably the easiest solution. Your caller program needs to be faster responding than the byte reception time.

  If you run for example 115.2 kbps no parity 1 stop, then 10 bits * 1/115.2 = 68.8us. This is the time you have to disable the interrupt, swap the pointers and enable the interrupt again, without risking data loss overrun. A Cortex M4 running at high clock should have no problem with that. But do the real-time calculation, don't "assume it is fast enough".