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

Add attachInterrupt to MIDIUSB.h #66

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

studiohsoftware
Copy link

Add attachInterrupt method to MIDIUSB.h so that sketch can define a callback function.

Example sketch included.

@PaulStoffregen
Copy link

Is this SAMD-specific code in the example meant to be part of the public API?

    USB->DEVICE.INTENCLR.reg = 0xFF; //SAMD interrupts continuously without this.

@studiohsoftware
Copy link
Author

studiohsoftware commented Dec 13, 2020 via email

@facchinm
Copy link
Contributor

The proposed solution is indeed samd-only since the handleEndpoint callback is not part of PluggableUSB generic APIs (beside the line to disable continuous interrupts which should be fixed at core level here ).
I like the idea but the library is intended to run on a few platforms and should behave the same on all of them, or at least limitations should be documented.

@studiohsoftware
Copy link
Author

studiohsoftware commented Dec 14, 2020

This PR is not about issue #65. It is about adding attachInterrupt to MIDIUSB.h for its own sake, using the fact that handleEndpoint is in the the public section of PluggableUSB.h. The example sketch is just showing how to use the proposed attachInterrupt method, it's not about the content of the callback.

I think you are saying that handleEndpoint in PluggableUSB.h is only supported for SAMD. If that's the case, it is my mistake, and this PR can be disregarded.

@PaulStoffregen
Copy link

My main concern is for any public API to not require CPU specific hacks, especially not writing to specific peripheral registers.

A secondary concern is exposing more use of interrupt context via public APIs. Arduino already has plenty of precedent for this with functions like attachInterrupt(). But use of interrupt context is error prone and difficult to do correctly, even for experts.

Sadly, Arduino lacks any sort of API where events can be triggered from interrupt context and then later cause user code to run in main program context. @facchinm, @cmaglie and I have talked about this before, but it's a daunting project to add such a large and consequential API to Arduino.

I know it is easy to dismiss concerns about the safety of interrupt context. But consider that even this example makes several calls to Serial1.print() from its interrupt function. Is that necessarily safe? What happens if any other part of the program also uses Serial1? What happens if events occur so rapidly that Serial1's transmit buffer fills up? Even if this is always safe on SAMD, will Serial1 use work on other platforms? Calling other libraries from a different interrupt than they use is a recipe for future bugs.

Arduino as a platform really does need a safe API for libraries like MIDIUSB to have users a way to respond to events....

@studiohsoftware
Copy link
Author

studiohsoftware commented Dec 14, 2020 via email

@CLAassistant
Copy link

CLAassistant commented Apr 9, 2021

CLA assistant check
All committers have signed the CLA.

@per1234 per1234 added topic: code Related to content of the project itself type: enhancement Proposed improvement labels Jan 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants