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

GPIO Refactoring #1542

Merged
merged 5 commits into from
May 15, 2024
Merged

GPIO Refactoring #1542

merged 5 commits into from
May 15, 2024

Conversation

bjoernQ
Copy link
Contributor

@bjoernQ bjoernQ commented May 7, 2024

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • My changes were added to the CHANGELOG.md in the proper section.

Extra:

Pull Request Details 📖

Description

This is a huge PR with dramatic changes. I also have some ideas for a follow-up PR but I guess this is more than enough to review already.

  • Pin / OutputPin / InputPin are now not accessible to users anymore
    • they probably should never have been
    • gives us more freedom for internal refactorings in future
    • maybe there was something users would wish to be allowed to do - we can decide case-by-case if there any requests for it
  • drivers for Output / Input / OutputOpenDrain
  • drivers for AnyOutput / AnyInput / AnyOutputOpenDrain without any generics
  • AnyPin can be used for peripherals
  • most drivers initialized the pins but some relied on the user to configure the pins - now all drivers initialize their pins (despite LpUart and LpI2c - something for a follow-up PR)

Most probably this will need a migration-guide for the next release but probably best to wait for the review (and probably for a potential follow-up PR)

Testing

All examples and tests work

@bjoernQ bjoernQ marked this pull request as ready for review May 7, 2024 14:06
@@ -42,7 +42,7 @@ pub struct SystemTimer<'d, DM: crate::Mode> {
pub alarm0: Alarm<Target, DM, 0>,
pub alarm1: Alarm<Target, DM, 1>,
pub alarm2: Alarm<Target, DM, 2>,
_phantom: &'d PhantomData<()>,
_phantom: PhantomData<&'d ()>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unrelated to this PR but makes this ZST

// Begin configuring the TWAI peripheral. The peripheral is in a reset like
// state that prevents transmission but allows configuration.
let mut can_config = twai::TwaiConfiguration::new(
let mut can_config = twai::TwaiConfiguration::new_no_transceiver(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't really like it but probably not too bad

}

/// Trait implemented by pins which can be used as outputs
pub trait OutputPin: Pin {
/// Configure open-drain mode
fn set_to_open_drain_output(&mut self) -> &mut Self;
fn set_to_open_drain_output(&mut self, _: private::Internal);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

returning &mut Self makes it inconvenient for delegation - since it's an internal API I don't think this is a problem

Copy link
Contributor

@JurajSadel JurajSadel left a comment

Choose a reason for hiding this comment

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

I did initial review, overall looks really good! I left a few comments, some of them might not make any sense, so feel free to ignore them.
I will go through the changes again and I will test more examples as well.
Thanks for working on this!

esp-hal/src/gpio/lp_io.rs Show resolved Hide resolved
esp-hal/src/gpio/mod.rs Outdated Show resolved Hide resolved
esp-hal/src/gpio/mod.rs Outdated Show resolved Hide resolved
esp-hal/src/gpio/mod.rs Show resolved Hide resolved
esp-hal/src/gpio/mod.rs Outdated Show resolved Hide resolved
esp-hal/src/gpio/mod.rs Outdated Show resolved Hide resolved
esp-hal/src/gpio/rtc_io.rs Show resolved Hide resolved
esp-hal/src/twai/mod.rs Show resolved Hide resolved
esp-hal/src/twai/mod.rs Show resolved Hide resolved
examples/src/bin/blinky.rs Show resolved Hide resolved
@bjoernQ bjoernQ force-pushed the refactor-gpio branch 2 times, most recently from 739e382 to 29b5230 Compare May 14, 2024 09:28
Copy link
Contributor

@JurajSadel JurajSadel left a comment

Choose a reason for hiding this comment

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

LGTM, thank you very much!

Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

This is a really nice change! Thanks for looking into this, I only have one suggestion around adc, we could choose to address this in another PR if needed though.

esp-hal/src/gpio/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

LGTM, thanks again for working on this!

@MabezDev MabezDev added this pull request to the merge queue May 15, 2024
Merged via the queue into esp-rs:main with commit 2faa265 May 15, 2024
22 checks passed
@bjoernQ bjoernQ deleted the refactor-gpio branch November 26, 2024 08:43
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.

4 participants