SAMD51 + Arduino: basic interrupts question

[autotel]

I think this is something most people would know the answer.

The problem in short, is that I have an array of numbers declared as volatile, aswell as a counter variable. I access these both, from a main loop and from a timer interrupt callback. The problem is that they seem to have different values on the main with respect to the interrupt. For example, if I add 1 for each call in the interrupt, and in the main I evaluate whether it is greater than a value (e.g if(counter>210) ), it never evaluates true, although it might evaluate true in the interrupt. You might skip reading the code and explanation, because the question in particular is: how do you share an array between a function which is called in the main and another which is called in the interrupt, and why the values of these could seem different depending on what function they are found? I am using volatile.

I want to know what I am missing, in order to learn more, because this challenges the understanding I have of volatile, although I have rarely used them. It seems to me that I have a misconception.

I posted my question in Adafruit forum too, but I might have asked in a confusing way, because I got no answers. I sometimes have a hard time making my questions understood in forums. Let me know if this is happening here.

This is the case in particular that I am dealing with: I want to make a sort of synthesizer. Reading the voltage of a potentiometer takes long enough to produce a sound glitch, so I need to make interrupts to keep up the audio writing. I intend to have a sound buffer shared among the main loop and the interrupt. In this way I can read potentiometers

The code might have a couple of unexplainable things, because I have been experimenting with it, but I tried to clean it up to make it understandable. This sketch is being "uploaded" using arduino to an Adafruit feather M4 board. I am also using code from the following repository to setup the interrupt: https://github.com/Dennis-van-Gils/SAMD51_InterruptTimer. I have already made sure that the interrupt is indeed called.

I also hear a buzz, but that is likey to be due to some overlook that I might find later. I have seen it making sound without the buzz.

Code: [Select]

#include "Adafruit_ZeroI2S.h"
#include "SAMD51_InterruptTimer.h"
//if false, there will be sound, if true there is no sound (only
// a buzz of the buffering frequency)
//by toggling the LED I can make sure the interrupt is actually
//running at audio rate.

#define CONCURRENT false
#define SAMPLERATE_HZ 44100

#define AMPLITUDE     ((1<<29)-1)

#define WAV_SIZE      256

#define BUFFERING_FREQUENCY 100

const uint32_t i2sIntervaluS = 20;

const uint32_t bufferSize = SAMPLERATE_HZ / BUFFERING_FREQUENCY;
volatile int32_t audioBuffer[bufferSize];

//variables used for the "circular" audio buffer

//indicates what part of the buffer has been written to the output
volatile uint32_t bufferPlayhead=0;
//indicates what part of the buffer has already been calculated
uint32_t bufferWritehead = 0;


#define F4_HZ      349.23

int32_t sine[WAV_SIZE]     = {0};

// Create I2S audio transmitter object.
Adafruit_ZeroI2S i2s;

#define Serial Serial1

void generateSine(int32_t amplitude, int32_t* buffer, uint16_t length) {
    // Generate a sine wave signal with the provided amplitude and store it in
    // the provided buffer of size length.
    for (int i=0; i<length; ++i) {
        buffer[i] = int32_t(float(amplitude)*sin(2.0*PI*(1.0/length)*i));
        audioBuffer[i] = int32_t(float(amplitude)*random(1.00));
    }
}

uint32_t sineHead=0;
float freq=F4_HZ;


int32_t sample(){
    //this will grow to be an expensive function

    float floatSampleRate=float(SAMPLERATE_HZ);
   
    float delta = (freq + 0) * WAV_SIZE/floatSampleRate;
    uint16_t pos = uint32_t(sineHead*delta) % WAV_SIZE;
    sineHead++;
    return sine[pos];
}

//fill all the "spent" buffer with fresh new samples
void calculateBuffer(){
    uint32_t tempBuffer[bufferSize];
    uint32_t updateStart=bufferWritehead;
    noInterrupts();
    uint32_t updateEnd=bufferPlayhead % bufferSize;
    interrupts();
    //Imagine that the sample() function actually was more complicated,
    //including the readout of analog data
    while(bufferWritehead!=updateEnd){
        tempBuffer[bufferWritehead]=sample();
        bufferWritehead++;
        bufferWritehead %= bufferSize;
    }

    //fast copy calculated buffer, not interruptible
    //TODO: only copy the updated section of the buffer
    noInterrupts();
    for(uint32_t sampleNumber = updateStart; sampleNumber<updateEnd; sampleNumber++){
        audioBuffer[sampleNumber]=tempBuffer[sampleNumber];
    }
    interrupts();
}

//actually send the sound out to I2S
void writeNextSample(){

    digitalWrite(PIN_LED, true );
    int32_t nspl=audioBuffer[bufferPlayhead % bufferSize];
    // int32_t nspl=sine[bufferPlayhead % bufferSize];
    i2s.write(nspl,nspl);

    noInterrupts();
    bufferPlayhead=bufferPlayhead+1;
    interrupts();

    digitalWrite(PIN_LED, false );

}

// when is time for next sample output (only concurrent false)
unsigned long nextWrite=0;

void setup() {
    if (!i2s.begin(I2S_32_BIT, SAMPLERATE_HZ)) {
        while (1);
    }
    i2s.enableTx();
    generateSine(AMPLITUDE, sine, WAV_SIZE);
    #if CONCURRENT
    TC.startTimer(
        i2sIntervaluS,
        //floor(1000000 / float(SAMPLERATE_HZ)),
        writeNextSample
    );
    #endif
}

void loop() {
    unsigned long now=micros();

    calculateBuffer();
    #if !CONCURRENT
    if(now>=nextWrite){
        writeNextSample();
        nextWrite=floor(float(now + 1000/SAMPLERATE_HZ));
    }
    #endif
}


[Jeroen3]

Can you reduce your problem to a minimum example? There is commented code and #ifs removing or adding code. Usually the process of doing this will make you discover the problem.
The ISR is also not following naming convention, is it writeNextSample ?

Also, this equals to floor(now + 0).

Code: [Select]

nextWrite=floor(float(now + 1000/SAMPLERATE_HZ));

The volatile qualifies is fairly stupid, it will just mandate a read/modify/write each statement. This will not be causing your problems.
--- edit:
The way you share an array with an ISR is simple. You declare and mark the array volatile.
Whenever your non-isr thread want to use the array, it disables the specific IRQ flag, and when it is done it enables it. Possibly causing immediate execution! So depending on system you may need a memory barrier.

Code: (example) [Select]

volatile type array[size];
void isr(){
  for(..){
   array[i] = thing();
  }
}

void non-isr(){
  NVIC_DisableIRQ(device_IRQn);
  for(..){
   printf(array[i]);
  }
  NVIC_EnableIRQ(device_IRQn);
}
It is not possible for the ISR to run during non-isr or vice versa.

With two non-isr threads things get complicated with mutexes and semaphores.