-
-
Notifications
You must be signed in to change notification settings - Fork 781
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
Feature: platforms/blackpill-f4: update gpio_set to gpio_set_val, only setup LED_BOOTLOADER once #1649
base: main
Are you sure you want to change the base?
Feature: platforms/blackpill-f4: update gpio_set to gpio_set_val, only setup LED_BOOTLOADER once #1649
Conversation
…l throughout blackpill-f4 code for consistency
…DER, to prevent setting up LED_BOOTLOADER twice
@dragonmux, now that v1.10 has been released, can this pull request be merged? |
It's on our list to review as we need to properly spend the time on this to understand what it's doing and if it's taking things in the direction we want the code base going or not. We'll try and get a review done in the next few days. |
@lenvm I took the liberty of reviewing this as I'm coming back to clean up and post platform-specific changes from my branches.
If you have issues with platform LED placement -- I suggest you remap them to normal GPIOs, not the power-switch source-limited ones in PC13-15 range. I implemented an enhanced systick led show and it works just fine thanks to you inverting Additionally, I propose dropping the LED_BOOTLOADER entity altogether for the platform, because its sole purpose is to be constantly lit when in BMPbootloader (and we can't do anything to force ST MaskROM to blink it) via a By the way, which bootloader option of this platform are you using -- ST or BMP? TL,DR: I fail to see the problem that this PR solves. |
Having given this a review finally (sorry it took so long!) we agree with ALTracer that we're not sure that switching all the set/clear calls to |
Detailed description
This pull request slightly updates the code for the blackpill-f4 platforms:
gpio_set()
andgpio_clear()
togpio_set_val()
, as this is consistent with the rest of the code, which usesgpio_set_val()
. The usage ofgpio_set_val()
withtrue
andfalse
is also clearer in my opinion than the usage ofgpio_set()
andgpio_clear()
.#ifdef
around the line of the code where theLED_BOOTLOADER
pin is set up usinggpio_mode_setup()
for the second time in the same file, in caseBMP_BOOTLOADER
is not defined. This change is made as it is redundant to setup theLED_BOOTLOADER
pin twice.Your checklist for this pull request
make PROBE_HOST=native
)make PROBE_HOST=hosted
)