RingBuffer interrupt issue (with simple solution)

[Klaus-KK]

This issue was discovered by the following thread in the Arduino forum.

https://forum.arduino.cc/t/issue-receiving-serial-data-on-nano-33-ble-uart/851945

The RingBuffer implementation for the Arduino Nano 33 BLE (and others) is not interrupt safe, because it uses a variable to keep track of the number of elements in the buffer (_numElems), instead of calculating it from the write and read index (_iHead, _iTail). I could not find the RingBuffer.h file in this repository but believe it is in the following location.

This RingBuffer.h is not interrupt safe
https://github.com/arduino/ArduinoCore-API/blob/master/api/RingBuffer.h

By accident I found the Nano 33 IoT has a different RingBuffer.h file that handles this correctly by calculating the available elements in the buffer.

This RingBuffer.h is interrupt safe.
https://github.com/arduino/ArduinoCore-samd/blob/master/cores/arduino/RingBuffer.h

Can you update the RingBuffer.h file to the version already used in the ArduinoCore-samd?

[aentinger]

Very quick question @Klaus-KK? What makes you think that one version is inherently thread-safe while the other is not?

Keep in mind that the code can be interrupted literally anywhere/anytime and arithmetical operations consist of multiple assembly instructions (with non of them atomic). If there's no locking ensuring atomic ring buffer operations both variations are bound to fail.

[Klaus-KK]

The issue is that _numElems gets written in store_char and read_char. This can create a permanent error if one read-modify-write to _numElems is interrupted with another read-modify-write to _numElems. I guess we both agree on that part.
I described the exact failure behavior in reply # 9 in the forum.

https://forum.arduino.cc/t/issue-receiving-serial-data-on-nano-33-ble-uart/851945/9

I believe your point about the atomic access does not matter under the condition that _iHead and _iTail type are equal or smaller than the architecture bit size. Then writes will be single cycle. With that I mean the write-back/store of the updated value e.g., of an increment.

Maybe you think of another corner case, I have not thought of. In any case can we agree that the version in ArduinoCore-samd is safer than the version in ArduinoCore-API?

[aentinger]

I believe your point about the atomic access does not matter under the condition that _iHead and _iTail type are equal or smaller than the architecture bit size. Then writes will be single cycle. With that I mean the write-back/store of the updated value e.g., of an increment.

This is a very interesting statement. Indeed I've been contemplating it for over half an hour and I did not manage to find fault in your statement. Regardless, you are missing a point here: The ArduinoCore-samd:Ringbuffer implementation has been made thread-safe by wrapping access to the Ringbuffer within critical sections (ArduinoCore-samd:SafeRingbuffer, Usage see here). You see, even if your theory is correct it doesn't matter as the ArduinoCore-samd:Ringbuffer is already protected by critical sections.

I've read the whole forums post once more, especially your explanation and I find it correct. That is to say - for ArduinoCore-mbed:Serial.cpp:v2.0.0 for which you made that analysis. Checking in on ArduinoCore-mbed:Serial.cpp:v2.3.1
I find that all (well most @facchinm 😉) access to the ring buffer has been protected within critical sections. Consequently the issue reported by the OP should no longer appear.

My suggestion: Can you carry that info back to the forum and ask the OP to just test with the latest version again?

I'll be waiting your reply, but otherwise I intend to close this issue.

[Klaus-KK]

I just checked the Arduino mbed Nano package on my Windows PC.

..\AppData\Local\Arduino15\packages\arduino\hardware\mbed_nano\2.3.1

There is no SafeRingBuffer file, the Serial.h includes the standard RingBuffer.h and has a private standard RingBufferN instance implemented. It matches the file on GitHub.

https://github.com/arduino/ArduinoCore-mbed/blob/2.3.1/cores/arduino/Serial.h

I think you "just" need to apply the changes made to the SAMD package to all packages that use the standard RingBuffer implementation. I know, it is likely a bit more work than a copy and paste. Thank you for looking into this issue.

[aentinger]

There is no SafeRingBuffer file, the Serial.h includes the standard RingBuffer.h and has a private standard RingBufferN instance implemented. It matches the file on GitHub.

Of course not, the ArduinoCore-mbed ring buffer is made thread-safe via core_util_critical_section_enter / core_util_critical_section_exit. The ArduinoCore-samd ring buffer is made thread-safe via SafeRingBuffer.

@Klaus-KK - Let me be honest - you are wasting my time here. I spelled out everything perfectly clear above. There likely way an issue with 2.0.0 but no longer with 2.3.1.

[Klaus-KK]

I am sorry for wasting your time. I made a mistake and looked at the wrong part of your explanation. Thank you for spelling it out for me.

Next time you want to be rude, do not hide behind honesty. I can take it. You are not going to hurt my feelings.