[C][ARM] changing variables also accessed by an interrupt routine

[Simon]

I have an array that is the screen buffer to store characters that are repeatedly put onto a character display. When I want to change the display text I copy the data into the array. The screen is constantly updated from an interrupt routine that cycles through each character.

Is there any possible issue that can arise due to the interrupt routine wanting to access a variable as it is being updated? I thinking no as the current instruction has to end if the interrupt fires, so the data will be there or the code that is about to write the data has to wait and then write after the now old data was read.

[RoGeorge]

If the array have some long datatype that requires consecutive assembler instructions to write a single variable value, then you have to make sure the writes/reads are atomic.

For example, when the normal program read or write a variable that requires more than a single memory address access (lets' say a long long int of 64 bits or more), the interrupt may occur somewhere between the first and the second memory address access, so the interrupt might change the value in the middle of a read from the main program, which would be bad, the main program would end with a corrupt value.

The interrupt occurs after each machine instructions, not after each C line, so it can happen in the middle of a read or halfway of a write of a C variable that is stored on multiple bytes.

I think GCC has some "atomic" mechanisms, but I don't master C well enough.  I only google about atomic in critical cases, otherwise I just disable the interrupts while working with the memory area that can be changed by an interrupt.

[Simon]

it's a 32 bit system and these are 8 bit variables.

[tggzzz]

I have an array that is the screen buffer to store characters that are repeatedly put onto a character display. When I want to change the display text I copy the data into the array. The screen is constantly updated from an interrupt routine that cycles through each character.

I would avoid that architecture.

In general interrupts should capture the necessary information, and pass that to a background task for processing.

I would have a task (or equivalent) that processes the information and updates the screen when necessary. The interrupt routine should capture the information and put it in a shared buffer then set a flag to indicate that the task should run. The scheduler (or equivalent) looks at that flag, not the buffer.

The benefits are that

• the interrupt/background interaction is limited to a single simple variable, the flag
• it is easy to inspect the binary to determine that the compiler has generated the code you want
• interrupts are short duration
• you can test and debug without hardware interrupts being present, and add the hardware when all that is satisfactory

[Simon]

It's one of those hitachi knockoff character display controllers. A small amount of code runs each time the 100µs interrupt fires, it takes 2 cycles to put a character on the display so it's not taking much time. This means that it just runs in the background constantly updating the display with whatever I put in the buffer. I have now created a flag variable that stops the screen refresh function running while I am putting any data into the buffer array.

I don't think this was causing any problem but I have locked out simultaneous access to the display buffer array to be on the safe side.

[tggzzz]

Provided you have the appropriate modifiers on the variable declarations, that should work in this case.

Nonetheless, "my" architecture is general purpose, scaleable, predictable, and testable. The last two are always advantageous.

[Simon]

Well this was meant to be quite general purpose. If i take this board to a new project the code will start with this project with everything stripped out bu the display updater with it's buffer array ready to have data put into it.

I have moved the screen updating call to the main program based on a flag. It works just the same.

[tggzzz]

By general purpose I was implying a radically different project.

With the screen updater in the main program, you can easily know when it will and won't execute. That might be advantageous in cases where it is less urgent than some other activity.

[Siwastaja]

Simon's system, if I understood correctly, generates the data outside of ISR, and does the display bus access in the ISR.

It's exactly the correct way to do it. No need to turn it around.

All you need is guarding against non-atomic operations.

The problem is content update happening in parallel to transferring that content into the display. Because updating the display is a never-ending process, you can philosophically say the content is always partial. But the interesting notion is, how the content is partial. Is it:
* Lacking part of a single value (for example, lowest 8 bits out of 16-bit value are old, highest 8 bits are new; this results in corruption, as the mixed value makes no sense)
* Lacking part of a single full screen frame (say, upper half of screen is old, lower half of screen is new)
* The whole display frame is old data

Given this, the practical implementation options are, respectively:

1) don't do anything - temporary display corruption occurs when the content is updated at bad timing
2) guard against single word accesses by disabling ISR
   - no corruption, but it's unpredictable when the modification is visible: this frame, or the next frame
      - in moving video, this could look like smear (part of the picture is from previous frame; what part, that depends and jitters)
      - this is likely perfectly OK in Simon's case
   - slight jitter caused to ISR timing because of atomic safeguards disabling the ISR
3) double buffering of the whole display content
   - no corruption; all updates done to the buffer apply at once, to the next full frame
   - requires more memory
   - requires some buffer handling / swapping code
   - causes a delay in display update

To implement 2, qualify the variable that needs to be written in the main thread loop, volatile, and surround writing to it by first disabling, then re-enabling interrupts.

Unless you are 100% sure the operation is inherently atomic; then volatile alone is sufficient.

You can also look at the C11 atomic types.

[FlyingDutch]

I have an array that is the screen buffer to store characters that are repeatedly put onto a character display. When I want to change the display text I copy the data into the array. The screen is constantly updated from an interrupt routine that cycles through each character.

Is there any possible issue that can arise due to the interrupt routine wanting to access a variable as it is being updated? I thinking no as the current instruction has to end if the interrupt fires, so the data will be there or the code that is about to write the data has to wait and then write after the now old data was read.

Hello,

you can use simple semaphore variable (even bool), and set it to true when you starting update the array, and set back to false when updating is done.
The in interrupt routine have to check if this variable is true - if yes then exit from routine without refreshing display. And vice-versa - set this variable in interrupt routine and check in place before updating an array.

Best Regards

[WatchfulEye]

Potential issues that can occur when sharing a variable between a main program and ISR are: non-atomic operations and potentially reordering of operations.

Not all memory loads and stores are atomic (occur in a single define operation) e.g. A uint32_t operation will likely be broken into multiple sub-operations on an 8bit architecture; and a double or int64_t operation will almost certainly be broken up on any MCU. Whether this matters depends on the data and its usage. For a frame buffer, this may not be relevant (if an ISR reads a shorn write, then it will be fixed next refresh) . For acquired data or a process control output this may be very relevant, as a partially completed operation could result in corrupt data being processed.

The solution, if one is needed, is to use a datatype which is known to be atomic on your hardware, or to use an atomic operation library to ensure a logically atomic result (see atomic.h for c, or std::atomic for c++).

The other issue is ordering of operations. Compilers, especially optimising compilers, may change the order of operations from what is written. The assumption the compiler makes is that the state of the program at the end should be correct, but not necessarily the state at any intermediate point. If you are sharing data with an ISR, correctness of the intermediate states may be relevant.

Consider the following pseudo-scenario, If your ISR is forwarding status messages of a football match, to a main loop which will print the final score, then the order in which The main loop checks the "match over" variable and the "score" variable matters. If the program checks "score" first, the ISR may add an extra goal and finalise the match before the loop checks for finality, and there is a risk of a wrong final score being printed. So you write your loop to check "match over" first. The consistency problem is now fixed.

Or is it? If the compiler or hardware has optimised your code and reordered the operations, then the code may not work correctly. In fact, the compiler might have noticed that the score and match over variables are never modified in the main loop, and simply deletes them (why check these variables if they never change?)

The solution in this case is to advise the compiler that these access to these multiple variables must not be reordered or skipped and they must be accessed exactly as written. In c, this is done by marking the variables as volatile.

In the case of a frame buffer, this should be marked volatile to ensure that operations are not deleted/skipped.

[Simon]

Hello,

you can use simple semaphore variable (even bool), and set it to true when you starting update the array, and set back to false when updating is done.
The in interrupt routine have to check if this variable is true - if yes then exit from routine without refreshing display. And vice-versa - set this variable in interrupt routine and check in place before updating an array.

Best Regards

This is what i have been doing. Using a flag in the interrupt routine and updating when I see it set yields the same results.

Corrupt data on the screen is not an issue, it's updating 62.5 times a second so the screen always looks flawless.

[hans]

In this case, if the IRQ is only moving data around and cannot get stuck on some kind of control depending on the data.. then I don't see any issue with this. Worst-case you're printing out a string of 16 characters, and on char X the IRQ fires and writes the old character X to the display. On a PC you might say you observe some screen tearing, where the top and bottom half parts are of two separate frames. That is annoying but often quickly resolved, since I assume this program is not constantly refreshing but not rendering new frames.
Do you really want to go to the effort to synchronize screen buffer pushes to the rendering functions? (such as Freesync technology on PCs now works)

I guess you could, and perhaps it's an all round nicer approach, but I don't think this structure would compromise stability, with or without semaphores/atomic instructions. Certainly for larger framebuffers (e.g. pixel displays), I would go to the effort to mark a bounding box around the pixel data that is "dirty" and needs to be refreshed on the display. But depending on the display size, that is only an optimization step.

If you're working with larger variables (16-bit on a 8-bit MCU, 64-bit on 32-bit etc.), and especially when that variable will be used for some kind of loop, then I would be a lot more careful, in both directions (IRQ write or main-code write). If you write to a variable in an IRQ, while the main code takes 2 instructions to load it, you can get race conditions and strange behaviour that only occurs very rarely (depending on how often the IRQ fires and/or variable is loaded). In that case I would make sure to work with atomic structures, such as exclusive load/store or disable IRQs.

[tggzzz]

Simon's system, if I understood correctly, generates the data outside of ISR, and does the display bus access in the ISR.

It's exactly the correct way to do it. No need to turn it around.

In that case why not have one thread generating the data and writing to the display. No need for interrupts.

Possible variant: only write to the display when the data has changed.

[DavidAlfa]

Just disable interrupts before updating the data and reenable them after done.
Don't disable the interrupts for the whole process of getting/parsing the data, use a temporal buffer instead, and when done transfer the data to the LCD buffer disabling interrupts as said.

[Simon]

Simon's system, if I understood correctly, generates the data outside of ISR, and does the display bus access in the ISR.

It's exactly the correct way to do it. No need to turn it around.

In that case why not have one thread generating the data and writing to the display. No need for interrupts.

Possible variant: only write to the display when the data has changed.

It's a system of pages of data. Rather than use flash space to store the data in an array only to copy it to a same size array in RAM I call from the pages array that is a constant the page I want. I then fill in the blanks with the variables I want to dispaly. So I do a memcpy to the buffer when the page is changed and then update specific locations of the array once a second with the lastest data. All the time the buffer is being written to the display.

To generate the data and put it on the display means taking a chunk of continuous time to do that, it also means delays because this is a parallel display (all they had in stock). So doing it in an interrupt with a variable that keeps track of where I am in the update sequence works very well as the time taken is small and at regular intervals rather than say 25ms of doing nothing but updating the dispaly and wasting the vast majority of the CPU's performance during that time.

1) Interrupt fires and data is put on the bus and clock raised, exit interrupt.
2) Interrupt fires and clock is lowered. exit interrupt
3) goto 1)

My problem seems to lie elsewhere. something is going wrong somewhere and I don't know where. Now my buttons are mot being read. but then they are, it's like the CPU randomly locks up.

I start to wonder if it's something going wrong with memcpy() as this is the only piece of code I have not written and I cannot inspect it.

[Simon]

It seems I need to learn to use the debugger, that is if anyone can be bothered to teach me to use it. It looks like I have lots of videos from microchip where they will show me what I could do if only I knew but actually showing me how breaks some law of idiocy and stringing this stuff out to make it look like it's more than it is.

[Jeroen3]

Corrupt data on the screen is not an issue, it's updating 62.5 times a second so the screen always looks flawless.

Stop worrying about the data corruption then. Obviously there is no harm in bad data integrity you probably won't even see it.

[Simon]

I'm not worried about corrupt data as unlikely as that is as a mere 1 byte is moved at a time and the data is refreshed on screen every 16ms. What I am worried about is that my program now locks up with another rather unusual effect. I wanted to make sure I was not triggering something. From what I can now tell when I start, I get as far as the first memccy and data going onto the screen, then it all goes awol.

[Siwastaja]

Not having any write guards can only lead to screen data corruption (in case the write is not atomic; now I'd guess it is atomic, hence no corruption). But as writing to the LCD is one-way process, there simply is no way it could cause any hang-up, delay, or lockup. Your lockup problem is elsewhere. Keep looking.

Be very careful with memcpy.

[Simon]

In my hunt for sample code when working with the non volatile memory controller I found what I think is some microchip code that seems to include it's own version named with a capital M:

Code: [Select]

void * Memcpy(void* dst, const void* src, unsigned int cnt)
{
char *pszDest = (char *)dst;
const char *pszSource =( const char*)src;
if((pszDest!= NULL) && (pszSource!= NULL))
{
while(cnt) //till cnt
{
//Copy byte by byte
*(pszDest++)= *(pszSource++);
--cnt;
}
}
return dst;
}

I assume this is returning a pointer (void * ) it is because "dst" is returned presumably as a pointer to the last location? maybe as a means of calling code checking it ended in the right place?

I'm thinking of using that maybe with the returned value removed.

Can compiler code optimization cause any issues?

[tggzzz]

Can compiler code optimization cause any issues?

Yes, many many many problems, both obvious and subtle.

The higher the optimisation level, the more the compiler writers optimise against what isn't said in the C standard. Many people don't understand what the compiler flags and C keywords do and don't guarantee. Volatile is a classic example of that.

Higher optimisation levels can lead to "surprising" removal and re-ordering of code.

[langwadt]

In my hunt for sample code when working with the non volatile memory controller I found what I think is some microchip code that seems to include it's own version named with a capital M:

Code: [Select]

void * Memcpy(void* dst, const void* src, unsigned int cnt)
{
char *pszDest = (char *)dst;
const char *pszSource =( const char*)src;
if((pszDest!= NULL) && (pszSource!= NULL))
{
while(cnt) //till cnt
{
//Copy byte by byte
*(pszDest++)= *(pszSource++);
--cnt;
}
}
return dst;
}

I assume this is returning a pointer (void * ) it is because "dst" is returned presumably as a pointer to the last location? maybe as a means of calling code checking it ended in the right place?

the return value is just the destination unchanged

[Siwastaja]

Don't reimplement memcpy. The standard from C library or compiler builtin will work.

The only problem is, memcpy doesn't accept volatile arguments.

memcpy in general is just prone to stupid programmer mistakes like getting the length wrong. Reimplementing it in the same way won't fix this.

[DavidAlfa]

Atomic writes to a single variable won't cause issues, but probably you're updating more than a single variable (an array of few values, or a text string), thus it behaves very much like a non-atomic operation and it's very probable that at some point the interrupt gets weird data.

Nobody said anything, about what I posted earlier. What's the problem with simply blocking interrupts during these few instructions writing to the variables?

If you can't do that for whatever reason, a flag would do the job, ex. "buffer_busy", set before writing to the LCD buffer, and clear after.
The flag should be checked by the LCD interrupt, and skip LCD update if active.

Not easy to guess without any code to review.