Interrupt safe?

[chaeron]

Is this implementation interrupt safe?

That is, if you add items in the interrupt handler and remove them in the main loop, will things work correctly?

Thanks!

[tonton81]

nothing is interrupt safe, you should design your code to be interrupt safe while using libraries like this

[rlogiacco]

The library should be able to support you in writing interrupt safe code, but please be aware interrupts are not threads.
Also note you might have to change some internals of the library preventing certain compiler optimizations, specifically the private member attribute uint16_t count in CircularBuffer.h must be redefined as volatile uint16_t count, otherwise, you might get unpredictable results: a slight change which can have substantial performance drawbacks when the library is not used within an ISR.

The following code should work adding items into the buffer within the ISR and removing them from within the loop function, just connect a push button to digital pin 2 (pull up or pull down doesn't matter):

CircularBuffer<volatile long,10> times;

void setup() {
    Serial.begin(9600);
    Serial.print("buffer size is "); Serial.println(times.size());
    attachInterrupt(digitalPinToInterrupt(2), count, RISING);
}

long time = 0;

void loop() {
    Serial.print("buffer count is "); Serial.println(times.count());
    delay(250);
    if (millis() - time >= 10000 && !times.isEmpty()) {
        Serial.print("popping "); Serial.println(times.pop());
        time = millis();
    }
}

void count() {
  times.unshift(millis());
}

Please consider I didn't test this code, yet...

[rlogiacco]

@chaeron have you the opportunity to check the proposed code?
I'm willing to add a pre-processor flag to switch in/out the volatile modifier on the count variable if you confirm it works as expected...

[rlogiacco]

I've added a feature branch called interrupts where we can discuss this addition: please review that and provide any feedback

[tonton81]

adding volatile can affect performance, but at a very miniscule level. Fact remains is the tail and head are being separately accessed, and so are their slots, so if ones writing to tail and other removing from head, there shouldnt be any issues within the ISR

I designed a circular buffer system that runs up against 30mhz SPI bus between 2 microcontrollers exchanging 100 dword data at 250uS intervals through the queue system which ive set to 8 slots. considering the queue system is dual purpose (ring buffer and array queueing), the SPI interface is transferring data over queue array system rather than a ring buffer implementation, without the hindering of the volatility, and able to achieve 5000Hz message transfers at those bus speeds of 100dwords

[rlogiacco]

@tonton81 is this library at the core of your bus? The count variable is central to some highly important functions (like available()) and if it gets corrupted the buffer becomes unreliable. But once count is marked as volatile that is not a risk anymore.
Obviously I agree with your first answer: nothing is interrupt safe and you have to program being aware of what you are doing...

[tonton81]

yes sir, its the main buffer, theres 2 array versions for 2-way traffic, got no misses or failures at those speeds :)

[tonton81]

yes, count, available should be available, but you could also make them std::atomic, which wouldnt need to be volatiled, as it handles the variable atomically. during the ISR interactions with my library, honestly, on a teensy a volatile bool or volatile uint8_t.... didnt work, because they are not native types like uint32 on arm, so the software has to handle the conversion. during the SPI transfers, bool, uint8_t, even with volatile, did NOT work. uint16_t and uint32_t DID, but i omitted them and went with the std::atomic, never failed! perhaps you should use std::atomic<uint16_t> for your count/available, they work great in ISRS. and dont even bother with mutexes in isrs, youll deadlock always :)

[rlogiacco]

I don't think std::atomic is compatible with Arduino (is it?), while volatile should be more portable...

Apart from an error I just spotted on the count member variable, its type should be switchable between uint16_t and uint8_t using the pre-processor flag CIRCULAR_BUFFER_XS (defaults to 16 bits)...

In other words the switchable volatile should, by default, end up defining volatile uint16_T count, which I understand would work for you...

If you can confirm that, not only I'm willing to add the pre-processor switch, but I will also improve the documentation adding this precious bit of information for the Teensy (does it apply to other processors? what is the Teensy mcu?).

BTW, are you willing to contribute the changes yourself, so to leave a more prominent trace of your valuable help?

[tonton81]

std::atomic is part of STL you need only #include , it should be in the arduino core (no download necessary), its definately in teensy core. I just wanted to report my findings on the SPI ISR how volatile bool and uint8_t failed during the 190uS timed transfers at ~51uS transfers and processing time inclusive. I know you want to maintain portability but yours could work fine for mcus that are 8bits (avrs), but on 32bit arms, they have to do extra processing to return the registers (volatile) that are not native 32bit writes, which is why the 16/32 primitives worked but bool/U8 failed during my tests even though they were volatile. testing was done on a T3.2, T3.5, and T3.6. but yeah volatile 16 and volatile 32 work on teensy, but atomics are usually preferred over volatiles anyways, and dont need to define them as volatile either :)

[tonton81]

#include atomic

github removed the brackets around atomic sorry

[tonton81]

i just tried changing my volatile uint16_t (head,tail,_available) to std::atomic<uint16_t>, unfortuneately i cant convert mine without rewriting methods to support it, compiler complaining because some of my methods dont match, but i would just stick to volatile uint16_t for the count, head, and tail, to be safe amongst many microcontrollers without the need to sprinkle ifdefs everywhere, keeping uint8_t volatile seems to be an issue with teensy, or perhaps ARM controllers, as the data is not accessed atomically. So, even with 8bit mcus, i think you should still keep uint16_t and not use a switch feature, for clarity and perhaps, people prefer to store more than 255 entries

[rlogiacco]

people prefer to store more than 255 entries

I agree that's why uint16_t is the default value. I wanted to provide an easy way to switch to the extra small size in those cases where you want to squeeze each and every byte... the saved memory is generally so tiny it's not worth it and on > 8bit MCUs is practically useless (other than not working, as you reported).

The flag name I've picked might be misleading though CIRCULAR_BUFFER_INT_SAFE: any suggestion for a better name?

The feature/interrupts branch already contains the updated code

[tonton81]

INT_SAFE seems good, i dont think there are other words for it

[Koryphon]

No STL on small embedded systems please :)

[rlogiacco]

I've added the CIRCULAR_BUFFER_INT_SAFE switch: in a couple of days I will release the new version for the plugin manager