Use only one interrup callback for one chip. #2408
Conversation
dotnet-policy-service
bot
added
the
area-device-bindings
src/devices/Tca955x/Tca955x.cs
Outdated
| @@ -434,7 +434,12 @@ protected override void AddCallbackForPinValueChangedEvent(int pinNumber, PinEve | |||
|
|
|||
| _interruptPins.Add(pinNumber, eventType); | |||
| _interruptLastInputValues.Add(pinNumber, Read(pinNumber)); | |||
| _controller.RegisterCallbackForPinValueChangedEvent(_interrupt, PinEventTypes.Falling, InterruptHandler); | |||
|
|
|||
| // Only register the callback if this is the first add callback | |||
There was a problem hiding this comment.
Can you improve a comment a bit to clarify that we need to register interrupt once for host but this logic is for extender interrupts therefore the single handler will handle all extender interrupts registrations
There was a problem hiding this comment.
(same for removal - it seems like many of us were confused about what's being fixed initially)
There was a problem hiding this comment.
I updated the comment so it should be now more clear why I register the callback for the interrupt only once
src/devices/Tca955x/Tca955x.cs
Outdated
| _controller.RegisterCallbackForPinValueChangedEvent(_interrupt, PinEventTypes.Falling, InterruptHandler); | ||
|
|
||
| // Only register the callback if this is the first add callback | ||
| if (_interruptPins.Count == 1) |
There was a problem hiding this comment.
This class is not necessarily thread safe, but these checks are very likely to fail in any concurrent environment. Is it worth it to add a lock before these checks?
There was a problem hiding this comment.
you can lock (_interruptHandlerLock) as we do elsewhere for all register/deregister
Add lock on Add and Remove Callback
src/devices/Tca955x/Tca955x.cs
Outdated
| _controller.RegisterCallbackForPinValueChangedEvent(_interrupt, PinEventTypes.Falling, InterruptHandler); | ||
| } | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
I think _eventHandlers[pinNumber] = ... shuold also be inside the lock
| { | ||
| _controller.RegisterCallbackForPinValueChangedEvent(_interrupt, PinEventTypes.Falling, InterruptHandler); | ||
| } | ||
|
|
|
I think this should just sign up when the driver is created/disposed. See my implementation in #2417 Sorry, I didn't see this PR until just now. |
|
I got carried away and made the TCA955x work better. I dropped my other PR and opened another #2420 |
Prevent multiple callbacks on the interrupt pin. Only use one inpterrupt callback.
Fixing typos.
Code is test with a Tca9555 and Tca9554
Microsoft Reviewers: Open in CodeFlow