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

Change ST drivers to use ST's driver repos as submodules #5976

Merged
merged 5 commits into from
Feb 4, 2022

Conversation

sgauche
Copy link

@sgauche sgauche commented Feb 3, 2022

This will update the ST drivers to the latest from ST, as well as fix issue #5968 .

I had to have the CMSIS_5 submodule point to tag 5.4.0, as I was getting the error in the below issue when trying to build CircuitPython with the latest CMSIS_5.

ARM-software/CMSIS_5#1397

@sgauche
Copy link
Author

sgauche commented Feb 3, 2022

I created an issue on the STMF4 HAL repo for the unused variable error we're seeing when trying to build the stm32f412zg_discovery board.
STMicroelectronics/stm32f4xx-hal-driver#9

@sgauche sgauche marked this pull request as ready for review February 3, 2022 16:47
@tannewt
Copy link
Member

tannewt commented Feb 3, 2022

Thank you! Do you want to wait for ST or should I fork and fix that build error? That's normally what we do if the source repo has a bug.

@sgauche
Copy link
Author

sgauche commented Feb 3, 2022

@tannewt I'll leave that up to you. If/when they eventually fix it, I'm guessing we can switch back to theirs?

@sgauche
Copy link
Author

sgauche commented Feb 3, 2022

It is probably safe to fork and fix by just removing the unused variable, it looks like the f7 qspi driver has this fixed:
https://github.com/STMicroelectronics/stm32f7xx_hal_driver/blob/27458ea876aabd8fd568c5e2a8a3448f082e2817/Src/stm32f7xx_hal_qspi.c#L874

@tannewt
Copy link
Member

tannewt commented Feb 3, 2022

@tannewt I'll leave that up to you. If/when they eventually fix it, I'm guessing we can switch back to theirs?

Yup, that'd be the idea.

I'll make a fork and remove that line. Would you mind updating this PR after I do?

@tannewt
Copy link
Member

tannewt commented Feb 3, 2022

Done in: adafruit/stm32f4xx_hal_driver@abb4cd6

@sgauche
Copy link
Author

sgauche commented Feb 3, 2022

STM32F4 HAL driver submodule changed to the fork in adafruit to fix the unused variable error until ST fixes it in their repo.

@jepler
Copy link
Member

jepler commented Feb 4, 2022

It's also possible to silence diagnostics strictly within source files and headers from a third party, meaning it may not be necessary to fork a repo just to make the ST code compile cleanly with our preferred CFLAGS.

To silence a warning inside a header controlled by a third party, modify the CFLAGS so that it is the "-isystem" flag that introduces the location where the header resides, rather than "-I" or "-iquote". For example, in the espressif port we have

espressif/Makefile:     -isystem esp-idf \

To silence a warning inside a source file controlled by a third party, modify CFLAGS for that file only. For example, in the raspberrypi port we have

$(patsubst %.c,$(BUILD)/%.o,$(SRC_SDK)): CFLAGS += -Wno-missing-prototypes

where SRC_SDK is filled with the list of files from the pico sdk that we use.

@sgauche
Copy link
Author

sgauche commented Feb 4, 2022

@jepler you're getting outside my comfort zone. If you can tell me what to add to what file, I can switch back to using the ST submodule and add something to the Makefile to ignore unused variable warnings just for the qspi driver.

@sgauche
Copy link
Author

sgauche commented Feb 4, 2022

I think the method that @tannewt suggested with the Adafruit fork is a little cleaner and easier to remember what to update when ST does fix their code.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick update!

@jepler Thanks for the tip. I didn't know we could do that for C files. Perhaps we need a separate file or section for fixes like that.

@tannewt tannewt merged commit 204ae32 into adafruit:main Feb 4, 2022
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