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

Use set_interrupt_handler() everywhere and consider creating a trait #1489

Closed
MabezDev opened this issue Apr 21, 2024 · 3 comments · Fixed by #1819
Closed

Use set_interrupt_handler() everywhere and consider creating a trait #1489

MabezDev opened this issue Apr 21, 2024 · 3 comments · Fixed by #1819
Assignees
Labels
peripheral:interrupt Interrupt peripheral(s)
Milestone

Comments

@MabezDev
Copy link
Member

Imo after some time playing with it, the constructors taking Option<InterruptHandler> are not that pleasant to use. I think it would be better if everywhere was consistent and used set_interrupt_handler. I think having this in a trait would also be good for creating abstractions over this.

@bjoernQ
Copy link
Contributor

bjoernQ commented Apr 22, 2024

related to #1478

While implementing the changes to the constructors it also felt not like a very nice change API wise

The good thing about the ctor taking the interrupt handler would be the user can be sure nothing will change (well - we have unsafe API to change it ... so it's not a strong gurarantee). Besides that, consistently having a set_interrupt_handler should be a nicer option in general

Not too sure about having a trait for that - it won't hurt but in most cases the user code will need to know exactly what interrupt handler it's dealing with - but we can add such a trait and see how useful it is in the end

@Dominaezzz
Copy link
Collaborator

In the case of PARL_IO and LCD_CAM where the driver splits in two, I think these two are stuck with c'tor way since both drivers share an interrupt.
Unless there's some way to have multiple handlers with a mask or something

@MabezDev
Copy link
Member Author

MabezDev commented May 2, 2024

The good thing about the ctor taking the interrupt handler would be the user can be sure nothing will change (well - we have unsafe API to change it ... so it's not a strong guarantee)

I agree, plus it's generally quite easy to spot when it's changed by adding a log statement in set_interrupt_handler.

In the case of PARL_IO and LCD_CAM where the driver splits in two, I think these two are stuck with c'tor way since both drivers share an interrupt.

set_interrupt_handler is already quite a low level idea, because dealing with interrupts manually assumes a lot of domain knowledge anyways - I think most users would prefer an async API. So I think just having set_interrupt_handler on LcdCam with docs saying you as the user have to manage handling both "sides" of the peripheral would be fine. Once, LcdCam has been split it won't be possible to (safely) update the interrupt handler.

@jessebraham jessebraham added the peripheral:interrupt Interrupt peripheral(s) label Jul 15, 2024
@tom-borcin tom-borcin added this to the 0.20.0 milestone Jul 15, 2024
@bjoernQ bjoernQ self-assigned this Jul 16, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in esp-rs Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
peripheral:interrupt Interrupt peripheral(s)
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants