Remove automatic interrupt attachment #30
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Moving away from Arduino-ness conventions
Having the class functions share the name of the global function was good for intuitiveness, but causes issues on platforms that implement the global 'attachInterrupt' as something other than a function (see #27). Removing 'Interrupt' from the name also makes these functions match their counterparts in the Servo library.
This was causing issues on certain platforms where the platform's interrupt attachment system was not set up or available when the object was constructed in global scope (see #28). Instead, the user must attach() the object themselves before they can read signals. The library examples have been updated accordingly.
Uses a reference counter to detach the platform interrupt if no class instances remain.
The flag indicates whether the interrupt is currently attached, and will prevent the attach/detach functions from calling the platform functions unless it's to change state. This guards against two edge cases. The first is if the user calls 'attach()' multiple times and the platform does something unexpected when calling the 'attachInterrupt()' function repeatedly. It shouldn't, but you never know and it's something outside of our control. Better to play it safe. The second, and even more of an edge case, is if the user defines their own interrupt for the pin (not using the library 'attach()' function), and the last class instance is deallocated. This avoids the destructor automatically calling 'detach()' and removing the user's interrupt function. I can't imagine that occuring in practice, but it's a potential pitfall nonetheless.
This is now required for the library to work (c8d7925).
This was referenced Jan 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order to read servo signals, the library attaches an interrupt handler to an interrupt-capable pin that fires whenever the pin changes state. The library reads the rising/falling edges and calculates the time elapsed between the two to find the length of the servo pulses.
On some platforms, notably the Uno R4 (#28) and ESP32 (#19) the interrupt request (IRQ) table is not set up until the
main()function is entered. The version 1 library idiom, following Arduino-y convention, is to declareServoInputPinobjects as globals. These objects call the interrupt handler during the constructor to apply the function hook. Because the request table is not setup untilmain(), these calls fail and the library will not work until the user tries to attach the interrupt themselves.This PR removes this call to attach the interrupt in the constructor, and instead requires the user do it themselves before the library can begin parsing signals. This is a breaking change to the API, and necessitates a new major library version.
Because of that, I've taken the opportunity to make a few other breaking changes:
attachInterrupt()anddetachInterrupt()functions have been refactored to simplyattach()anddetach(). This matches the syntax of theServolibrary, and should fix issues with platforms that use macros instead of global functions for their interrupt handler (Compilation Issue on Arduino Nano ESP32 #27).booleantypes have been refactored to simplybool, which is the proper C typeSince the user is now responsible for interrupt attachment/detachment, I've also added reference counting that will automatically detach the interrupt whenever no more class instances for a given pin exist.