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

Fix GPIO AF settings being overridden by timerInit(). #12886

Merged
merged 4 commits into from Jun 13, 2023

Conversation

hydra
Copy link
Contributor

@hydra hydra commented Jun 13, 2023

timerInit blindly reconfigures GPIO AF for all pins in the fullTimerHardware array regardless of if the timer pin is used already by something else.

For example, if it is called AFTER the SPI initilisation (e.g. after configureSPIBusses), then any AF settings for the SPI are overridden by timer AF, making the SPI hang when it's used.

This PR extracts the AF init code into timerIOInit and calls it early in the init code.

Targets like the SPRacingH7ZERO, which use PB3/PB4 for SPI3 gyro, now boot. With out this PR the target would hang on SPI3 access because the GPIO AF settings were reset from AF6 (SPI3) to AF1 and AF2 respectively.

@github-actions
Copy link

Do you want to test this code? You can flash it directly from Betaflight Configurator:

  • Simply put #12886 (this pull request number) in the Select commit field of the Configurator firmware flasher tab (you need to Enable expert mode, Show release candidates and Development).

WARNING: It may be unstable. Use only for testing!

@hydra
Copy link
Contributor Author

hydra commented Jun 13, 2023

There are binaries for testing in betaflight/config#66 for the SPRacingH7ZERO and SPRacingH7NANO. The former of which requires this PR to boot.

Copy link
Contributor

@ledvinap ledvinap left a comment

Choose a reason for hiding this comment

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

Reasonable first step. But isn't it possible to init timers only when they are used.
Also, this PR does not affect non-hal times. Is that intentional?

@hydra
Copy link
Contributor Author

hydra commented Jun 13, 2023

Reasonable first step. But isn't it possible to init timers only when they are used. Also, this PR does not affect non-hal times. Is that intentional?

This is the simplest work-around to allow targets to boot until a BF maintainer, i.e. not me, comes up with a long-term solution. I looked for similar code in timer_stdperiph.c but didn't find it. I didn't check any other architectures either.

FYI: From what I can tell the code was broken in #12862 by @blckmn

The default was TIM_USE_ANY which was an alias for TIM_USE_NONE, and then when the check to see if the timer was actually used before initialising the AF was deleted everything broke.

See https://github.com/betaflight/betaflight/pull/12862/files#diff-7308a69a0b62f3e86b3f82fa62403d15d3f154261f03e1211fc04e879bdc8908L47-L48

The check that was removed was this:

for (unsigned timerIndex = 0; timerIndex < TIMER_CHANNEL_COUNT; timerIndex++) {
        const timerHardware_t *timerHardwarePtr = &TIMER_HARDWARE[timerIndex];
        if (timerHardwarePtr->usageFlags == TIM_USE_NONE) {
            continue; 
        }
        // XXX IOConfigGPIOAF in timerInit should eventually go away.
        IOConfigGPIOAF(IOGetByTag(timerHardwarePtr->tag), IOCFG_AF_PP, timerHardwarePtr->alternateFunction);
    }

See https://github.com/betaflight/betaflight/pull/12862/files#diff-e83e68d46b461bd090d5a593952e3ed2517b55f439dd79028e08d0fdf5890763L1034-L1036

Enjoy!

@blckmn
Copy link
Member

blckmn commented Jun 13, 2023

I agree with @ledvinap. We can initialise the AF when the pin is first requested to be used with a timer.

I'm happy to clean this up given I broke it.

@hydra
Copy link
Contributor Author

hydra commented Jun 13, 2023

FYI, this PR is the quickest thing I could think of that would allow me to continue working on restoring the H7ZERO target. I didn't spend a lot of time on this PR, nor do I intend to.

Suggest merging as-is, and then @blckmn can do a follow-up PR which should be tested on the H7ZERO before being merged. This PR is tested on 4 H7 targets, binaries are in the linked issue.

@ledvinap
Copy link
Contributor

@hydra : Can you add comments about workaround to code.

@blckmn : IMO it is best to merge this and then work on correct fix. The change is small

@blckmn
Copy link
Member

blckmn commented Jun 13, 2023

Going to merge immediately as it fixes the issue, and I'll work on a longer term fix.

@blckmn blckmn merged commit 646de8c into betaflight:master Jun 13, 2023
18 checks passed
@blckmn
Copy link
Member

blckmn commented Jun 13, 2023

Servos, PWM Motor outputs all seem to have the IOConfigGPIOAF already located within their respective init. It may be simply a case of finding any loose ends, and amending to remove the for loop entirely from HAL.

@ledvinap explains why it may have only been present for the HAL driver, a hangover.

@hydra
Copy link
Contributor Author

hydra commented Jun 14, 2023

@hydra : Can you add comments about workaround to code.

There's already comments in the init method in my PR. 😄

Thanks everyone for being pragmatic and getting this merged.

davidbitton pushed a commit to davidbitton/betaflight that referenced this pull request Feb 5, 2024
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.

None yet

3 participants