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

Mark new_data_is_received as volatile #69

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chrissbarr
Copy link

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
Copy link

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
Copy link

uzi18 commented Aug 10, 2024

@Serasidis any suggestions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants