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: do not overwrite interrupt handler #2486

Merged
merged 10 commits into from
Nov 8, 2024
Merged

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Nov 7, 2024

Another idea I'd want some feedback on. Closes #2009 I hope.

Why not keep this in Io::new? I intend to move the Pins out into Peripherals so that users don't need to create Io - unless they want to handle user interrupts. That is a bigger change, though.

@bugadani bugadani marked this pull request as draft November 7, 2024 20:23
@bugadani bugadani marked this pull request as ready for review November 8, 2024 08:56
@bugadani bugadani changed the title GPIO: auto-bind interrupt handler GPIO: do not overwrite interrupt handler Nov 8, 2024
@jessebraham
Copy link
Member

I'm not sure I understand these changes, it seems to solve one problem while creating another. As far as I can tell, if you do not want to bind the interrupt handler you can no longer call esp_hal::init()? This will result in a large amount of boilerplate required for initializing the WDTs, clocks, etc. in this case.

@bugadani
Copy link
Contributor Author

bugadani commented Nov 8, 2024

In which case do you explicitly not want to bind any interrupt handlers?

@jessebraham
Copy link
Member

In which case do you explicitly not want to bind any interrupt handlers?

When using e.g. RTIC and using hardware tasks involving GPIO. This was the reason the Io::new_no_bind_interrupt() constructor was added in the first place. I've confirmed that using this branch and calling esp_hal::init() causes the sw_and_hw example to stop functioning as expected.

@bugadani
Copy link
Contributor Author

bugadani commented Nov 8, 2024

RTIC seems to be working for me now, please take another look. If the user doesn't want our handler, they will be able to disable it using the interrupt APIs. We are now detecting whether the peripheral interrupt is bound to any CPU interrupt, so it shouldn't be possible for us to overwrite any early bound interrupt handlers. Later the users can swap ours with anything they want using the esp_hal::interrupt APIs.

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.

Can confirm things are working as expected again, thanks for taking care of that!

@bugadani bugadani mentioned this pull request Nov 8, 2024
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

@bjoernQ bjoernQ added this pull request to the merge queue Nov 8, 2024
Merged via the queue into esp-rs:main with commit e10ae2d Nov 8, 2024
28 checks passed
@bugadani bugadani deleted the gpio-async branch November 8, 2024 17:21
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.

The Io struct has multiple constructors which makes it confusing to use correctly
3 participants