Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Arduino_LED_Matrix.h's timer has a data race (missing volatile) which breaks animations #418

Open
dangelog opened this issue Dec 23, 2024 · 1 comment
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project

Comments

@dangelog
Copy link

The Arduino_LED_Matrix class uses a FspTimer in order to advance its animations. The timer has this callback installed:

https://github.com/arduino/ArduinoCore-renesas/blob/main/libraries/Arduino_LED_Matrix/src/Arduino_LED_Matrix.h#L345

The callback touches a bunch of data fields of the Arduino_LED_Matrix object (that installed the callback). The object is indeed passed as the context argument (last parameter of https://github.com/arduino/ArduinoCore-renesas/blob/main/libraries/Arduino_LED_Matrix/src/Arduino_LED_Matrix.h#L182 ).

The problem is that these fields are not marked volatile.

FspTimer uses ISR to manage the timer, and any non-local data touched by ISR callbacks must be marked volatile, as per https://www.arduino.cc/reference/cs/language/functions/external-interrupts/attachinterrupt/

Typically global variables are used to pass data between an ISR and the main program. To make sure variables shared between an ISR and the main program are updated correctly, declare them as volatile.

Since the mark is not there, there's a data race. I am able to make animations block or behave erratically by simply changing the currently running animation. Most data members need volatile:

    int _currentFrame = 0;   // missing, touched by next(), called by the callback
    uint32_t _frameHolder[3];
    uint32_t* _frames; // missing, touched by next()
    uint32_t _framesCount; // missing, touched by next()
    uint32_t _interval = 0; // missing, touched by the callback
    uint32_t _lastInterval = 0; // missing, touched by the callback
    bool _loop = false; // missing, touched by next()
    FspTimer _ledTimer;
    bool _sequenceDone = false; // missing, touched by next()
    voidFuncPtr _callBack; // missing, touched by next()
@per1234 per1234 added type: imperfection Perceived defect in any part of project topic: code Related to content of the project itself labels Dec 23, 2024
@dangelog
Copy link
Author

I'm not really an expert of the Arduino compiler, but on "ordinary" platforms, one would also need atomic access, not just volatileness. (E.g. in C one needs volatile sig_atomic_t, or just use atomics with proper memory fencing.)

Is accessing 32 bit integers/pointers guaranteed to be atomic?

(Then, just wondering, what if one manipulates the memory pointed to by _frames? next() follows that pointer. Again, on ordinary systems, one would need an acquire fence (and a release fence when one writes into such memory). Not a use case for me anyhow, just wondering.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

No branches or pull requests

2 participants