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

Feature: platforms/blackpill-f4: update gpio_set to gpio_set_val, only setup LED_BOOTLOADER once #1649

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lenvm
Copy link
Contributor

@lenvm lenvm commented Oct 19, 2023

Detailed description

This pull request slightly updates the code for the blackpill-f4 platforms:

  • it updates the instances of gpio_set() and gpio_clear() to gpio_set_val(), as this is consistent with the rest of the code, which uses gpio_set_val(). The usage of gpio_set_val() with true and false is also clearer in my opinion than the usage of gpio_set() and gpio_clear().
  • it adds an #ifdef around the line of the code where the LED_BOOTLOADER pin is set up using gpio_mode_setup() for the second time in the same file, in case BMP_BOOTLOADER is not defined. This change is made as it is redundant to setup the LED_BOOTLOADER pin twice.

Your checklist for this pull request

  • I've read the Code of Conduct
  • I've read the guidelines for contributing to this repository
  • It builds for hardware native (make PROBE_HOST=native)
  • It builds as BMDA (make PROBE_HOST=hosted)
  • I've tested it to the best of my ability
  • My commit messages provide a useful short description of what the commits do

lenvm added 2 commits October 19, 2023 18:20
…l throughout blackpill-f4 code for consistency
…DER, to prevent setting up LED_BOOTLOADER twice
@dragonmux dragonmux added this to the v2.0 release milestone Oct 19, 2023
@dragonmux dragonmux added the Foreign Host Board Non Native hardware to runing Black Magic firmware on label Oct 19, 2023
@lenvm
Copy link
Contributor Author

lenvm commented Oct 31, 2023

@dragonmux, now that v1.10 has been released, can this pull request be merged?

@dragonmux
Copy link
Member

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.

@dragonmux dragonmux self-requested a review November 28, 2023 13:30
@ALTracer
Copy link
Contributor

ALTracer commented Jan 7, 2024

@lenvm I took the liberty of reviewing this as I'm coming back to clean up and post platform-specific changes from my branches.

  • gpio_set() & gpio_clear() are pure libopencm3 API functions, while gpio_set_val() is a BMD-specific shim over these. All of their usage can and does get inlined, but I would not Just migrate to it without some other added benefit (for example of honoring LED active polarity level). It would also break status-quo consistency with other platforms (yes they all also have a thing in SET_x_STATE() but it's a macro)
  • gpio_mode_setup() walks the bitset of 16 gpio pins in a for-loop, so in fact doing more calls takes more time (and flash space, but it's less relevant for this platform) than aggregating all pins of a port and paying for a single call.
  • LED_BOOTLOADER is not set up "by the bootloader section above" as far as BMF runtime payload is concerned. See that it ends in a scb_reset_core() into ST MaskROM, and is hidden away for BMP_BOOTLOADER=1 build variant (which I am pushing for).

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 SET_IDLE_STATE() polarity -- PR pending. PC15 cannot be used to drive an LED at active-high level, it can only sink current under an LED already connected to board 3.3v power rail (and current-limiting resistor), which is the reason WeAct followed wiring it that way.

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 gpio_clear() of PC15 if and only if there is an LED wired that way (similarly to PC13 one). That's how it was used in f4discovery we inherit code from. It can be obsoleted by a DFU blinky / LED show unless you can prove a sinking PC15 pin stays that way in ST MaskROM.

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.

@dragonmux
Copy link
Member

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 gpio_set_val() gains anything, other than having to then hope that the compiler does DCE (Dead Code Elimination) to the same underlying calls as were there before. We do like the improvement for the bootloader LED though so if you could rebase this onto main with just that change, we'll approve it for merge and get it in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Foreign Host Board Non Native hardware to runing Black Magic firmware on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants