-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Serial attach interrupt #299
base: master
Are you sure you want to change the base?
Conversation
added optional custom handler to Serial interrupt. Is minor enhancement of NeoHWSerial by SlashDevin (https://github.com/SlashDevin/NeoHWSerial), as user function also gets UART status as parameter to e.g. detect LIN break in user routine. Added as core extension instead of library due to similar extension for Due (https://github.com/gicking/ArduinoCore-sam) which AFAIK cannot be implemented as library.
fixed compiler warnings when compiling with max. waring level
add Serial.attachInterrupt_Send() rename Serial.attachInterrupt() to Serial.attachInterrupt_Receive()
optimized runtime for TXC custom function
minor cosmetics
- updated modification date to sync with SAM core - no functional change
Are the user functions called from interrupt context? |
yes. I need this for LIN master and LIN slave in background. |
As a general purpose Arduino API, exposing users to more interrupt context callbacks (the API already has some), especially for data handling, is a huge mistake. |
may I ask why? Specifically how do I detect a LIN sync break if the normal receive interrupt "forgets" the status byte and only allows polling operation? SerialEvent() also doesn't help. |
actually this behaves similar to attachInterrupt(), only for Serial receive and transmit. So what's your concern exactly? |
I do advocate these changes, as the TX-empty interrupt has to be used for RS485 transmitter disable. |
@PaulStoffregen: I can see you point. Actually I started this as an extension of the NeoHWSerial library, i.e. not a core extension. And for AVR obviously a library works, as NeoHWSerial shows. However, I had trouble getting it to work for the Arduino Due. If you see a way to define new "NeoSerial" instances which don't collide with the original Serial's, please let me know. I would be happy to do this as a library instead. PS: for the required changes for Due please see here |
For AVR, we applied a trick defining each Serial instances along with their ISR(s), which combined well with the .a file compilation done for the core, results in the ISR only being pulled in if the corresponding serial object is actually used. However, even without that trick, if you would not use any Serial object, the ISRs should not be pulled in and you would be free to use a third party library for serial. Does that sound familiar? If it works when not using any serial objects, then we might be able to apply the same trick to the Due core as well to better separate the serial objects? |
It might be possible to leave the interrupt context on the AVR by just calling sei() before executing the user interrupt function. This way no further interrupts are going to be blocked. I would also endorse adding an AttachInterrupt() function to the official Arduino libraries as this seems to be a commonly useful functionality to me. Relying on third party implementations for such a small change (related amount of code changed/added) will only mean increased maintaining effort. |
I agree with kcl93. The proposed core extension is very small and doesn't do anything unless the user explicitly attaches a callback function to the Serial handlers. IMHO this is a very useful - and sometimes necessary - functionality. Implementing it as a library creates a big maintenance effort, as e.g. the SAM core still seems to be "dynamic". Also, if these became standard API functions, the developers for other, also non-Arduino boards, would also implement them over time. But if not, someone would need to create and maintain a respective library for each board, which may not happen and may even not be possible (as currently on Due). I know that I am biased on this topic, but still I advocate to add these 2 functions to the standard Arduino-API. Whether or not this is done via merging this pull request, or implemented by a proper programmer, I don't really care. Thanks a lot and have a great day :-) @matthijskooijman: anyway, if whoever is in charge of deciding about this question decides against my above proposal, could you then please separate Serial the way you described? With your advice I would then create and maintain a respective library. PS: who does decide about such an API change anyway? I assume there is some kind of "decision board" to assert a stable API...? And how long does such a decision generally take? |
@matthijskooijman: coming back to your comment from Nov 24th. For me it would be acceptable (actually preferred) to separate Serial from the rest of the code, like for the AVRs. That would allow replacing the built-in UART-ISRs via a library, similar to NeoHWSerial for the AVRs. Any idea how to implement this - or maybe even a rough timeline...? |
I do no think there is an official (documented) process for this. Discussing on the arduino-developers mailing list is usually suggested, though the core developers do not participate so actively there (though they do read it, I think). In the end, it is up to the core devs such as @cmaglie or @facchinm to decide whether or not to include something, but they have a lot on their plate already. If this is something that should be added to all Arduino platforms (i.e. when it is a portable API, https://github.com/arduino/ArduinoCore-API/ might be the right place to discuss this (and it seems there is already a virtual HardwareSerial base class there).
I will not have the time to actually implement this, too much things on my plate already. However, if anyone else wants to take a stab at this, I'm happy to support them with advice and reviews. To keep the discussion clean, I created new issues for the "separate serial objects" implementation here: arduino/ArduinoCore-sam#100 and arduino/ArduinoCore-samd#489. Let's keep this issue about adding the serial interrupt API. |
@matthijskooijman: if the serials for SAM can be implemented similar to AVR, Rx- and Tx-interrupts can be added via a library like NeoHWSerial. In that case I would withdraw this pull request. For now I propose to leave it open and discuss it in the above issues arduino/ArduinoCore-sam#100 and arduino/ArduinoCore-samd#489, you created. |
seriously? It takes 16 months to automatically send a legal form...??? |
added optional argument pointer, see arduino/ArduinoCore-sam#95 (comment)
Memory usage change @ ef9df08
Click for full report table
Click for full report CSV
|
Added option to attach custom functions to Serial receive and transmit interrupts.
Receive is similar to NeoHWSerial by SlashDevin but
Transmit is new:
Attached please find an example (tested for Mega and Due). Connect Tx1 & Rx1 and check serial monitor. A separate pull request was started for SAM (see here).
Serial_attach_interrupt.zip