FreeJoy-Team / FreeJoy

STM32F103 USB HID game device controller with flexible configuration
GNU General Public License v3.0
704 stars 134 forks source link

Make `button_mutex` volatile #202

Closed shoftmadder closed 1 year ago

shoftmadder commented 1 year ago

Issue

When reading the button data in ButtonsGet(), it appears at first glance that the data is protected by button_mutex: https://github.com/FreeJoy-Team/FreeJoy/blob/8f144268c6065fa51e961f3d26034db604a606e9/application/Src/buttons.c#L1022-L1024

However, button_mutex is not marked as volatile: https://github.com/FreeJoy-Team/FreeJoy/blob/8f144268c6065fa51e961f3d26034db604a606e9/application/Src/buttons.c#L39

In ButtonsReadLogical(), the value is written here: https://github.com/FreeJoy-Team/FreeJoy/blob/8f144268c6065fa51e961f3d26034db604a606e9/application/Src/buttons.c#L940-L941 And then subsequently written again in the same function: https://github.com/FreeJoy-Team/FreeJoy/blob/8f144268c6065fa51e961f3d26034db604a606e9/application/Src/buttons.c#L977-L978

When optimizations are enabled, the compiler:

This in turn means that, in ButtonsGet(), isn't actually protected by the mutex in the expected manner (since it always has the value 0), and can therefore sometimes return all zeros for the button states, due to the button states being cleared here: https://github.com/FreeJoy-Team/FreeJoy/blob/8f144268c6065fa51e961f3d26034db604a606e9/application/Src/buttons.c#L943

Solution

Mark button_mutex as volatile

Discussion

I found this bug because my engine kept cutting out in the game I was playing -- a toggle switch was sometimes sending a value of 0, which caused the engine to be turned off. I then sniffed the USB data with Wireshark, and noticed that occasionally all buttons were set to 0. After that, I noticed that a variable was shared between the "main" and "interrupt" contexts, without being marked as volatile, which led me to investigate further.

This behaviour can also be seen as flickering in the freejoy configurator.

Since marking the variable as volatile, and flashing the new code, the problem has gone away.