Audio Library disable/enable interrupts, why?

G

Greg-R

Member
I'm working on a 4-output I2S class for the Open Audio Library. I'm trying to find the reason why code in the update() method of the 2-output I2S is done with interrupts disabled in some parts of the code. Here is an example from the 2-output I2S class update() with the disable/enables:

Code: 
  //now that we have our working memory, proceed with getting the audio data and processing
    block_f32 = receiveReadOnly_f32(0); // input 0 = left channel
        // Optional scaling for easy volume control.  Leave outputScale==1.0f for default
        if(outputScale<1.0f || outputScale>1.0f)
           arm_scale_f32 (block_f32->data, outputScale, block_f32->data, block_f32->length);
        scale_f32_to_i32(block_f32->data, block_f32_scaled->data, audio_block_samples);

         //now process the data blocks
        __disable_irq();
        if (block_left_1st == NULL) {
            block_left_1st = block_f32_scaled;
            block_left_offset = 0;
            __enable_irq();
        } else if (block_left_2nd == NULL) {
            block_left_2nd = block_f32_scaled;
            __enable_irq();
        } else {
            audio_block_f32_t *tmp = block_left_1st;
            block_left_1st = block_left_2nd;
            block_left_2nd = block_f32_scaled;
            block_left_offset = 0;
            __enable_irq();
            AudioStream_F32::release(tmp);
        }

I did a search and found this comment by Chip Audette where he says the disable/enable interrupts are not necessary:

https://forum.pjrc.com/index.php?threads/custom-audio-object-causes-lock-ups.75891/#post-350039

But if I search through either the I16 or F32 libraries, this is often done in the update() methods. Why?

Thank you,
Greg
 
That sort of low level interrupt stuff will be because there is an associated DMA completion interrupt which swaps buffers over.
 
It's the simplest method of ensuring all of the block variables get updated atomically. If interrupts weren't disabled, another routine could run when only some of the variables had been changed.
 
Teensy has priority nested interrupts. Audio uses 2 of the priority levels. Those update() functions run from a low priority interrupt, which is the reason the audio library can process audio automatically while other code and Arduino libraries run. Because it's a low priority, nearly all other libraries needing interrupts can also still work. Audio also uses a higher priority interrupt to respond to the DMA controller having completed moving data between buffers in memory and the actual audio inputs & outputs. Those DMA event interrupts can interrupt the lower priority update() functions. They do only the very quick work of swapping which memory the DMA controller will access in the near future. When the lower priority code needs to hand off data or otherwise set the conditions that higher priority code will use when it runs, the higher priority interrupt needs to be prevented from running while the variables it will use are being altered and would be seen in an inconsistent state if it happened to run at just that moment.
 
So you need to block the isr() which is run by the DMA in the middle and at the end of the data buffer.
Thank you for the description of the audio system interrupt scheme! That is very helpful.
I'm trying to think of some way of diagramming or modelling this problem. Following the code back and forth
between update() and isr() with pointer manipulations is not easy.

So how do you test or verify this sort of system? I would think that if the isr() runs during a crucial area of the update(), the result would be horrendous audio distortion. But then maybe the state of a particular system never goes there?
I removed the disables/enables in my quad output code, and it runs perfectly.
How would I create a system which is more likely to run the isr() during the update?
Or is the answer a collection of test cases?
 
Code protected inside a __disable_irq()/__enable_irq() bracket is called a critical section - datastructures shared with an ISR must be handled with critical sections, its quite simple. Then the ISR and the critical sections get serialized atomically without interfering with each other.

Even if the datastructure is a single memory location it may need a critical section (if for instance you read it then write a new value).

Some processor's require memory barrier calls as well if the caching hardware is different between DMA engine and the processor.
 
Greg-R said:
So how do you test or verify this sort of system?
Click to expand...

Maybe you're thinking from unit testing or other automated software tests? That sort of testing can be really valuable for higher level code. But it's not always so useful for low-level code, especially code that is essentially a device driver.

Even with higher level code, certain type of problems like security vulnerabilities due to buffer overflow can be notoriously difficult to find only by automated testing.

Greg-R said:
I removed the disables/enables in my quad output code, and it runs perfectly.
Click to expand...

You're playing with fire!

This is analogous to deleting seemingly unnecessary buffer size checking and then not observing any crashes during normal operation with ordinary data. Or even with unusual data. Modern software practice has a long legacy of buffer overflow bugs that went unnoticed for many years even in very widely used programs, even after extensive testing.

The extreme difficulty of race conditions in low level driver code makes those thorny buffer overflow problems seem simple. This is an incredibly hard problem.

Like how so much high level software today is written in memory safe languages to prevent buffer overflow problems, the usual approach to this sort of low level code is a conservative design where you either use a specific head-tail queue that's known to be safe, or you always disable interrupts while accessing the shared data.
 
PaulStoffregen said:
When the lower priority code needs to hand off data or otherwise set the conditions that higher priority code will use when it runs, the higher priority interrupt needs to be prevented from running while the variables it will use are being altered and would be seen in an inconsistent state if it happened to run at just that moment.
Click to expand...
Why isn't the higher priority IRQ simply masked instead of completely disabling all interrupts?
 
PaulStoffregen said:
Maybe you're thinking from unit testing or other automated software tests? That sort of testing can be really valuable for higher level code. But it's not always so useful for low-level code, especially code that is essentially a device driver.

Even with higher level code, certain type of problems like security vulnerabilities due to buffer overflow can be notoriously difficult to find only by automated testing.



You're playing with fire!

This is analogous to deleting seemingly unnecessary buffer size checking and then not observing any crashes during normal operation with ordinary data. Or even with unusual data. Modern software practice has a long legacy of buffer overflow bugs that went unnoticed for many years even in very widely used programs, even after extensive testing.

The extreme difficulty of race conditions in low level driver code makes those thorny buffer overflow problems seem simple. This is an incredibly hard problem.

Like how so much high level software today is written in memory safe languages to prevent buffer overflow problems, the usual approach to this sort of low level code is a conservative design where you either use a specific head-tail queue that's known to be safe, or you always disable interrupts while accessing the shared data.
Click to expand...

Removing the interrupt disables was a quick test to see if it would fail. Next, I am going to reduce the CPU frequency in steps to see if I can make it fail, and if it does fail, then add the interrupt disables back in to see if that fixes the problem.

One thing about this particular case seems like it should make it less dangerous. The calls to the output object isr() from the DMA are synchronous or at least semi-synchronous? It would seem that it has to be, because the I2S needs to be feed data continuously in order to not have gaps in the output audio.
So the interrupts are not like an asynchronous random button push. They can't be completely random with regards to the output data stream. It seems like they must occur in a restricted time interval.

So what causes the update() function to run? You mentioned it's from a lower priority interrupt. But it has to be running on a regular schedule, to allow for a continual flow of data. How is that timed? The question I have now is why can't the update() function do all of the interleaving. Then the interrupts from the DMA which run the isr() (in the output object) wouldn't be necessary. The DMA would still be activated by the I2S.
There is an interesting bit of code in the isr():

Code: 
       if (AudioOutputI2SQuad_F32::update_responsibility) AudioStream_F32::update_all();

What does that do?
 
IIRC updates are triggered by the first input or output class that initializes, such classes have synchronous I2S interrupts typically. Every buffer full of samples the class triggers a software interrupt on leaving its ISR I think. This then calls all the update() methods in the network of AudioObjects
 
Yup, that's how it works.

Newer versions of the core library have a fallback to IntervalTimer if no inputs or outputs took responsibility for calling update_all. That stuff is in AudioStream.cpp in the core library.


Greg-R said:
The question I have now is why can't the update() function do all of the interleaving.
Click to expand...

Please understand I can't answer every question for lack of time, but will take just a moment on this one.

Maybe the interleaving work could be done in the lower priority update() rather than the high priority DMA event interrupt. How beneficial that would be is an open question.

Again I want to emphasize that much like use of memory safe languages to prevent buffer overflow bugs, the common practice with data sharing to interrupts is an overly conservative approach. Race conditions are virtually impossible to fully test. They tend to be extremely hard to reproduce after they do strike, which makes finding and fixing really, really difficult. An overly conservative approach that prevents those problems is usually considered a good trade-off, even if it costs slightly more CPU time, memory, latency for other interrupts, and so on.

Asking these questions is fine. Experimenting with stuff is fine. Maybe you will find a better way?
 
PaulStoffregen said:
Yup, that's how it works.

Newer versions of the core library have a fallback to IntervalTimer if no inputs or outputs took responsibility for calling update_all. That stuff is in AudioStream.cpp in the core library.




Please understand I can't answer every question for lack of time, but will take just a moment on this one.

Maybe the interleaving work could be done in the lower priority update() rather than the high priority DMA event interrupt. How beneficial that would be is an open question.

Again I want to emphasize that much like use of memory safe languages to prevent buffer overflow bugs, the common practice with data sharing to interrupts is an overly conservative approach. Race conditions are virtually impossible to fully test. They tend to be extremely hard to reproduce after they do strike, which makes finding and fixing really, really difficult. An overly conservative approach that prevents those problems is usually considered a good trade-off, even if it costs slightly more CPU time, memory, latency for other interrupts, and so on.

Asking these questions is fine. Experimenting with stuff is fine. Maybe you will find a better way?
Click to expand...

"Extremely hard to reproduce" and "conservative approach that prevents those problems".
That's what I was looking for, the specific justification for disabling/enabling interrupts.

So the interrupt disables are now well understood and will stay in the F32 output object. But I'm still going to proceed with more experiments later after the next release of T41 radio code (the reason why I developed the F32 quad output class).

In another thread you mentioned you are planning to expand the Teensy documentation. I vote for this:

https://www.pjrc.com/teensy/td_libs_AudioProcessorUsage.html

At the bottom, "Understanding Audio Library Scheduling".
Thank you for your comments and answers, cool stuff!

Greg
 
You must log in or register to reply here.
Back
Top