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

QEI: Unable to (safely) define encoder pins as pull-up #321

Closed
FLamparski opened this issue Mar 14, 2021 · 6 comments · Fixed by #357, #358 or #359
Closed

QEI: Unable to (safely) define encoder pins as pull-up #321

FLamparski opened this issue Mar 14, 2021 · 6 comments · Fixed by #357, #358 or #359

Comments

@FLamparski
Copy link

I have this bit of code, which attempts to enable pull-up resistors on PB6 and PB7, and then construct a quadrature encoder interface on top of those pins using TIM4:

        let enc_a = gpiob.pb6.into_pull_up_input(&mut gpiob.crl);
        let enc_b = gpiob.pb7.into_pull_up_input(&mut gpiob.crl);
        let encoder = Timer::tim4(dp.TIM4, &clocks, &mut rcc.apb1).qei(
            (enc_a, enc_b),
            &mut afio.mapr,
            QeiOptions::default(),
        );

This fails because apparently the qei() method expects the pins to be floating instead:

error[E0277]: the trait bound `PB6<Input<PullUp>>: stm32f1xx_hal::gpio::Mode<Input<Floating>>` is not satisfied
  --> src\main.rs:95:68
   |
95 |         let encoder = Timer::tim4(dp.TIM4, &clocks, &mut rcc.apb1).qei(
   |                                                                    ^^^ the trait `stm32f1xx_hal::gpio::Mode<Input<Floating>>` is not implemented for `PB6<Input<PullUp>>`
   |
   = help: the following implementations were found:
             <PB6<MODE> as stm32f1xx_hal::gpio::Mode<MODE>>
   = note: required because of the requirements on the impl of `stm32f1xx_hal::pwm_input::Pins<Tim4NoRemap>` for `(PB6<Input<PullUp>>, PB7<Input<PullUp>>)`

error[E0277]: the trait bound `PB7<Input<PullUp>>: stm32f1xx_hal::gpio::Mode<Input<Floating>>` is not satisfied
  --> src\main.rs:95:68
   |
95 |         let encoder = Timer::tim4(dp.TIM4, &clocks, &mut rcc.apb1).qei(
   |                                                                    ^^^ the trait `stm32f1xx_hal::gpio::Mode<Input<Floating>>` is not implemented for `PB7<Input<PullUp>>`
   |
   = help: the following implementations were found:
             <PB7<MODE> as stm32f1xx_hal::gpio::Mode<MODE>>
   = note: required because of the requirements on the impl of `stm32f1xx_hal::pwm_input::Pins<Tim4NoRemap>` for `(PB6<Input<PullUp>>, PB7<Input<PullUp>>)`

Is there a way to do this safely and within the confines of the library? Note that this configuration seems to be valid as I have tested it using a C program generated in STM32CubeIDE and the behaviour is as I expect it.

@FLamparski
Copy link
Author

I'm guessing the unsafe workaround looks like this:

        // let enc_a = gpiob.pb6.into_pull_up_input(&mut gpiob.crl);
        unsafe { PB6::<Input<PullUp>>::set_mode(&mut gpiob.crl) };
        // let enc_b = gpiob.pb7.into_pull_up_input(&mut gpiob.crl);
        unsafe { PB7::<Input<PullUp>>::set_mode(&mut gpiob.crl) };

@Windfisch
Copy link
Contributor

Seems like a bug in pwm_input.rs, which implements Pins<REMAP> only for those GPIOs whose Mode = Input<Floating>. I guess it could work to replace the Floating by two PullState1/2 type parameters, so it accepts just any Input<Whatever>.

Do you see any problems with that?

@burrbull
Copy link
Member

burrbull commented Aug 5, 2021

Sorry. I've not seen this issue. What all allowed pin modes? Are they same for PwmPin? Could you make PR?

For any input you can replace it with:

impl<TIM, REMAP, P1, P2, MODE> Pins<REMAP> for (P1, P2)
where
    REMAP: Remap<Periph = TIM>,
    P1: Ch1<REMAP> + gpio::PinExt<Mode = Input<MODE>>,
    P2: Ch2<REMAP> + gpio::PinExt<Mode = Input<MODE>>,
{
}

@Windfisch
Copy link
Contributor

I am gonna look into it and will open a PR this weekend if I find the time

@Windfisch
Copy link
Contributor

Windfisch commented Aug 8, 2021

From reading the reference manual, I cannot see a reason why any pullup/down/floating restrictions should be made on any "alternate function" input pin (which aren't really in any "alternate function" mode on this chip).
Also, I could not see any reason why any peripheral that wants to drive an Alternate pin would make any restrictions on the pin being Push-Pull or OpenDrain.

For this cause, I would assume that Input<Mode> is valid for any Mode. PR incoming.

Update:
Chapter 9.1.4 in the reference manual clarifies this:
"For alternate function inputs, the port must be configured in Input mode (floating, pullup or pull-down) and the input pin must be driven externally.
For alternate function outputs, the port must be configured in Alternate Function Output mode (Push-Pull or Open-Drain).
For bidirectional Alternate Functions, the port bit must be configured in Alternate Function Output mode (Push-Pull or Open-Drain). In this case the input driver is configured in input floating mode."

(Highlights by myself)

@Windfisch
Copy link
Contributor

I've created the PRs #357, #359 and #358, which incrementally lift the Input<...> and Alternate<...> restrictions for most peripherals. (I did not touch USB, and I2C remains OpenDrain-only).

Please prefer to review and merge #358.

@bors bors bot closed this as completed in afd8087 Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants