arduino / arduino/ArduinoCore-renesas

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

Open
#418 3 comments 0 reactions 0 assignees View on GitHub
topic: code type: imperfection
Dominant language
C
Stars
193
Forks
112
PR merge metrics
No merged PRs in 30d

Description

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()
```

Contributor guide

No contributing guide indexed for this repository

Research direction

Read Arduino_LED_Matrix.h around the timer setup at line 182 and callback at line 345, then inspect the listed animation fields shared with FspTimer. Done means animation changes no longer block or behave erratically when the timer callback is active.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
embedded-iot
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.