Electronic – Accessing structures concurrently modified by ISR

interruptsmbedmicrocontrollerparallel

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?

Best Answer

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.