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

Add parameter for enabling pull-up and pull-down in RP PWM input mode #2476

Merged

Conversation

pawel00100
Copy link
Contributor

I created this simple change that allows pin B (the one that can be used as PWM counter input) to be set with pull-up or pull-down internal resistor.

I made tests for two most important cases. I was thinking about adding extra test for checking if the input is floating by default, but it even in case of code being wrong test could on occasion pass so I decided not to add it:

    // PWM input is floating by default
    {
        let pin2 = Input::new(&mut p11, Pull::Down);
        let pwm = Pwm::new_input(&mut p.PWM_CH3, &mut p7, Pull::None, InputMode::FallingEdge, cfg.clone());
        Timer::after_millis(1).await;
        assert!(pin2.is_low());
    }
    {
        let pin2 = Input::new(&mut p11, Pull::Up);
        let pwm = Pwm::new_input(&mut p.PWM_CH3, &mut p7, Pull::None, InputMode::FallingEdge, cfg.clone());
        Timer::after_millis(1).await;
        assert!(pin2.is_high());
    }

Alternative test for this case could be to use ADC with pullup/down and check voltage levels, in case the voltage was in the middle it would mean that an opposite pullup/down resistor was enabled when it shouldn't, but it would require rewiring test board and would be in my opinion a bit overkill.

Please note that I'm barely experienced in Rust and I'm an amateur in EE/embedded field so I'd advice extra caution reviewing the proposed change.

@@ -93,6 +93,7 @@ impl<'d, T: Channel> Pwm<'d, T> {
inner: impl Peripheral<P = T> + 'd,
a: Option<PeripheralRef<'d, AnyPin>>,
b: Option<PeripheralRef<'d, AnyPin>>,
b_pull: Pull,
Copy link
Member

Choose a reason for hiding this comment

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

could you make this a field in Config? this way the number of params doesn't explode and it doesn't increase verbosity for users that don't need the pull.

(yes, divmode should've been a field in config too...)

Copy link
Member

Choose a reason for hiding this comment

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

actually no, it's better to have it separate so it's only present in the new_ constructots that use inputs, since it doesnt' make sense for outputs.

(same for divmode, that's why it was that way...)

@Dirbaio Dirbaio force-pushed the rp_pwm_input_pull-up_and_pull-down branch from 4a34962 to 2b9a5f1 Compare April 4, 2024 23:10
Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

thank you!

@Dirbaio Dirbaio force-pushed the rp_pwm_input_pull-up_and_pull-down branch 3 times, most recently from 95c9215 to ac73029 Compare April 4, 2024 23:14
@Dirbaio Dirbaio enabled auto-merge April 4, 2024 23:14
@Dirbaio Dirbaio force-pushed the rp_pwm_input_pull-up_and_pull-down branch from ac73029 to 143b288 Compare April 4, 2024 23:20
@Dirbaio Dirbaio added this pull request to the merge queue Apr 4, 2024
Merged via the queue into embassy-rs:main with commit 485bfd1 Apr 4, 2024
6 checks passed
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.

2 participants