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

Bind "IRQ handlers" to GPIO pins #2146

Closed
wants to merge 4 commits into from
Closed

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Sep 11, 2024

Second attempt at this.

We now register interrupt handlers on-demand. Surprising behaviour may be:

  • The GPIO interrupts are handled at the maximum configured level. This is probably the point I have the most issues with.
  • Calling async functions overwrite any configured interrupt handlers. As an alternative, we could try restoring the handler, keeping both handlers in parallel and re-enabling the IRQ, or simply panicking if a user handler is set. I'm undecided, but "replace and keep working" is as equally valid a strategy as any.
  • There is no way to globally replace the IRQ handler using runtime binding. I don't find this a big negative.

All documentation is missing because it wasn't necessary to demonstrate the idea.

cc #2009

@bugadani bugadani changed the title Bind IRQ handlers to GPIO pins "Bind" IRQ handlers to GPIO pins Sep 11, 2024
@bugadani bugadani force-pushed the pinterrupt branch 2 times, most recently from 869c201 to c2a1f99 Compare September 11, 2024 18:30
@bugadani bugadani changed the title "Bind" IRQ handlers to GPIO pins Bind "IRQ handlers" to GPIO pins Sep 11, 2024
@bugadani bugadani force-pushed the pinterrupt branch 7 times, most recently from cbb84c6 to e26be16 Compare September 11, 2024 20:32
Self: GpioProperties,
{
fn set_interrupt_handler(&mut self, handler: InterruptHandler) {
raise_gpio_interrupt_priority(handler.priority());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I have a vauge feeling this won't play nicely with this idea. https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-reference/system/intr_alloc.html#iram-safe-interrupt-handlers .

There should be some way to force users to choose a single priority from the start.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm slightly afraid "the priority you set here will be ignored" isn't what we are aiming for, but it's something to consider.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or perhaps this shouldn't take InterruptHandler as a parameter? Since this isn't a real interrupt and that way there's nothing to ignore here

@bugadani bugadani force-pushed the pinterrupt branch 6 times, most recently from 858ec30 to 4fa457a Compare September 13, 2024 08:29
@bugadani bugadani marked this pull request as ready for review September 13, 2024 12:09
@bugadani bugadani force-pushed the pinterrupt branch 4 times, most recently from 42b3ebf to b9b7c34 Compare September 16, 2024 11:20
@MabezDev
Copy link
Member

Sorry I haven't looked at this yet, I still need to think about this some more. Particularly, "The GPIO interrupts are handled at the maximum configured level" seems like a recipe for confusion, but I need to spend a bit more time thinking about what kind of footguns this may or may not cause in reality.

@bjoernQ
Copy link
Contributor

bjoernQ commented Sep 16, 2024

I see how this can be convenient for the user - probably I'd prefer the user to not pass an InterruptHandler but a plain function and setting the priority separately

However, I think we want the user to be able to register a custom global GPIO handler - especially via enable_direct in case they need low latency (haven't measure but this adds a little bit of additional latency)

Mabe I'm missing something and it's possible but unfortunately this PR contains quite some "noise" so maybe I'm wrong

@bugadani
Copy link
Contributor Author

Users can use esp_hal::interrupt::bind_interrupt I believe if they need to.

@bugadani
Copy link
Contributor Author

As for "take a function pointer", there's #2110 (comment) . On one hand, there's API consistency (and convenience by doing things automagically). On the other, there's the need to not be confusing, but without some priority we can't late-bind the gpio handler like this PR tries to do.

We can set the priority in the Io constructor (or .pins() in this PR) but setting the priority of an unbound interrupt handler seems funny to me and may be problematic with RTIC still.

@bugadani bugadani closed this Oct 6, 2024
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