Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 23 additions & 21 deletions src/devices/Tca955x/Tca955x.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ public abstract class Tca955x : GpioDriver
private readonly int _interrupt;
private readonly Dictionary<int, PinValue> _pinValues = new Dictionary<int, PinValue>();
private readonly ConcurrentDictionary<int, PinChangeEventHandler> _eventHandlers = new ConcurrentDictionary<int, PinChangeEventHandler>();
private readonly Dictionary<int, PinEventTypes> _interruptPins = new Dictionary<int, PinEventTypes>();
private Dictionary<int, PinValue> _interruptLastInputValues = new Dictionary<int, PinValue>();
private readonly Dictionary<int, PinEventTypes> _interruptPinsSubscribedEvents = new Dictionary<int, PinEventTypes>();
private readonly ConcurrentDictionary<int, PinValue> _interruptLastInputValues = new ConcurrentDictionary<int, PinValue>();

private GpioController? _controller;

Expand Down Expand Up @@ -89,7 +89,15 @@ protected Tca955x(I2cDevice device, int interrupt = -1, GpioController? gpioCont
}

if (_interrupt != -1)
{
{
// Initialise the interrupt handling state because ints may start coming from the INT pin
// on the expander as soon as we register the interrupt handler.
for (int i = 0; i < PinCount; i++)
{
_interruptPinsSubscribedEvents.Add(i, PinEventTypes.None);
_interruptLastInputValues.TryAdd(i, PinValue.Low);
}

_shouldDispose = shouldDispose || gpioController is null;
_controller = gpioController ?? new GpioController();
if (!_controller.IsPinOpen(_interrupt))
Expand Down Expand Up @@ -456,7 +464,7 @@ private Task ProcessInterruptInTask()
{
// Take a snapshot of the current interrupt pin configuration and last known input values
// so we can safely process them outside the lock in a background task.
var interruptPinsSnapshot = new Dictionary<int, PinEventTypes>(_interruptPins);
var interruptPinsSnapshot = new Dictionary<int, PinEventTypes>(_interruptPinsSubscribedEvents);
var interruptLastInputValuesSnapshot = new Dictionary<int, PinValue>(_interruptLastInputValues);

Task processingTask = new Task(() =>
Expand Down Expand Up @@ -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]);
Copy link

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
}
}
});
Expand Down Expand Up @@ -557,16 +565,13 @@ protected override void AddCallbackForPinValueChangedEvent(int pinNumber, PinEve

lock (_interruptHandlerLock)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
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);
Copy link

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copilot uses AI. Check for mistakes.
if (!_eventHandlers.TryAdd(pinNumber, callback))
{
_interruptPins.Add(pinNumber, eventType);
_interruptLastInputValues.Add(pinNumber, Read(pinNumber));
_eventHandlers[pinNumber] = callback;
}
throw new InvalidOperationException($"An event handler is already registered for pin {pinNumber}");
}
}
}

Expand All @@ -575,11 +580,8 @@ protected override void RemoveCallbackForPinValueChangedEvent(int pinNumber, Pin
{
lock (_interruptHandlerLock)
{
if (_eventHandlers.TryRemove(pinNumber, out _))
{
_interruptPins.Remove(pinNumber);
_interruptLastInputValues.Remove(pinNumber);
}
_eventHandlers.TryRemove(pinNumber, out _);
_interruptPinsSubscribedEvents[pinNumber] = PinEventTypes.None;
}
}

Expand Down
Loading