search

Serasidis / STM32_HID_Bootloader

Driverless USB HID bootloader and flashing tool for STM32F10X devices
418 stars 150 forks source link

Mark new_data_is_received as volatile #69

Open chrissbarr opened 5 months ago

chrissbarr commented 5 months ago

In the F4 bootloader, I think the variable new_data_is_received should be marked as volatile.

I noticed that if I change the optimisation level from -Og to -Os or -O2 etc., the bootloader starts misbehaving for me. When the HID-Flash tool starts an upload, it freezes on the first . 1024 Bytes message, like so:

+-----------------------------------------------------------------------+
|         HID-Flash v2.2.1 - STM32 HID Bootloader Flash Tool            |
|     (c)      2018 - Bruno Freitas       http://www.brunofreitas.com   |
|   Customized for STM32duino ecosystem   https://www.stm32duino.com    |
+-----------------------------------------------------------------------+

> Trying to open the [NONE]...
> Unable to open the [NONE]
> Searching for [1209:BEBA] device...
#
> [1209:BEBA] device is found !
> Sending <reset pages> command...
> Flashing firmware...
. 1024 Bytes

Connecting with a debugger, I can see that the code is stuck in the loop waiting for an incoming command to process. Despite new_data_is_received having been set to 1 by the USB HID CUSTOM_HID_OutEvent_FS callback/ISR, the below loop in main.c appears to be optimised as if new_data_is_received never changes (presumably because it is never changed anywhere in main.c).

  while (1) {
    if (new_data_is_received == 1) {   // <-- despite this being true, if statement body is never entered

Marking new_data_is_received as volatile fixed this behaviour, and the bootloader started working for me at all optimisation levels.

I believe this is a reasonable change. new_data_is_received is changed in an ISR, which seems reason enough to mark it volatile. Even if it wasn't an ISR, given the code that sets it is outside the scope of main.c, and therefore not in the translation unit when main.c is compiled, it should probably be marked volatile for that reason anyway.

I am unsure if the same consideration should be extended to the other variables shared between main.c and the ISR, such as USB_RX_Buffer. Given that the buffer is read/written non-atomically, it's plausible the USB ISR could fire while USB_RX_Buffer is mid-read by main.c and change the values. I think this would only happen if consecutive USB HID ISRs were received before main.c could process the buffer (which seems unlikely / I haven't had it happen). It's more complicated than new_data_is_received, though, so I'm hesitant to suggest anything here.

arthurfprecht commented 1 month ago

Wonderful work! I once tried to use a more updated CMSIS but it caused the bootloader to be bigger. When I increased the optimization, it behaved exactly as you described. Really good to know 👍

uzi18 commented 1 month ago

@Serasidis any suggestions?