Tca955x - Fixed bug with interrupt state monitoring

[felixcollins]

The interrupt state monitoring in the driver is not ideal. I hit a bug where the previous pin state had not been set before an interrupt was received from the chip. Not surprising really as any line on the chip could cause the interrupt. By initialising the pin state dict before signing up for the interrupt we will avoid this issue.

This PR also removes the lock in the end of the interrupt handling task and substitutes using a ConcurrentDictionary. The way it was being done, by copying the whole pin state dict, could lead to concurrency bugs due to pin states added during the task running being effectively removed when the whole dict was replaced.

Microsoft Reviewers: Open in CodeFlow

[raffaeler]

Any reason why you prefer the lock instead of the ConcurrentDictionary for this as well?

[Copilot]

Pull Request Overview

This PR fixes interrupt state monitoring bugs in the Tca955x GPIO driver. The main issue addressed is a race condition where interrupts could occur before the driver's internal state was properly initialized, leading to KeyNotFoundException when accessing pin states.

• Initialize interrupt state dictionaries during construction to prevent race conditions
• Replace lock-based state updates with ConcurrentDictionary to avoid concurrency bugs
• Rename variables for better clarity and fix event handler management logic

[Copilot]

The TryUpdate call uses !interruptLastInputValuesSnapshot[pin] as the comparison value, which appears incorrect. TryUpdate expects the current value to compare against, not the inverse of the new value. This should likely be the actual current value from the ConcurrentDictionary or use TryAdd/AddOrUpdate instead.

Suggested change

	_interruptLastInputValues.TryUpdate(pin, interruptLastInputValuesSnapshot[pin], !interruptLastInputValuesSnapshot[pin]);
	_interruptLastInputValues.TryUpdate(pin, interruptLastInputValuesSnapshot[pin], interruptLastInputValuesSnapshot[pin]);

[Copilot]

Similar to line 507, this TryUpdate call uses !currentValue as the comparison value, which is incorrect. The third parameter should be the expected current value in the dictionary, not the inverse of the new value being set.

Suggested change

	_interruptLastInputValues.TryUpdate(pinNumber, currentValue, !currentValue);
	_interruptLastInputValues.TryUpdate(pinNumber, currentValue, currentValue);

[felixcollins]

The locks were in there when I got here. I think we could do away with them almost entirely. Given that the class is not supposed to be thread safe, the only concurrency is in the interrupt event handler which comes on another thread. I am away until about October 6. Will have another look then. Feel free to make changes. Yahoo Mail: Search, organise, conquer On Fri, 26 Sep 2025 at 3:38 AM, ***@***.***> wrote: @Copilot commented on this pull request. Pull Request Overview This PR fixes interrupt state monitoring bugs in the Tca955x GPIO driver. The main issue addressed is a race condition where interrupts could occur before the driver's internal state was properly initialized, leading to KeyNotFoundException when accessing pin states. - Initialize interrupt state dictionaries during construction to prevent race conditions - Replace lock-based state updates with ConcurrentDictionary to avoid concurrency bugs - Rename variables for better clarity and fix event handler management logic In src/devices/Tca955x/Tca955x.cs:

@@ -494,9 +502,9 @@ private Task ProcessInterruptInTask()

interruptLastInputValuesSnapshot[pin] = newValue; } - lock (_interruptHandlerLock) - { - _interruptLastInputValues = interruptLastInputValuesSnapshot; + foreach (var pin in interruptLastInputValuesSnapshot.Keys) + { + _interruptLastInputValues.TryUpdate(pin, interruptLastInputValuesSnapshot[pin], !interruptLastInputValuesSnapshot[pin]); The TryUpdate call uses !interruptLastInputValuesSnapshot[pin] as the comparison value, which appears incorrect. TryUpdate expects the current value to compare against, not the inverse of the new value. This should likely be the actual current value from the ConcurrentDictionary or use TryAdd/AddOrUpdate instead. ⬇️ Suggested change- _interruptLastInputValues.TryUpdate(pin, interruptLastInputValuesSnapshot[pin], !interruptLastInputValuesSnapshot[pin]); + _interruptLastInputValues.TryUpdate(pin, interruptLastInputValuesSnapshot[pin], interruptLastInputValuesSnapshot[pin]); In src/devices/Tca955x/Tca955x.cs:

@@ -557,16 +565,13 @@ protected override void AddCallbackForPinValueChangedEvent(int pinNumber, PinEve

lock (_interruptHandlerLock) { - if (_interruptPins.ContainsKey(pinNumber)) - { - throw new InvalidOperationException($"A callback is already registered for pin {pinNumber}"); - } - else + _interruptPinsSubscribedEvents[pinNumber] = eventType; + var currentValue = Read(pinNumber); + _interruptLastInputValues.TryUpdate(pinNumber, currentValue, !currentValue); Similar to line 507, this TryUpdate call uses !currentValue as the comparison value, which is incorrect. The third parameter should be the expected current value in the dictionary, not the inverse of the new value being set. ⬇️ Suggested change- _interruptLastInputValues.TryUpdate(pinNumber, currentValue, !currentValue); + _interruptLastInputValues.TryUpdate(pinNumber, currentValue, currentValue); — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: ***@***.***>

[felixcollins]

@Copilot I think you are wrong in this case. Interestingly you are wrong in exactly the way a human reading this code might be wrong if they didn't check the documentation carefully. ConcurrentDictionary.TryUpdate(TKey, TValue, TValue) Method (System.Collections.Concurrent) | | | | | | | | | | | ConcurrentDictionary<TKey,TValue>.TryUpdate(TKey, TValue, TValue) Method (System.Collections.Concurrent) Updates the value associated with key to newValue if the existing value with key is equal to comparisonValue. | | | | The documentation is terrible and feels like it was autogenerated rather than written for human comprehension. My understanding is that the update takes place if the comparison value is equal to the existing value. Given that I'm storing a bool, updating only occurs if the new value is the complement of the existing value.  I'd appreciate it if a human could double check this, given that copilot seems to be trying to put me wrong. Thanks, Felix Yahoo Mail: Search, organise, conquer On Fri, 26 Sep 2025 at 3:38 AM, ***@***.***> wrote: @Copilot commented on this pull request. Pull Request Overview This PR fixes interrupt state monitoring bugs in the Tca955x GPIO driver. The main issue addressed is a race condition where interrupts could occur before the driver's internal state was properly initialized, leading to KeyNotFoundException when accessing pin states. - Initialize interrupt state dictionaries during construction to prevent race conditions - Replace lock-based state updates with ConcurrentDictionary to avoid concurrency bugs - Rename variables for better clarity and fix event handler management logic In src/devices/Tca955x/Tca955x.cs:

@@ -494,9 +502,9 @@ private Task ProcessInterruptInTask()

interruptLastInputValuesSnapshot[pin] = newValue; } - lock (_interruptHandlerLock) - { - _interruptLastInputValues = interruptLastInputValuesSnapshot; + foreach (var pin in interruptLastInputValuesSnapshot.Keys) + { + _interruptLastInputValues.TryUpdate(pin, interruptLastInputValuesSnapshot[pin], !interruptLastInputValuesSnapshot[pin]); The TryUpdate call uses !interruptLastInputValuesSnapshot[pin] as the comparison value, which appears incorrect. TryUpdate expects the current value to compare against, not the inverse of the new value. This should likely be the actual current value from the ConcurrentDictionary or use TryAdd/AddOrUpdate instead. ⬇️ Suggested change- _interruptLastInputValues.TryUpdate(pin, interruptLastInputValuesSnapshot[pin], !interruptLastInputValuesSnapshot[pin]); + _interruptLastInputValues.TryUpdate(pin, interruptLastInputValuesSnapshot[pin], interruptLastInputValuesSnapshot[pin]); In src/devices/Tca955x/Tca955x.cs:

@@ -557,16 +565,13 @@ protected override void AddCallbackForPinValueChangedEvent(int pinNumber, PinEve

lock (_interruptHandlerLock) { - if (_interruptPins.ContainsKey(pinNumber)) - { - throw new InvalidOperationException($"A callback is already registered for pin {pinNumber}"); - } - else + _interruptPinsSubscribedEvents[pinNumber] = eventType; + var currentValue = Read(pinNumber); + _interruptLastInputValues.TryUpdate(pinNumber, currentValue, !currentValue); Similar to line 507, this TryUpdate call uses !currentValue as the comparison value, which is incorrect. The third parameter should be the expected current value in the dictionary, not the inverse of the new value being set. ⬇️ Suggested change- _interruptLastInputValues.TryUpdate(pinNumber, currentValue, !currentValue); + _interruptLastInputValues.TryUpdate(pinNumber, currentValue, currentValue); — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: ***@***.***>

[raffaeler]

@felixcollins Yes, the changes were already there, but I believe there is the opportunity to make this code a bit better :) I will try to trigger the copilot again to see what happens

[raffaeler]

@copilot read the latest comments and fix your review. Add comments to clarify the intent of the new or revised code.

[Copilot]

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[krwq]

[Triage] We discussed this briefly and this is good as is. We can address anything else separately