Closed chaeron closed 6 years ago
nothing is interrupt safe, you should design your code to be interrupt safe while using libraries like this
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...
@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...
I've added a feature branch called interrupts
where we can discuss this addition: please review that and provide any feedback
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
@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...
yes sir, its the main buffer, theres 2 array versions for 2-way traffic, got no misses or failures at those speeds :)
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
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?
std::atomic is part of STL you need only #include
github removed the brackets around atomic sorry
i just tried changing my volatile uint16_t (head,tail,_available) to std::atomic
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
INT_SAFE seems good, i dont think there are other words for it
No STL on small embedded systems please :)
I've added the CIRCULAR_BUFFER_INT_SAFE
switch: in a couple of days I will release the new version for the plugin manager
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!