diff --git a/src/devices/Tca955x/Tca955x.cs b/src/devices/Tca955x/Tca955x.cs index bad6cb7462..c47c26af81 100644 --- a/src/devices/Tca955x/Tca955x.cs +++ b/src/devices/Tca955x/Tca955x.cs @@ -20,7 +20,7 @@ public abstract class Tca955x : GpioDriver private readonly int _interrupt; private readonly Dictionary _pinValues = new Dictionary(); private readonly ConcurrentDictionary _eventHandlers = new ConcurrentDictionary(); - private readonly Dictionary _interruptPinsSubscribedEvents = new Dictionary(); + private readonly ConcurrentDictionary _interruptPinsSubscribedEvents = new ConcurrentDictionary(); private readonly ConcurrentDictionary _interruptLastInputValues = new ConcurrentDictionary(); private GpioController? _controller; @@ -29,6 +29,10 @@ public abstract class Tca955x : GpioDriver private I2cDevice _busDevice; + // Lock protects: + // 1. I2C bus access - I2C operations must be atomic and sequential + // 2. _pinValues dictionary - tightly coupled with I2C read/write operations + // 3. Interrupt task coordination - _interruptProcessingTask and _interruptPending state private object _interruptHandlerLock = new object(); // This task processes the i2c reading of the io expander in a background task to @@ -89,13 +93,13 @@ 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); + { + // 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.TryAdd(i, PinEventTypes.None); + _interruptLastInputValues.TryAdd(i, PinValue.Low); } _shouldDispose = shouldDispose || gpioController is null; @@ -182,6 +186,7 @@ protected void InternalWriteByte(byte register, byte value) /// The mode to be set. protected override void SetPinMode(int pinNumber, PinMode mode) { + // Lock required: I2C bus operations must be atomic and sequential lock (_interruptHandlerLock) { if (mode != PinMode.Input && mode != PinMode.Output && mode != PinMode.InputPullUp) @@ -250,6 +255,7 @@ protected override void SetPinMode(int pinNumber, PinMode mode) protected override PinValue Read(int pinNumber) { PinValue pinValue; + // Lock required: I2C bus operations must be atomic, and _pinValues is updated during read lock (_interruptHandlerLock) { ValidatePin(pinNumber); @@ -272,6 +278,7 @@ protected override PinValue Read(int pinNumber) /// protected override void Read(Span pinValuePairs) { + // Lock required: I2C bus operations must be atomic, and _pinValues is updated during read lock (_interruptHandlerLock) { byte? lowReg = null; @@ -318,6 +325,7 @@ protected override void Read(Span pinValuePairs) /// The value to be written. protected override void Write(int pinNumber, PinValue value) { + // Lock required: I2C bus operations must be atomic, and _pinValues is updated during write lock (_interruptHandlerLock) { ValidatePin(pinNumber); @@ -338,6 +346,7 @@ protected override void Write(ReadOnlySpan pinValuePairs) bool lowChanged = false; bool highChanged = false; + // Lock required: I2C bus operations must be atomic, and _pinValues is updated during write lock (_interruptHandlerLock) { (uint mask, uint newBits) = new PinVector32(pinValuePairs); @@ -445,8 +454,11 @@ private void InterruptHandler(object sender, PinValueChangedEventArgs e) // i2c and detect a change in the returned input register data, not to mention run the // event handlers that the consumer of the library has signed up, we are likely // to miss edges while we are doing that anyway. Dropping interrupts in this - // case is the best we can do and prevents flooding the consumer with events + // case is the best we can do and prevents flooding the consumer with events // that could queue up in the INT gpio pin driver. + + // Lock required for task coordination: atomically check/start _interruptProcessingTask + // or set _interruptPending flag to ensure proper interrupt queueing lock (_interruptHandlerLock) { if (_interruptProcessingTask == null) @@ -461,10 +473,11 @@ private void InterruptHandler(object sender, PinValueChangedEventArgs e) } 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(_interruptPinsSubscribedEvents); + { + // Take a snapshot of the current interrupt pin configuration and last known input values + // so we can safely process them in a background task. ConcurrentDictionary enumeration + // is thread-safe and provides a consistent snapshot. + var interruptPinsSnapshot = new Dictionary(_interruptPinsSubscribedEvents); var interruptLastInputValuesSnapshot = new Dictionary(_interruptLastInputValues); Task processingTask = new Task(() => @@ -511,14 +524,16 @@ private Task ProcessInterruptInTask() processingTask.ContinueWith(t => { - lock (_interruptHandlerLock) - { - _interruptProcessingTask = null; - if (_interruptPending) - { - _interruptPending = false; - _interruptProcessingTask = ProcessInterruptInTask(); - } + // Lock required for task coordination: atomically check/update _interruptProcessingTask + // and _interruptPending to ensure only one processing task runs at a time + lock (_interruptHandlerLock) + { + _interruptProcessingTask = null; + if (_interruptPending) + { + _interruptPending = false; + _interruptProcessingTask = ProcessInterruptInTask(); + } } }); @@ -563,26 +578,29 @@ protected override void AddCallbackForPinValueChangedEvent(int pinNumber, PinEve throw new InvalidOperationException("No interrupt pin configured"); } + // Update subscription state using thread-safe ConcurrentDictionary operations + _interruptPinsSubscribedEvents[pinNumber] = eventType; + + // Read current value needs lock because it accesses I2C bus + PinValue currentValue; lock (_interruptHandlerLock) { - _interruptPinsSubscribedEvents[pinNumber] = eventType; - var currentValue = Read(pinNumber); - _interruptLastInputValues.TryUpdate(pinNumber, currentValue, !currentValue); - if (!_eventHandlers.TryAdd(pinNumber, callback)) - { - throw new InvalidOperationException($"An event handler is already registered for pin {pinNumber}"); - } + currentValue = Read(pinNumber); + } + + _interruptLastInputValues.TryUpdate(pinNumber, currentValue, !currentValue); + if (!_eventHandlers.TryAdd(pinNumber, callback)) + { + throw new InvalidOperationException($"An event handler is already registered for pin {pinNumber}"); } } /// protected override void RemoveCallbackForPinValueChangedEvent(int pinNumber, PinChangeEventHandler callback) { - lock (_interruptHandlerLock) - { - _eventHandlers.TryRemove(pinNumber, out _); - _interruptPinsSubscribedEvents[pinNumber] = PinEventTypes.None; - } + // Use thread-safe ConcurrentDictionary operations - no lock needed + _eventHandlers.TryRemove(pinNumber, out _); + _interruptPinsSubscribedEvents[pinNumber] = PinEventTypes.None; } ///