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

Async: GPIO #333

Merged
merged 12 commits into from
Jan 27, 2023
Merged

Async: GPIO #333

merged 12 commits into from
Jan 27, 2023

Conversation

MabezDev
Copy link
Member

@MabezDev MabezDev commented Jan 6, 2023

  • Upgrade to new eha
  • Implement Wait for GpioPin
  • Add an example for C3

TODO

  • Add examples for all chips
  • Make the interrupt handler cleaner by being able to conjure a pin type

Blocked on

future work (probably not going in this PR)

  • Adding support for multi-core chips that have interrupt registration per core.

@MabezDev MabezDev force-pushed the async/gpio-input branch 4 times, most recently from b527d71 to c733731 Compare January 6, 2023 16:26
esp-hal-common/src/gpio.rs Outdated Show resolved Hide resolved
@MabezDev MabezDev marked this pull request as ready for review January 26, 2023 22:19
@MabezDev MabezDev requested review from jessebraham and bjoernQ and removed request for jessebraham January 26, 2023 22:19
Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM

Only nit-pick is, there is no explanation of what to expect from the example - we have that for most examples - but not the end of the world and shouldn't keep us from merging

@MabezDev
Copy link
Member Author

Good point, I added them for both this example and the embassy hello world :)

@MabezDev
Copy link
Member Author

MabezDev commented Jan 27, 2023

Btw what should we do about MSRV? 1.67 is only needed for async?

@bjoernQ
Copy link
Contributor

bjoernQ commented Jan 27, 2023

Btw what should we do about MSRV? 1.67 is only needed for async?

Maybe we can keep 1.65 and just say for async it's 1.67 I think I have seen that for other crates, too

Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

LGTM!

RE the MSRV, I think what @bjoernQ said is probably best, keep it as-is but mention that for async specifically it's 1.67. Once we've had another toolchain release or two we can re-visit this if needed.

}

// clear interrupts
Bank0GpioRegisterAccess::write_interrupt_status_clear(!0);
Copy link
Contributor

@bugadani bugadani May 14, 2023

Choose a reason for hiding this comment

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

@MabezDev I'm not entirely sure but I think that set_int_enable does not implicitly clear interrupt status bits, it is possible for an edge-triggered listener to wake a task twice (in this call, and at a later GPIO interrupt request).

I think it is also possible that we lose GPIO interrupt requests here, because read and clear are not atomic. Probably only those bits should be cleared here that are set in intrs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't quite understand your first point, but I do agree that we should only clear the interrupts read from the status inside the GPIO interrupt handler.

Copy link
Contributor

@bugadani bugadani May 15, 2023

Choose a reason for hiding this comment

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

I am in the process of opening an issue with more data that confirms my suspicions: level-triggered interrupts fire multiple times, it looks like. But I might just have incorrect assumptions here, we'll see :)

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