From e2535bb8337db68cc356175352cd3ee436633b59 Mon Sep 17 00:00:00 2001 From: Patrick Grawehr Date: Sun, 15 May 2022 09:00:01 +0200 Subject: [PATCH 1/9] Add test that causes an access violation --- .../LibGpiodDriverTests.cs | 107 ++++++++++++++++++ 1 file changed, 107 insertions(+) create mode 100644 src/System.Device.Gpio.Tests/LibGpiodDriverTests.cs diff --git a/src/System.Device.Gpio.Tests/LibGpiodDriverTests.cs b/src/System.Device.Gpio.Tests/LibGpiodDriverTests.cs new file mode 100644 index 0000000000..d317c44f78 --- /dev/null +++ b/src/System.Device.Gpio.Tests/LibGpiodDriverTests.cs @@ -0,0 +1,107 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Device.Gpio.Drivers; +using System.Threading; +using Xunit; +using Xunit.Abstractions; + +namespace System.Device.Gpio.Tests +{ + [Trait("feature", "gpio")] + [Trait("feature", "gpio-libgpiod")] + [Trait("SkipOnTestRun", "Windows_NT")] + public class LibGpiodDriverTests : GpioControllerTestBase + { + public LibGpiodDriverTests(ITestOutputHelper testOutputHelper) + : base(testOutputHelper) + { + } + + protected override GpioDriver GetTestDriver() => new LibGpiodDriver(); + + protected override PinNumberingScheme GetTestNumberingScheme() => PinNumberingScheme.Logical; + + [Fact] + public void SetPinModeSetsDefaultValue() + { + using (GpioController controller = new GpioController(GetTestNumberingScheme(), GetTestDriver())) + { + int testPin = OutputPin; + // Set value to low prior to test, so that we have a defined start situation + controller.OpenPin(testPin, PinMode.Output); + controller.Write(testPin, PinValue.Low); + controller.ClosePin(testPin); + // For this test, we use the input pin as an external pull-up + controller.OpenPin(InputPin, PinMode.Output); + controller.Write(InputPin, PinValue.High); + Thread.Sleep(2); + + controller.OpenPin(testPin, PinMode.Input); + Thread.Sleep(50); + // It's not possible to change the direction while listening to events (causes an error). Therefore the real behavior of the driver + // can only be tested with a scope (or if we had a third pin connected in the lab hardware) + + // We do another test here and make sure the + // pin is really high now + controller.Write(testPin, PinValue.High); + controller.SetPinMode(testPin, PinMode.Output); + controller.SetPinMode(InputPin, PinMode.Input); + + Assert.True(controller.Read(InputPin) == PinValue.High); + + controller.ClosePin(OutputPin); + controller.ClosePin(InputPin); + } + } + + [Fact] + public void UnregisterPinValueChangedShallNotThrow() + { + using var gc = new GpioController(GetTestNumberingScheme(), GetTestDriver()); + gc.OpenPin(InputPin, PinMode.Input); + + static void PinChanged(object sender, PinValueChangedEventArgs args) + { + } + + for (var i = 0; i < 1000; i++) + { + gc.RegisterCallbackForPinValueChangedEvent(InputPin, PinEventTypes.Rising | PinEventTypes.Falling, PinChanged); + gc.UnregisterCallbackForPinValueChangedEvent(InputPin, PinChanged); + } + } + + /// + /// Ensure leaking instances of the driver doesn't cause a segfault + /// See #1849 for a description of this test case + /// + [Fact] + public void LeakingDriverDoesNotCrash() + { + GpioController controller1 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver(4)); + controller1.OpenPin(10, PinMode.Output); + GpioController controller2 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver(4)); + controller2.OpenPin(11, PinMode.Output); + GpioController controller3 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver(4)); + controller3.OpenPin(12, PinMode.Output); + GpioController controller4 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver(4)); + controller4.OpenPin(13, PinMode.Output); + GpioController controller5 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver(4)); + controller5.OpenPin(14, PinMode.Output); + + for (int i = 0; i < 10; i++) + { + GC.Collect(); + GpioController controller6 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver(4)); + controller6.OpenPin(15, PinMode.Output); + controller6.ClosePin(15); + controller6.Dispose(); + GC.Collect(); + Thread.Sleep(20); + } + + GC.WaitForPendingFinalizers(); + } + } +} From be654e667382d02f5da831a4224856f5046f68da Mon Sep 17 00:00:00 2001 From: Patrick Grawehr Date: Sun, 15 May 2022 17:25:48 +0200 Subject: [PATCH 2/9] Do not use SafeHandle for Line The handle is "owned" by the chip and is freed with that one. Freeing them in the wrong order causes an access violation --- .../LibGpiodDriverTests.cs | 14 +- .../Interop/Unix/SafeLineHandle.cs | 64 +++ .../Unix/libgpiod/V1/Interop.libgpiod.cs | 24 +- .../Device/Gpio/Drivers/LibGpiodDriver.cs | 410 ++++++++++++++++++ .../System/Device/Gpio/GpioDriver.cs | 254 +++++------ 5 files changed, 624 insertions(+), 142 deletions(-) create mode 100644 src/System.Device.Gpio/Interop/Unix/SafeLineHandle.cs create mode 100644 src/System.Device.Gpio/System/Device/Gpio/Drivers/LibGpiodDriver.cs diff --git a/src/System.Device.Gpio.Tests/LibGpiodDriverTests.cs b/src/System.Device.Gpio.Tests/LibGpiodDriverTests.cs index d317c44f78..63e7451c64 100644 --- a/src/System.Device.Gpio.Tests/LibGpiodDriverTests.cs +++ b/src/System.Device.Gpio.Tests/LibGpiodDriverTests.cs @@ -74,26 +74,26 @@ static void PinChanged(object sender, PinValueChangedEventArgs args) /// /// Ensure leaking instances of the driver doesn't cause a segfault - /// See #1849 for a description of this test case + /// Regression test for https://github.com/dotnet/iot/issues/1849 /// [Fact] public void LeakingDriverDoesNotCrash() { - GpioController controller1 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver(4)); + GpioController controller1 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); controller1.OpenPin(10, PinMode.Output); - GpioController controller2 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver(4)); + GpioController controller2 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); controller2.OpenPin(11, PinMode.Output); - GpioController controller3 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver(4)); + GpioController controller3 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); controller3.OpenPin(12, PinMode.Output); - GpioController controller4 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver(4)); + GpioController controller4 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); controller4.OpenPin(13, PinMode.Output); - GpioController controller5 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver(4)); + GpioController controller5 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); controller5.OpenPin(14, PinMode.Output); for (int i = 0; i < 10; i++) { GC.Collect(); - GpioController controller6 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver(4)); + GpioController controller6 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); controller6.OpenPin(15, PinMode.Output); controller6.ClosePin(15); controller6.Dispose(); diff --git a/src/System.Device.Gpio/Interop/Unix/SafeLineHandle.cs b/src/System.Device.Gpio/Interop/Unix/SafeLineHandle.cs new file mode 100644 index 0000000000..36bd87ca4f --- /dev/null +++ b/src/System.Device.Gpio/Interop/Unix/SafeLineHandle.cs @@ -0,0 +1,64 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Runtime.InteropServices; + +namespace System.Device.Gpio +{ + /// + /// Pointer to a pin (Not a real SafeLineHandle, because we need to align its finalization with the owning object) + /// + internal sealed class SafeLineHandle : IDisposable + { + private IntPtr _handle; + public SafeLineHandle() + { + _handle = IntPtr.Zero; + } + + public SafeLineHandle(IntPtr handle) + { + _handle = handle; + PinMode = PinMode.Input; + } + + public PinMode PinMode { get; set; } + + public IntPtr Handle + { + get + { + return _handle; + } + set + { + _handle = value; + } + } + + /// + /// Release the lock on the line handle. + /// + public void ReleaseLock() + { + // Contrary to intuition, this does not invalidate the handle (see comment on declaration) + Interop.libgpiod.gpiod_line_release(_handle); + } + + public bool IsInvalid => _handle == IntPtr.Zero || _handle == Interop.libgpiod.InvalidHandleValue; + + public void Dispose() + { + if (_handle != IntPtr.Zero) + { + Interop.libgpiod.gpiod_line_release(_handle); + _handle = IntPtr.Zero; + } + } + + public static implicit operator IntPtr(SafeLineHandle self) + { + return self.Handle; + } + } +} diff --git a/src/System.Device.Gpio/Interop/Unix/libgpiod/V1/Interop.libgpiod.cs b/src/System.Device.Gpio/Interop/Unix/libgpiod/V1/Interop.libgpiod.cs index fa80c71e95..5c40bcca4b 100644 --- a/src/System.Device.Gpio/Interop/Unix/libgpiod/V1/Interop.libgpiod.cs +++ b/src/System.Device.Gpio/Interop/Unix/libgpiod/V1/Interop.libgpiod.cs @@ -81,7 +81,7 @@ static LibgpiodV1() /// The offset of the GPIO line /// Handle to the GPIO line or if an error occurred. [DllImport(LibgpiodLibrary, SetLastError = true)] - internal static extern SafeLineHandle gpiod_chip_get_line(SafeChipHandle chip, int offset); + internal static extern IntPtr gpiod_chip_get_line(SafeChipHandle chip, int offset); /// /// Reserve a single line, set the direction to input. @@ -90,7 +90,7 @@ static LibgpiodV1() /// Name of the consumer. /// 0 if the line was properly reserved, -1 on failure. [DllImport(LibgpiodLibrary, SetLastError = true)] - internal static extern int gpiod_line_request_input(SafeLineHandle line, string consumer); + internal static extern int gpiod_line_request_input(IntPtr line, string consumer); /// /// Reserve a single line, set the direction to input with flags @@ -100,7 +100,7 @@ static LibgpiodV1() /// Additional request flags. /// 0 if the line was properly reserved, -1 on failure. [DllImport(LibgpiodLibrary, SetLastError = true)] - internal static extern int gpiod_line_request_input_flags(SafeLineHandle line, string consumer, int flags); + internal static extern int gpiod_line_request_input_flags(IntPtr line, string consumer, int flags); /// /// Reserve a single line, set the direction to output. @@ -110,7 +110,7 @@ static LibgpiodV1() /// Initial value of the line /// 0 if the line was properly reserved, -1 on failure. [DllImport(LibgpiodLibrary, SetLastError = true)] - internal static extern int gpiod_line_request_output(SafeLineHandle line, string consumer, int default_val); + internal static extern int gpiod_line_request_output(IntPtr line, string consumer, int default_val); /// /// Set the value of a single GPIO line. @@ -119,7 +119,7 @@ static LibgpiodV1() /// New value. /// 0 if the operation succeeds. In case of an error this routine returns -1 and sets the last error number. [DllImport(LibgpiodLibrary)] - internal static extern int gpiod_line_set_value(SafeLineHandle line, int value); + internal static extern int gpiod_line_set_value(IntPtr line, int value); /// /// Read current value of a single GPIO line. @@ -127,7 +127,7 @@ static LibgpiodV1() /// GPIO line handle /// 0 or 1 if the operation succeeds. On error this routine returns -1 and sets the last error number. [DllImport(LibgpiodLibrary, SetLastError = true)] - internal static extern int gpiod_line_get_value(SafeLineHandle line); + internal static extern int gpiod_line_get_value(IntPtr line); /// /// Check if line is no used (not set as Input or Output, not listening events). @@ -135,7 +135,7 @@ static LibgpiodV1() /// GPIO line handle /// false if pin is used as Input/Output or Listening an event, true if it is free [DllImport(LibgpiodLibrary)] - internal static extern bool gpiod_line_is_free(SafeLineHandle line); + internal static extern bool gpiod_line_is_free(IntPtr line); /// /// Release a previously reserved line. @@ -153,7 +153,7 @@ static LibgpiodV1() /// GPIO line handle /// 1 for input, 2 for output [DllImport(LibgpiodLibrary)] - internal static extern int gpiod_line_direction(SafeLineHandle lineHandle); + internal static extern int gpiod_line_direction(IntPtr lineHandle); /// /// Read the GPIO line bias setting. @@ -161,7 +161,7 @@ static LibgpiodV1() /// GPIO line handle /// GPIOD_LINE_BIAS_PULL_UP (3), GPIOD_LINE_BIAS_PULL_DOWN (4), GPIOD_LINE_BIAS_DISABLE (2) or GPIOD_LINE_BIAS_UNKNOWN (1). [DllImport(LibgpiodLibrary)] - internal static extern int gpiod_line_bias(SafeLineHandle lineHandle); + internal static extern int gpiod_line_bias(IntPtr lineHandle); /// /// Request all event type notifications on a single line. @@ -170,7 +170,7 @@ static LibgpiodV1() /// Name of the consumer. /// 0 the operation succeeds, -1 on failure. [DllImport(LibgpiodLibrary, SetLastError = true)] - internal static extern int gpiod_line_request_both_edges_events(SafeLineHandle line, string consumer); + internal static extern int gpiod_line_request_both_edges_events(IntPtr line, string consumer); /// /// Wait for an event on a single line. @@ -179,7 +179,7 @@ static LibgpiodV1() /// The TimeSpec to wait for before timing out /// 0 if wait timed out, -1 if an error occurred, 1 if an event occurred. [DllImport(LibgpiodLibrary, SetLastError = true)] - internal static extern WaitEventResult gpiod_line_event_wait(SafeLineHandle line, ref TimeSpec timeout); + internal static extern WaitEventResult gpiod_line_event_wait(IntPtr line, ref TimeSpec timeout); /// /// Read the last event from the GPIO line. @@ -188,7 +188,7 @@ static LibgpiodV1() /// Reference to the gpio event that was detected /// 1 if rising edge event occurred, 2 on falling edge, -1 on error. [DllImport(LibgpiodLibrary, SetLastError = true)] - internal static extern int gpiod_line_event_read(SafeLineHandle line, ref GpioLineEvent gpioEvent); + internal static extern int gpiod_line_event_read(IntPtr line, ref GpioLineEvent gpioEvent); /// /// Open a gpiochip by number. diff --git a/src/System.Device.Gpio/System/Device/Gpio/Drivers/LibGpiodDriver.cs b/src/System.Device.Gpio/System/Device/Gpio/Drivers/LibGpiodDriver.cs new file mode 100644 index 0000000000..003f21a6d4 --- /dev/null +++ b/src/System.Device.Gpio/System/Device/Gpio/Drivers/LibGpiodDriver.cs @@ -0,0 +1,410 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.Generic; +using System.Threading; +using System.Runtime.InteropServices; +using System.Collections.Concurrent; +using System.Diagnostics; + +namespace System.Device.Gpio.Drivers +{ + /// + /// This driver uses the Libgpiod library to get user-level access to the gpio ports. + /// It superseeds the SysFsDriver, but requires that libgpiod is installed. To do so, run + /// "sudo apt install -y libgpiod-dev". + /// + public class LibGpiodDriver : UnixDriver + { + private static string s_consumerName = Process.GetCurrentProcess().ProcessName; + private readonly object _pinNumberLock; + private readonly ConcurrentDictionary _pinNumberToSafeLineHandle; + private readonly ConcurrentDictionary _pinNumberToEventHandler; + private readonly int _pinCount; + private SafeChipHandle _chip; + + /// + protected internal override int PinCount => _pinCount; + + // for use the bias flags we need libgpiod version 1.5 or later + private static bool IsLibgpiodVersion1_5orHigher() + { + IntPtr libgpiodVersionPtr = Interop.libgpiod.gpiod_version_string(); + string? libgpiodVersionMatch = Marshal.PtrToStringAnsi(libgpiodVersionPtr); + + if (libgpiodVersionMatch is object) + { + Version libgpiodVersion = new Version(libgpiodVersionMatch); + return (libgpiodVersion.Major >= 1 && libgpiodVersion.Minor >= 5); + } + + return false; + } + + private static bool s_isLibgpiodVersion1_5orHigher = IsLibgpiodVersion1_5orHigher(); + + private enum RequestFlag : ulong + { + GPIOD_LINE_REQUEST_FLAG_OPEN_DRAIN = (1UL << 0), + GPIOD_LINE_REQUEST_FLAG_OPEN_SOURCE = (1UL << 1), + GPIOD_LINE_REQUEST_FLAG_ACTIVE_LOW = (1UL << 2), + GPIOD_LINE_REQUEST_FLAG_BIAS_DISABLE = (1UL << 3), + GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN = (1UL << 4), + GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP = (1UL << 5) + } + + /// + /// Construct an instance + /// + /// Number of the gpio Chip. Default 0 + public LibGpiodDriver(int gpioChip = 0) + { + if (Environment.OSVersion.Platform != PlatformID.Unix) + { + throw new PlatformNotSupportedException($"{GetType().Name} is only supported on Linux/Unix."); + } + + try + { + _pinNumberLock = new object(); + _chip = Interop.libgpiod.gpiod_chip_open_by_number(gpioChip); + if (_chip == null) + { + throw ExceptionHelper.GetIOException(ExceptionResource.NoChipFound, Marshal.GetLastWin32Error()); + } + + _pinCount = Interop.libgpiod.gpiod_chip_num_lines(_chip); + _pinNumberToEventHandler = new ConcurrentDictionary(); + _pinNumberToSafeLineHandle = new ConcurrentDictionary(); + } + catch (DllNotFoundException) + { + throw ExceptionHelper.GetPlatformNotSupportedException(ExceptionResource.LibGpiodNotInstalled); + } + } + + /// + protected internal override void AddCallbackForPinValueChangedEvent(int pinNumber, PinEventTypes eventTypes, PinChangeEventHandler callback) + { + if ((eventTypes & PinEventTypes.Rising) != 0 || (eventTypes & PinEventTypes.Falling) != 0) + { + LibGpiodDriverEventHandler eventHandler = _pinNumberToEventHandler.GetOrAdd(pinNumber, PopulateEventHandler); + + if ((eventTypes & PinEventTypes.Rising) != 0) + { + eventHandler.ValueRising += callback; + } + + if ((eventTypes & PinEventTypes.Falling) != 0) + { + eventHandler.ValueFalling += callback; + } + } + else + { + throw ExceptionHelper.GetArgumentException(ExceptionResource.InvalidEventType); + } + } + + private LibGpiodDriverEventHandler PopulateEventHandler(int pinNumber) + { + lock (_pinNumberLock) + { + _pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle); + + if (pinHandle is null || (pinHandle is object && !Interop.libgpiod.gpiod_line_is_free(pinHandle))) + { + pinHandle?.Dispose(); + pinHandle = new SafeLineHandle(Interop.libgpiod.gpiod_chip_get_line(_chip, pinNumber)); + _pinNumberToSafeLineHandle[pinNumber] = pinHandle; + } + + return new LibGpiodDriverEventHandler(pinNumber, pinHandle!); + } + } + + /// + protected internal override void ClosePin(int pinNumber) + { + lock (_pinNumberLock) + { + if (_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle) && + !IsListeningEvent(pinNumber)) + { + pinHandle?.Dispose(); + // We know this works + _pinNumberToSafeLineHandle.TryRemove(pinNumber, out _); + } + } + } + + private bool IsListeningEvent(int pinNumber) + { + return _pinNumberToEventHandler.ContainsKey(pinNumber); + } + + /// + protected internal override int ConvertPinNumberToLogicalNumberingScheme(int pinNumber) => + throw ExceptionHelper.GetPlatformNotSupportedException(ExceptionResource.ConvertPinNumberingSchemaError); + + /// + protected internal override PinMode GetPinMode(int pinNumber) + { + lock (_pinNumberLock) + { + if (!_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle)) + { + throw ExceptionHelper.GetInvalidOperationException(ExceptionResource.PinNotOpenedError, + pin: pinNumber); + } + + return pinHandle.PinMode; + } + } + + /// + protected internal override bool IsPinModeSupported(int pinNumber, PinMode mode) => mode switch + { + PinMode.Input or PinMode.Output => true, + PinMode.InputPullDown or PinMode.InputPullUp => s_isLibgpiodVersion1_5orHigher, + _ => false, + }; + + /// + protected internal override void OpenPin(int pinNumber) + { + lock (_pinNumberLock) + { + if (_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out _)) + { + return; + } + + SafeLineHandle pinHandle = new SafeLineHandle(Interop.libgpiod.gpiod_chip_get_line(_chip, pinNumber)); + if (pinHandle == null) + { + throw ExceptionHelper.GetIOException(ExceptionResource.OpenPinError, Marshal.GetLastWin32Error()); + } + + int mode = Interop.libgpiod.gpiod_line_direction(pinHandle); + if (mode == 1) + { + pinHandle.PinMode = PinMode.Input; + } + else if (mode == 2) + { + pinHandle.PinMode = PinMode.Output; + } + + if (s_isLibgpiodVersion1_5orHigher && pinHandle.PinMode == PinMode.Input) + { + int bias = Interop.libgpiod.gpiod_line_bias(pinHandle); + if (bias == (int)RequestFlag.GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN) + { + pinHandle.PinMode = PinMode.InputPullDown; + } + + if (bias == (int)RequestFlag.GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP) + { + pinHandle.PinMode = PinMode.InputPullUp; + } + } + + _pinNumberToSafeLineHandle.TryAdd(pinNumber, pinHandle); + } + } + + /// + protected internal override PinValue Read(int pinNumber) + { + if (_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle)) + { + int result = Interop.libgpiod.gpiod_line_get_value(pinHandle); + if (result == -1) + { + throw ExceptionHelper.GetIOException(ExceptionResource.ReadPinError, Marshal.GetLastWin32Error(), pinNumber); + } + + return result; + } + + throw ExceptionHelper.GetInvalidOperationException(ExceptionResource.PinNotOpenedError, pin: pinNumber); + } + + /// + protected internal override void RemoveCallbackForPinValueChangedEvent(int pinNumber, PinChangeEventHandler callback) + { + if (_pinNumberToEventHandler.TryGetValue(pinNumber, out LibGpiodDriverEventHandler? eventHandler)) + { + eventHandler.ValueFalling -= callback; + eventHandler.ValueRising -= callback; + if (eventHandler.IsCallbackListEmpty()) + { + _pinNumberToEventHandler.TryRemove(pinNumber, out eventHandler); + eventHandler?.Dispose(); + } + } + else + { + throw ExceptionHelper.GetInvalidOperationException(ExceptionResource.NotListeningForEventError); + } + } + + /// + protected internal override void SetPinMode(int pinNumber, PinMode mode) + { + if (_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle)) + { + // This call does not release the handle. It only releases the lock on the handle. Without this, changing the direction of a line is not possible. + // Line handles cannot be freed and are cached until the chip is closed. + pinHandle.ReleaseLock(); + int requestResult = mode switch + { + PinMode.Input => Interop.libgpiod.gpiod_line_request_input(pinHandle, s_consumerName), + PinMode.InputPullDown => Interop.libgpiod.gpiod_line_request_input_flags(pinHandle, s_consumerName, + (int)RequestFlag.GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN), + PinMode.InputPullUp => Interop.libgpiod.gpiod_line_request_input_flags(pinHandle, s_consumerName, + (int)RequestFlag.GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP), + PinMode.Output => Interop.libgpiod.gpiod_line_request_output(pinHandle, s_consumerName, 0), + _ => -1, + }; + + if (requestResult == -1) + { + throw ExceptionHelper.GetIOException(ExceptionResource.SetPinModeError, Marshal.GetLastWin32Error(), + pinNumber); + } + + pinHandle.PinMode = mode; + return; + } + + throw new InvalidOperationException($"Pin {pinNumber} is not open"); + } + + /// + protected internal override void SetPinMode(int pinNumber, PinMode mode, PinValue initialValue) + { + if (_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle)) + { + // This call does not release the handle. It only releases the lock on the handle. Without this, changing the direction of a line is not possible. + // Line handles cannot be freed and are cached until the chip is closed. + pinHandle.ReleaseLock(); + int requestResult = mode switch + { + PinMode.Input => Interop.libgpiod.gpiod_line_request_input(pinHandle, s_consumerName), + PinMode.InputPullDown => Interop.libgpiod.gpiod_line_request_input_flags(pinHandle, s_consumerName, + (int)RequestFlag.GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN), + PinMode.InputPullUp => Interop.libgpiod.gpiod_line_request_input_flags(pinHandle, s_consumerName, + (int)RequestFlag.GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP), + PinMode.Output => Interop.libgpiod.gpiod_line_request_output(pinHandle, s_consumerName, initialValue == PinValue.High ? 1 : 0), + _ => -1, + }; + + if (requestResult == -1) + { + throw ExceptionHelper.GetIOException(ExceptionResource.SetPinModeError, Marshal.GetLastWin32Error(), + pinNumber); + } + + pinHandle.PinMode = mode; + return; + } + + throw new InvalidOperationException($"Pin {pinNumber} is not open"); + } + + /// + protected internal override WaitForEventResult WaitForEvent(int pinNumber, PinEventTypes eventTypes, CancellationToken cancellationToken) + { + if ((eventTypes & PinEventTypes.Rising) != 0 || (eventTypes & PinEventTypes.Falling) != 0) + { + LibGpiodDriverEventHandler eventHandler = _pinNumberToEventHandler.GetOrAdd(pinNumber, PopulateEventHandler); + + if ((eventTypes & PinEventTypes.Rising) != 0) + { + eventHandler.ValueRising += Callback; + } + + if ((eventTypes & PinEventTypes.Falling) != 0) + { + eventHandler.ValueFalling += Callback; + } + + bool eventOccurred = false; + PinEventTypes typeOfEventOccured = PinEventTypes.None; + void Callback(object o, PinValueChangedEventArgs e) + { + eventOccurred = true; + typeOfEventOccured = e.ChangeType; + } + + WaitForEventResult(cancellationToken, eventHandler.CancellationToken, ref eventOccurred); + RemoveCallbackForPinValueChangedEvent(pinNumber, Callback); + + return new WaitForEventResult + { + TimedOut = !eventOccurred, + EventTypes = eventOccurred ? typeOfEventOccured : PinEventTypes.None, + }; + } + else + { + throw ExceptionHelper.GetArgumentException(ExceptionResource.InvalidEventType); + } + } + + private void WaitForEventResult(CancellationToken sourceToken, CancellationToken parentToken, ref bool eventOccurred) + { + while (!(sourceToken.IsCancellationRequested || parentToken.IsCancellationRequested || eventOccurred)) + { + Thread.Sleep(1); + } + } + + /// + protected internal override void Write(int pinNumber, PinValue value) + { + if (!_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle)) + { + throw ExceptionHelper.GetInvalidOperationException(ExceptionResource.PinNotOpenedError, + pin: pinNumber); + } + + Interop.libgpiod.gpiod_line_set_value(pinHandle, (value == PinValue.High) ? 1 : 0); + } + + /// + protected override void Dispose(bool disposing) + { + if (_pinNumberToEventHandler != null) + { + foreach (KeyValuePair kv in _pinNumberToEventHandler) + { + LibGpiodDriverEventHandler eventHandler = kv.Value; + eventHandler.Dispose(); + } + + _pinNumberToEventHandler.Clear(); + } + + if (_pinNumberToSafeLineHandle != null) + { + foreach (int pin in _pinNumberToSafeLineHandle.Keys) + { + if (_pinNumberToSafeLineHandle.TryGetValue(pin, out SafeLineHandle? pinHandle)) + { + pinHandle?.Dispose(); + } + } + + _pinNumberToSafeLineHandle.Clear(); + } + + _chip?.Dispose(); + _chip = null!; + + base.Dispose(disposing); + } + } +} diff --git a/src/System.Device.Gpio/System/Device/Gpio/GpioDriver.cs b/src/System.Device.Gpio/System/Device/Gpio/GpioDriver.cs index f4abedef9c..5837085270 100644 --- a/src/System.Device.Gpio/System/Device/Gpio/GpioDriver.cs +++ b/src/System.Device.Gpio/System/Device/Gpio/GpioDriver.cs @@ -13,141 +13,149 @@ namespace System.Device.Gpio; public abstract class GpioDriver : IDisposable { /// - /// The number of pins provided by the driver. - /// - protected internal abstract int PinCount { get; } - - /// - /// Converts a board pin number to the driver's logical numbering scheme. - /// - /// The board pin number to convert. - /// The pin number in the driver's logical numbering scheme. - protected internal abstract int ConvertPinNumberToLogicalNumberingScheme(int pinNumber); - - /// - /// Opens a pin in order for it to be ready to use. - /// The driver attempts to open the pin without changing its mode or value. - /// - /// The pin number in the driver's logical numbering scheme. - protected internal abstract void OpenPin(int pinNumber); - - /// - /// Closes an open pin. - /// - /// The pin number in the driver's logical numbering scheme. - protected internal abstract void ClosePin(int pinNumber); - - /// - /// Sets the mode to a pin. - /// - /// The pin number in the driver's logical numbering scheme. - /// The mode to be set. - protected internal abstract void SetPinMode(int pinNumber, PinMode mode); - - /// - /// Sets the mode to a pin and sets an initial value for an output pin. - /// - /// The pin number in the driver's logical numbering scheme. - /// The mode to be set. - /// The initial value if the is output. The driver will do it's best to prevent glitches to the other value when - /// changing from input to output. - protected internal virtual void SetPinMode(int pinNumber, PinMode mode, PinValue initialValue) - { - SetPinMode(pinNumber, mode); - if (mode == PinMode.Output) + /// Finalizer to clean up unmanaged resources + /// + ~GpioDriver() { - Write(pinNumber, initialValue); + Dispose(false); } - } - /// - /// Gets the mode of a pin. - /// - /// The pin number in the driver's logical numbering scheme. - /// The mode of the pin. - protected internal abstract PinMode GetPinMode(int pinNumber); - - /// - /// Checks if a pin supports a specific mode. - /// - /// The pin number in the driver's logical numbering scheme. - /// The mode to check. - /// The status if the pin supports the mode. - protected internal abstract bool IsPinModeSupported(int pinNumber, PinMode mode); - - /// - /// Reads the current value of a pin. - /// - /// The pin number in the driver's logical numbering scheme. - /// The value of the pin. - protected internal abstract PinValue Read(int pinNumber); + /// + /// The number of pins provided by the driver. + /// + protected internal abstract int PinCount { get; } + + /// + /// Converts a board pin number to the driver's logical numbering scheme. + /// + /// The board pin number to convert. + /// The pin number in the driver's logical numbering scheme. + protected internal abstract int ConvertPinNumberToLogicalNumberingScheme(int pinNumber); + + /// + /// Opens a pin in order for it to be ready to use. + /// The driver attempts to open the pin without changing its mode or value. + /// + /// The pin number in the driver's logical numbering scheme. + protected internal abstract void OpenPin(int pinNumber); + + /// + /// Closes an open pin. + /// + /// The pin number in the driver's logical numbering scheme. + protected internal abstract void ClosePin(int pinNumber); + + /// + /// Sets the mode to a pin. + /// + /// The pin number in the driver's logical numbering scheme. + /// The mode to be set. + protected internal abstract void SetPinMode(int pinNumber, PinMode mode); + + /// + /// Sets the mode to a pin and sets an initial value for an output pin. + /// + /// The pin number in the driver's logical numbering scheme. + /// The mode to be set. + /// The initial value if the is output. The driver will do it's best to prevent glitches to the other value when + /// changing from input to output. + protected internal virtual void SetPinMode(int pinNumber, PinMode mode, PinValue initialValue) + { + SetPinMode(pinNumber, mode); + if (mode == PinMode.Output) + { + Write(pinNumber, initialValue); + } + } - /// + /// + /// Gets the mode of a pin. + /// + /// The pin number in the driver's logical numbering scheme. + /// The mode of the pin. + protected internal abstract PinMode GetPinMode(int pinNumber); + + /// + /// Checks if a pin supports a specific mode. + /// + /// The pin number in the driver's logical numbering scheme. + /// The mode to check. + /// The status if the pin supports the mode. + protected internal abstract bool IsPinModeSupported(int pinNumber, PinMode mode); + + /// + /// Reads the current value of a pin. + /// + /// The pin number in the driver's logical numbering scheme. + /// The value of the pin. + protected internal abstract PinValue Read(int pinNumber); + + /// /// Toggle the current value of a pin. /// /// The pin number in the driver's logical numbering scheme. protected internal virtual void Toggle(int pinNumber) => Write(pinNumber, !Read(pinNumber)); /// - /// Writes a value to a pin. - /// - /// The pin number in the driver's logical numbering scheme. - /// The value to be written to the pin. - protected internal abstract void Write(int pinNumber, PinValue value); - - /// - /// Blocks execution until an event of type eventType is received or a cancellation is requested. - /// - /// The pin number in the driver's logical numbering scheme. - /// The event types to wait for. - /// The cancellation token of when the operation should stop waiting for an event. - /// A structure that contains the result of the waiting operation. - protected internal abstract WaitForEventResult WaitForEvent(int pinNumber, PinEventTypes eventTypes, CancellationToken cancellationToken); - - /// - /// Async call until an event of type eventType is received or a cancellation is requested. - /// - /// The pin number in the driver's logical numbering scheme. - /// The event types to wait for. - /// The cancellation token of when the operation should stop waiting for an event. - /// A task representing the operation of getting the structure that contains the result of the waiting operation - protected internal virtual ValueTask WaitForEventAsync(int pinNumber, PinEventTypes eventTypes, CancellationToken cancellationToken) - { - return new ValueTask(Task.Run(() => WaitForEvent(pinNumber, eventTypes, cancellationToken))); - } - - /// - /// Adds a handler for a pin value changed event. - /// - /// The pin number in the driver's logical numbering scheme. - /// The event types to wait for. - /// Delegate that defines the structure for callbacks when a pin value changed event occurs. - protected internal abstract void AddCallbackForPinValueChangedEvent(int pinNumber, PinEventTypes eventTypes, PinChangeEventHandler callback); - - /// - /// Removes a handler for a pin value changed event. - /// - /// The pin number in the driver's logical numbering scheme. - /// Delegate that defines the structure for callbacks when a pin value changed event occurs. - protected internal abstract void RemoveCallbackForPinValueChangedEvent(int pinNumber, PinChangeEventHandler callback); + /// Writes a value to a pin. + /// + /// The pin number in the driver's logical numbering scheme. + /// The value to be written to the pin. + protected internal abstract void Write(int pinNumber, PinValue value); + + /// + /// Blocks execution until an event of type eventType is received or a cancellation is requested. + /// + /// The pin number in the driver's logical numbering scheme. + /// The event types to wait for. + /// The cancellation token of when the operation should stop waiting for an event. + /// A structure that contains the result of the waiting operation. + protected internal abstract WaitForEventResult WaitForEvent(int pinNumber, PinEventTypes eventTypes, CancellationToken cancellationToken); + + /// + /// Async call until an event of type eventType is received or a cancellation is requested. + /// + /// The pin number in the driver's logical numbering scheme. + /// The event types to wait for. + /// The cancellation token of when the operation should stop waiting for an event. + /// A task representing the operation of getting the structure that contains the result of the waiting operation + protected internal virtual ValueTask WaitForEventAsync(int pinNumber, PinEventTypes eventTypes, CancellationToken cancellationToken) + { + return new ValueTask(Task.Run(() => WaitForEvent(pinNumber, eventTypes, cancellationToken))); + } - /// - /// Disposes this instance, closing all open pins - /// - public void Dispose() - { - Dispose(true); - GC.SuppressFinalize(this); - } + /// + /// Adds a handler for a pin value changed event. + /// + /// The pin number in the driver's logical numbering scheme. + /// The event types to wait for. + /// Delegate that defines the structure for callbacks when a pin value changed event occurs. + protected internal abstract void AddCallbackForPinValueChangedEvent(int pinNumber, PinEventTypes eventTypes, PinChangeEventHandler callback); + + /// + /// Removes a handler for a pin value changed event. + /// + /// The pin number in the driver's logical numbering scheme. + /// Delegate that defines the structure for callbacks when a pin value changed event occurs. + protected internal abstract void RemoveCallbackForPinValueChangedEvent(int pinNumber, PinChangeEventHandler callback); + + /// + /// Disposes this instance, closing all open pins + /// + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } - /// - /// Disposes this instance - /// - /// True if explicitly disposing, false if in finalizer - protected virtual void Dispose(bool disposing) - { - // Nothing to do in base class. - } + /// + /// Disposes this instance + /// + /// True if explicitly disposing, false if in finalizer + protected virtual void Dispose(bool disposing) + { + // Nothing to do in base class. + } /// /// Query information about a component and its children. From 1f4b066d6953ce402b2e19e96fbb701baecbc6b7 Mon Sep 17 00:00:00 2001 From: Patrick Grawehr Date: Thu, 14 Jul 2022 20:02:50 +0200 Subject: [PATCH 3/9] Fix a merge problem --- .../LibGpiodDriverTests.cs | 58 ++++++------- .../Interop/Unix/SafeLineHandle.cs | 85 +++++++++---------- 2 files changed, 71 insertions(+), 72 deletions(-) diff --git a/src/System.Device.Gpio.Tests/LibGpiodDriverTests.cs b/src/System.Device.Gpio.Tests/LibGpiodDriverTests.cs index 63e7451c64..f4c4eafe89 100644 --- a/src/System.Device.Gpio.Tests/LibGpiodDriverTests.cs +++ b/src/System.Device.Gpio.Tests/LibGpiodDriverTests.cs @@ -71,37 +71,37 @@ static void PinChanged(object sender, PinValueChangedEventArgs args) gc.UnregisterCallbackForPinValueChangedEvent(InputPin, PinChanged); } } + } - /// - /// Ensure leaking instances of the driver doesn't cause a segfault - /// Regression test for https://github.com/dotnet/iot/issues/1849 - /// - [Fact] - public void LeakingDriverDoesNotCrash() - { - GpioController controller1 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); - controller1.OpenPin(10, PinMode.Output); - GpioController controller2 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); - controller2.OpenPin(11, PinMode.Output); - GpioController controller3 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); - controller3.OpenPin(12, PinMode.Output); - GpioController controller4 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); - controller4.OpenPin(13, PinMode.Output); - GpioController controller5 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); - controller5.OpenPin(14, PinMode.Output); - - for (int i = 0; i < 10; i++) - { - GC.Collect(); - GpioController controller6 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); - controller6.OpenPin(15, PinMode.Output); - controller6.ClosePin(15); - controller6.Dispose(); - GC.Collect(); - Thread.Sleep(20); - } + /// + /// Ensure leaking instances of the driver doesn't cause a segfault + /// Regression test for https://github.com/dotnet/iot/issues/1849 + /// + [Fact] + public void LeakingDriverDoesNotCrash() + { + GpioController controller1 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); + controller1.OpenPin(10, PinMode.Output); + GpioController controller2 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); + controller2.OpenPin(11, PinMode.Output); + GpioController controller3 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); + controller3.OpenPin(12, PinMode.Output); + GpioController controller4 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); + controller4.OpenPin(13, PinMode.Output); + GpioController controller5 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); + controller5.OpenPin(14, PinMode.Output); - GC.WaitForPendingFinalizers(); + for (int i = 0; i < 10; i++) + { + GC.Collect(); + GpioController controller6 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); + controller6.OpenPin(15, PinMode.Output); + controller6.ClosePin(15); + controller6.Dispose(); + GC.Collect(); + Thread.Sleep(20); } + + GC.WaitForPendingFinalizers(); } } diff --git a/src/System.Device.Gpio/Interop/Unix/SafeLineHandle.cs b/src/System.Device.Gpio/Interop/Unix/SafeLineHandle.cs index 36bd87ca4f..ef5333338d 100644 --- a/src/System.Device.Gpio/Interop/Unix/SafeLineHandle.cs +++ b/src/System.Device.Gpio/Interop/Unix/SafeLineHandle.cs @@ -3,62 +3,61 @@ using System.Runtime.InteropServices; -namespace System.Device.Gpio +namespace System.Device.Gpio; + +/// +/// Pointer to a pin (Not a real SafeLineHandle, because we need to align its finalization with the owning object) +/// +internal sealed class SafeLineHandle : IDisposable { - /// - /// Pointer to a pin (Not a real SafeLineHandle, because we need to align its finalization with the owning object) - /// - internal sealed class SafeLineHandle : IDisposable + private IntPtr _handle; + public SafeLineHandle() { - private IntPtr _handle; - public SafeLineHandle() - { - _handle = IntPtr.Zero; - } + _handle = IntPtr.Zero; + } - public SafeLineHandle(IntPtr handle) - { - _handle = handle; - PinMode = PinMode.Input; - } + public SafeLineHandle(IntPtr handle) + { + _handle = handle; + PinMode = PinMode.Input; + } - public PinMode PinMode { get; set; } + public PinMode PinMode { get; set; } - public IntPtr Handle + public IntPtr Handle + { + get { - get - { - return _handle; - } - set - { - _handle = value; - } + return _handle; } - - /// - /// Release the lock on the line handle. - /// - public void ReleaseLock() + set { - // Contrary to intuition, this does not invalidate the handle (see comment on declaration) - Interop.libgpiod.gpiod_line_release(_handle); + _handle = value; } + } - public bool IsInvalid => _handle == IntPtr.Zero || _handle == Interop.libgpiod.InvalidHandleValue; + /// + /// Release the lock on the line handle. + /// + public void ReleaseLock() + { + // Contrary to intuition, this does not invalidate the handle (see comment on declaration) + Interop.libgpiod.gpiod_line_release(_handle); + } - public void Dispose() - { - if (_handle != IntPtr.Zero) - { - Interop.libgpiod.gpiod_line_release(_handle); - _handle = IntPtr.Zero; - } - } + public bool IsInvalid => _handle == IntPtr.Zero || _handle == Interop.libgpiod.InvalidHandleValue; - public static implicit operator IntPtr(SafeLineHandle self) + public void Dispose() + { + if (_handle != IntPtr.Zero) { - return self.Handle; + Interop.libgpiod.gpiod_line_release(_handle); + _handle = IntPtr.Zero; } } + + public static implicit operator IntPtr(SafeLineHandle self) + { + return self.Handle; + } } From cd8331fa4697cc110446a70b8a9514960e7d83c4 Mon Sep 17 00:00:00 2001 From: Patrick Grawehr Date: Sat, 27 Apr 2024 08:35:24 +0200 Subject: [PATCH 4/9] Adjust to changed directory structure --- .../LibGpiodDriverTests.cs | 107 ----- .../LibGpiodV1DriverTests.cs | 34 +- .../Interop/Unix/SafeLineHandle.cs | 63 --- .../Unix/libgpiod/V1/SafeLineHandle.cs | 46 +- .../Device/Gpio/Drivers/LibGpiodDriver.cs | 410 ------------------ 5 files changed, 67 insertions(+), 593 deletions(-) delete mode 100644 src/System.Device.Gpio.Tests/LibGpiodDriverTests.cs delete mode 100644 src/System.Device.Gpio/Interop/Unix/SafeLineHandle.cs delete mode 100644 src/System.Device.Gpio/System/Device/Gpio/Drivers/LibGpiodDriver.cs diff --git a/src/System.Device.Gpio.Tests/LibGpiodDriverTests.cs b/src/System.Device.Gpio.Tests/LibGpiodDriverTests.cs deleted file mode 100644 index f4c4eafe89..0000000000 --- a/src/System.Device.Gpio.Tests/LibGpiodDriverTests.cs +++ /dev/null @@ -1,107 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System.Device.Gpio.Drivers; -using System.Threading; -using Xunit; -using Xunit.Abstractions; - -namespace System.Device.Gpio.Tests -{ - [Trait("feature", "gpio")] - [Trait("feature", "gpio-libgpiod")] - [Trait("SkipOnTestRun", "Windows_NT")] - public class LibGpiodDriverTests : GpioControllerTestBase - { - public LibGpiodDriverTests(ITestOutputHelper testOutputHelper) - : base(testOutputHelper) - { - } - - protected override GpioDriver GetTestDriver() => new LibGpiodDriver(); - - protected override PinNumberingScheme GetTestNumberingScheme() => PinNumberingScheme.Logical; - - [Fact] - public void SetPinModeSetsDefaultValue() - { - using (GpioController controller = new GpioController(GetTestNumberingScheme(), GetTestDriver())) - { - int testPin = OutputPin; - // Set value to low prior to test, so that we have a defined start situation - controller.OpenPin(testPin, PinMode.Output); - controller.Write(testPin, PinValue.Low); - controller.ClosePin(testPin); - // For this test, we use the input pin as an external pull-up - controller.OpenPin(InputPin, PinMode.Output); - controller.Write(InputPin, PinValue.High); - Thread.Sleep(2); - - controller.OpenPin(testPin, PinMode.Input); - Thread.Sleep(50); - // It's not possible to change the direction while listening to events (causes an error). Therefore the real behavior of the driver - // can only be tested with a scope (or if we had a third pin connected in the lab hardware) - - // We do another test here and make sure the - // pin is really high now - controller.Write(testPin, PinValue.High); - controller.SetPinMode(testPin, PinMode.Output); - controller.SetPinMode(InputPin, PinMode.Input); - - Assert.True(controller.Read(InputPin) == PinValue.High); - - controller.ClosePin(OutputPin); - controller.ClosePin(InputPin); - } - } - - [Fact] - public void UnregisterPinValueChangedShallNotThrow() - { - using var gc = new GpioController(GetTestNumberingScheme(), GetTestDriver()); - gc.OpenPin(InputPin, PinMode.Input); - - static void PinChanged(object sender, PinValueChangedEventArgs args) - { - } - - for (var i = 0; i < 1000; i++) - { - gc.RegisterCallbackForPinValueChangedEvent(InputPin, PinEventTypes.Rising | PinEventTypes.Falling, PinChanged); - gc.UnregisterCallbackForPinValueChangedEvent(InputPin, PinChanged); - } - } - } - - /// - /// Ensure leaking instances of the driver doesn't cause a segfault - /// Regression test for https://github.com/dotnet/iot/issues/1849 - /// - [Fact] - public void LeakingDriverDoesNotCrash() - { - GpioController controller1 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); - controller1.OpenPin(10, PinMode.Output); - GpioController controller2 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); - controller2.OpenPin(11, PinMode.Output); - GpioController controller3 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); - controller3.OpenPin(12, PinMode.Output); - GpioController controller4 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); - controller4.OpenPin(13, PinMode.Output); - GpioController controller5 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); - controller5.OpenPin(14, PinMode.Output); - - for (int i = 0; i < 10; i++) - { - GC.Collect(); - GpioController controller6 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); - controller6.OpenPin(15, PinMode.Output); - controller6.ClosePin(15); - controller6.Dispose(); - GC.Collect(); - Thread.Sleep(20); - } - - GC.WaitForPendingFinalizers(); - } -} diff --git a/src/System.Device.Gpio.Tests/LibGpiodV1DriverTests.cs b/src/System.Device.Gpio.Tests/LibGpiodV1DriverTests.cs index 0cd8f4332f..a0d69c0076 100644 --- a/src/System.Device.Gpio.Tests/LibGpiodV1DriverTests.cs +++ b/src/System.Device.Gpio.Tests/LibGpiodV1DriverTests.cs @@ -72,4 +72,36 @@ static void PinChanged(object sender, PinValueChangedEventArgs args) gc.UnregisterCallbackForPinValueChangedEvent(InputPin, PinChanged); } } -} + + /// + /// Ensure leaking instances of the driver doesn't cause a segfault + /// Regression test for https://github.com/dotnet/iot/issues/1849 + /// + [Fact] + public void LeakingDriverDoesNotCrash() + { + GpioController controller1 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); + controller1.OpenPin(10, PinMode.Output); + GpioController controller2 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); + controller2.OpenPin(11, PinMode.Output); + GpioController controller3 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); + controller3.OpenPin(12, PinMode.Output); + GpioController controller4 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); + controller4.OpenPin(13, PinMode.Output); + GpioController controller5 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); + controller5.OpenPin(14, PinMode.Output); + + for (int i = 0; i < 10; i++) + { + GC.Collect(); + GpioController controller6 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver()); + controller6.OpenPin(15, PinMode.Output); + controller6.ClosePin(15); + controller6.Dispose(); + GC.Collect(); + Thread.Sleep(20); + } + + GC.WaitForPendingFinalizers(); + } +} \ No newline at end of file diff --git a/src/System.Device.Gpio/Interop/Unix/SafeLineHandle.cs b/src/System.Device.Gpio/Interop/Unix/SafeLineHandle.cs deleted file mode 100644 index ef5333338d..0000000000 --- a/src/System.Device.Gpio/Interop/Unix/SafeLineHandle.cs +++ /dev/null @@ -1,63 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System.Runtime.InteropServices; - -namespace System.Device.Gpio; - -/// -/// Pointer to a pin (Not a real SafeLineHandle, because we need to align its finalization with the owning object) -/// -internal sealed class SafeLineHandle : IDisposable -{ - private IntPtr _handle; - public SafeLineHandle() - { - _handle = IntPtr.Zero; - } - - public SafeLineHandle(IntPtr handle) - { - _handle = handle; - PinMode = PinMode.Input; - } - - public PinMode PinMode { get; set; } - - public IntPtr Handle - { - get - { - return _handle; - } - set - { - _handle = value; - } - } - - /// - /// Release the lock on the line handle. - /// - public void ReleaseLock() - { - // Contrary to intuition, this does not invalidate the handle (see comment on declaration) - Interop.libgpiod.gpiod_line_release(_handle); - } - - public bool IsInvalid => _handle == IntPtr.Zero || _handle == Interop.libgpiod.InvalidHandleValue; - - public void Dispose() - { - if (_handle != IntPtr.Zero) - { - Interop.libgpiod.gpiod_line_release(_handle); - _handle = IntPtr.Zero; - } - } - - public static implicit operator IntPtr(SafeLineHandle self) - { - return self.Handle; - } -} diff --git a/src/System.Device.Gpio/Interop/Unix/libgpiod/V1/SafeLineHandle.cs b/src/System.Device.Gpio/Interop/Unix/libgpiod/V1/SafeLineHandle.cs index be7172fa31..5e71c2eeec 100644 --- a/src/System.Device.Gpio/Interop/Unix/libgpiod/V1/SafeLineHandle.cs +++ b/src/System.Device.Gpio/Interop/Unix/libgpiod/V1/SafeLineHandle.cs @@ -7,31 +7,53 @@ namespace System.Device.Gpio.Libgpiod.V1; /// -/// Pointer to a pin. +/// Pointer to a pin (Not a real SafeLineHandle, because we need to align its finalization with the owning object) /// -internal class SafeLineHandle : SafeHandle +internal sealed class SafeLineHandle : IDisposable { - public PinMode PinMode { get; set; } - + private IntPtr _handle; public SafeLineHandle() - : base(IntPtr.Zero, true) { + _handle = IntPtr.Zero; } - protected override bool ReleaseHandle() + public SafeLineHandle(IntPtr handle) { - // Contrary to intuition, this does not invalidate the handle (see comment on declaration) - LibgpiodV1.gpiod_line_release(handle); - return true; + _handle = handle; + PinMode = PinMode.Input; + } + + public PinMode PinMode { get; set; } + + public IntPtr Handle + { + get + { + return _handle; + } + set + { + _handle = value; + } } /// - /// Release the lock on the line handle. + /// Release the lock on the line handle. /// public void ReleaseLock() { - ReleaseHandle(); + // Contrary to intuition, this does not invalidate the handle (see comment on declaration) + Interop.libgpiod.gpiod_line_release(_handle); } - public override bool IsInvalid => handle == IntPtr.Zero || handle == LibgpiodV1.InvalidHandleValue; + public bool IsInvalid => _handle == IntPtr.Zero || _handle == Interop.libgpiod.InvalidHandleValue; + + public void Dispose() + { + if (_handle != IntPtr.Zero) + { + Interop.libgpiod.gpiod_line_release(_handle); + _handle = IntPtr.Zero; + } + } } diff --git a/src/System.Device.Gpio/System/Device/Gpio/Drivers/LibGpiodDriver.cs b/src/System.Device.Gpio/System/Device/Gpio/Drivers/LibGpiodDriver.cs deleted file mode 100644 index 003f21a6d4..0000000000 --- a/src/System.Device.Gpio/System/Device/Gpio/Drivers/LibGpiodDriver.cs +++ /dev/null @@ -1,410 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System.Collections.Generic; -using System.Threading; -using System.Runtime.InteropServices; -using System.Collections.Concurrent; -using System.Diagnostics; - -namespace System.Device.Gpio.Drivers -{ - /// - /// This driver uses the Libgpiod library to get user-level access to the gpio ports. - /// It superseeds the SysFsDriver, but requires that libgpiod is installed. To do so, run - /// "sudo apt install -y libgpiod-dev". - /// - public class LibGpiodDriver : UnixDriver - { - private static string s_consumerName = Process.GetCurrentProcess().ProcessName; - private readonly object _pinNumberLock; - private readonly ConcurrentDictionary _pinNumberToSafeLineHandle; - private readonly ConcurrentDictionary _pinNumberToEventHandler; - private readonly int _pinCount; - private SafeChipHandle _chip; - - /// - protected internal override int PinCount => _pinCount; - - // for use the bias flags we need libgpiod version 1.5 or later - private static bool IsLibgpiodVersion1_5orHigher() - { - IntPtr libgpiodVersionPtr = Interop.libgpiod.gpiod_version_string(); - string? libgpiodVersionMatch = Marshal.PtrToStringAnsi(libgpiodVersionPtr); - - if (libgpiodVersionMatch is object) - { - Version libgpiodVersion = new Version(libgpiodVersionMatch); - return (libgpiodVersion.Major >= 1 && libgpiodVersion.Minor >= 5); - } - - return false; - } - - private static bool s_isLibgpiodVersion1_5orHigher = IsLibgpiodVersion1_5orHigher(); - - private enum RequestFlag : ulong - { - GPIOD_LINE_REQUEST_FLAG_OPEN_DRAIN = (1UL << 0), - GPIOD_LINE_REQUEST_FLAG_OPEN_SOURCE = (1UL << 1), - GPIOD_LINE_REQUEST_FLAG_ACTIVE_LOW = (1UL << 2), - GPIOD_LINE_REQUEST_FLAG_BIAS_DISABLE = (1UL << 3), - GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN = (1UL << 4), - GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP = (1UL << 5) - } - - /// - /// Construct an instance - /// - /// Number of the gpio Chip. Default 0 - public LibGpiodDriver(int gpioChip = 0) - { - if (Environment.OSVersion.Platform != PlatformID.Unix) - { - throw new PlatformNotSupportedException($"{GetType().Name} is only supported on Linux/Unix."); - } - - try - { - _pinNumberLock = new object(); - _chip = Interop.libgpiod.gpiod_chip_open_by_number(gpioChip); - if (_chip == null) - { - throw ExceptionHelper.GetIOException(ExceptionResource.NoChipFound, Marshal.GetLastWin32Error()); - } - - _pinCount = Interop.libgpiod.gpiod_chip_num_lines(_chip); - _pinNumberToEventHandler = new ConcurrentDictionary(); - _pinNumberToSafeLineHandle = new ConcurrentDictionary(); - } - catch (DllNotFoundException) - { - throw ExceptionHelper.GetPlatformNotSupportedException(ExceptionResource.LibGpiodNotInstalled); - } - } - - /// - protected internal override void AddCallbackForPinValueChangedEvent(int pinNumber, PinEventTypes eventTypes, PinChangeEventHandler callback) - { - if ((eventTypes & PinEventTypes.Rising) != 0 || (eventTypes & PinEventTypes.Falling) != 0) - { - LibGpiodDriverEventHandler eventHandler = _pinNumberToEventHandler.GetOrAdd(pinNumber, PopulateEventHandler); - - if ((eventTypes & PinEventTypes.Rising) != 0) - { - eventHandler.ValueRising += callback; - } - - if ((eventTypes & PinEventTypes.Falling) != 0) - { - eventHandler.ValueFalling += callback; - } - } - else - { - throw ExceptionHelper.GetArgumentException(ExceptionResource.InvalidEventType); - } - } - - private LibGpiodDriverEventHandler PopulateEventHandler(int pinNumber) - { - lock (_pinNumberLock) - { - _pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle); - - if (pinHandle is null || (pinHandle is object && !Interop.libgpiod.gpiod_line_is_free(pinHandle))) - { - pinHandle?.Dispose(); - pinHandle = new SafeLineHandle(Interop.libgpiod.gpiod_chip_get_line(_chip, pinNumber)); - _pinNumberToSafeLineHandle[pinNumber] = pinHandle; - } - - return new LibGpiodDriverEventHandler(pinNumber, pinHandle!); - } - } - - /// - protected internal override void ClosePin(int pinNumber) - { - lock (_pinNumberLock) - { - if (_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle) && - !IsListeningEvent(pinNumber)) - { - pinHandle?.Dispose(); - // We know this works - _pinNumberToSafeLineHandle.TryRemove(pinNumber, out _); - } - } - } - - private bool IsListeningEvent(int pinNumber) - { - return _pinNumberToEventHandler.ContainsKey(pinNumber); - } - - /// - protected internal override int ConvertPinNumberToLogicalNumberingScheme(int pinNumber) => - throw ExceptionHelper.GetPlatformNotSupportedException(ExceptionResource.ConvertPinNumberingSchemaError); - - /// - protected internal override PinMode GetPinMode(int pinNumber) - { - lock (_pinNumberLock) - { - if (!_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle)) - { - throw ExceptionHelper.GetInvalidOperationException(ExceptionResource.PinNotOpenedError, - pin: pinNumber); - } - - return pinHandle.PinMode; - } - } - - /// - protected internal override bool IsPinModeSupported(int pinNumber, PinMode mode) => mode switch - { - PinMode.Input or PinMode.Output => true, - PinMode.InputPullDown or PinMode.InputPullUp => s_isLibgpiodVersion1_5orHigher, - _ => false, - }; - - /// - protected internal override void OpenPin(int pinNumber) - { - lock (_pinNumberLock) - { - if (_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out _)) - { - return; - } - - SafeLineHandle pinHandle = new SafeLineHandle(Interop.libgpiod.gpiod_chip_get_line(_chip, pinNumber)); - if (pinHandle == null) - { - throw ExceptionHelper.GetIOException(ExceptionResource.OpenPinError, Marshal.GetLastWin32Error()); - } - - int mode = Interop.libgpiod.gpiod_line_direction(pinHandle); - if (mode == 1) - { - pinHandle.PinMode = PinMode.Input; - } - else if (mode == 2) - { - pinHandle.PinMode = PinMode.Output; - } - - if (s_isLibgpiodVersion1_5orHigher && pinHandle.PinMode == PinMode.Input) - { - int bias = Interop.libgpiod.gpiod_line_bias(pinHandle); - if (bias == (int)RequestFlag.GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN) - { - pinHandle.PinMode = PinMode.InputPullDown; - } - - if (bias == (int)RequestFlag.GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP) - { - pinHandle.PinMode = PinMode.InputPullUp; - } - } - - _pinNumberToSafeLineHandle.TryAdd(pinNumber, pinHandle); - } - } - - /// - protected internal override PinValue Read(int pinNumber) - { - if (_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle)) - { - int result = Interop.libgpiod.gpiod_line_get_value(pinHandle); - if (result == -1) - { - throw ExceptionHelper.GetIOException(ExceptionResource.ReadPinError, Marshal.GetLastWin32Error(), pinNumber); - } - - return result; - } - - throw ExceptionHelper.GetInvalidOperationException(ExceptionResource.PinNotOpenedError, pin: pinNumber); - } - - /// - protected internal override void RemoveCallbackForPinValueChangedEvent(int pinNumber, PinChangeEventHandler callback) - { - if (_pinNumberToEventHandler.TryGetValue(pinNumber, out LibGpiodDriverEventHandler? eventHandler)) - { - eventHandler.ValueFalling -= callback; - eventHandler.ValueRising -= callback; - if (eventHandler.IsCallbackListEmpty()) - { - _pinNumberToEventHandler.TryRemove(pinNumber, out eventHandler); - eventHandler?.Dispose(); - } - } - else - { - throw ExceptionHelper.GetInvalidOperationException(ExceptionResource.NotListeningForEventError); - } - } - - /// - protected internal override void SetPinMode(int pinNumber, PinMode mode) - { - if (_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle)) - { - // This call does not release the handle. It only releases the lock on the handle. Without this, changing the direction of a line is not possible. - // Line handles cannot be freed and are cached until the chip is closed. - pinHandle.ReleaseLock(); - int requestResult = mode switch - { - PinMode.Input => Interop.libgpiod.gpiod_line_request_input(pinHandle, s_consumerName), - PinMode.InputPullDown => Interop.libgpiod.gpiod_line_request_input_flags(pinHandle, s_consumerName, - (int)RequestFlag.GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN), - PinMode.InputPullUp => Interop.libgpiod.gpiod_line_request_input_flags(pinHandle, s_consumerName, - (int)RequestFlag.GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP), - PinMode.Output => Interop.libgpiod.gpiod_line_request_output(pinHandle, s_consumerName, 0), - _ => -1, - }; - - if (requestResult == -1) - { - throw ExceptionHelper.GetIOException(ExceptionResource.SetPinModeError, Marshal.GetLastWin32Error(), - pinNumber); - } - - pinHandle.PinMode = mode; - return; - } - - throw new InvalidOperationException($"Pin {pinNumber} is not open"); - } - - /// - protected internal override void SetPinMode(int pinNumber, PinMode mode, PinValue initialValue) - { - if (_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle)) - { - // This call does not release the handle. It only releases the lock on the handle. Without this, changing the direction of a line is not possible. - // Line handles cannot be freed and are cached until the chip is closed. - pinHandle.ReleaseLock(); - int requestResult = mode switch - { - PinMode.Input => Interop.libgpiod.gpiod_line_request_input(pinHandle, s_consumerName), - PinMode.InputPullDown => Interop.libgpiod.gpiod_line_request_input_flags(pinHandle, s_consumerName, - (int)RequestFlag.GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN), - PinMode.InputPullUp => Interop.libgpiod.gpiod_line_request_input_flags(pinHandle, s_consumerName, - (int)RequestFlag.GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP), - PinMode.Output => Interop.libgpiod.gpiod_line_request_output(pinHandle, s_consumerName, initialValue == PinValue.High ? 1 : 0), - _ => -1, - }; - - if (requestResult == -1) - { - throw ExceptionHelper.GetIOException(ExceptionResource.SetPinModeError, Marshal.GetLastWin32Error(), - pinNumber); - } - - pinHandle.PinMode = mode; - return; - } - - throw new InvalidOperationException($"Pin {pinNumber} is not open"); - } - - /// - protected internal override WaitForEventResult WaitForEvent(int pinNumber, PinEventTypes eventTypes, CancellationToken cancellationToken) - { - if ((eventTypes & PinEventTypes.Rising) != 0 || (eventTypes & PinEventTypes.Falling) != 0) - { - LibGpiodDriverEventHandler eventHandler = _pinNumberToEventHandler.GetOrAdd(pinNumber, PopulateEventHandler); - - if ((eventTypes & PinEventTypes.Rising) != 0) - { - eventHandler.ValueRising += Callback; - } - - if ((eventTypes & PinEventTypes.Falling) != 0) - { - eventHandler.ValueFalling += Callback; - } - - bool eventOccurred = false; - PinEventTypes typeOfEventOccured = PinEventTypes.None; - void Callback(object o, PinValueChangedEventArgs e) - { - eventOccurred = true; - typeOfEventOccured = e.ChangeType; - } - - WaitForEventResult(cancellationToken, eventHandler.CancellationToken, ref eventOccurred); - RemoveCallbackForPinValueChangedEvent(pinNumber, Callback); - - return new WaitForEventResult - { - TimedOut = !eventOccurred, - EventTypes = eventOccurred ? typeOfEventOccured : PinEventTypes.None, - }; - } - else - { - throw ExceptionHelper.GetArgumentException(ExceptionResource.InvalidEventType); - } - } - - private void WaitForEventResult(CancellationToken sourceToken, CancellationToken parentToken, ref bool eventOccurred) - { - while (!(sourceToken.IsCancellationRequested || parentToken.IsCancellationRequested || eventOccurred)) - { - Thread.Sleep(1); - } - } - - /// - protected internal override void Write(int pinNumber, PinValue value) - { - if (!_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle)) - { - throw ExceptionHelper.GetInvalidOperationException(ExceptionResource.PinNotOpenedError, - pin: pinNumber); - } - - Interop.libgpiod.gpiod_line_set_value(pinHandle, (value == PinValue.High) ? 1 : 0); - } - - /// - protected override void Dispose(bool disposing) - { - if (_pinNumberToEventHandler != null) - { - foreach (KeyValuePair kv in _pinNumberToEventHandler) - { - LibGpiodDriverEventHandler eventHandler = kv.Value; - eventHandler.Dispose(); - } - - _pinNumberToEventHandler.Clear(); - } - - if (_pinNumberToSafeLineHandle != null) - { - foreach (int pin in _pinNumberToSafeLineHandle.Keys) - { - if (_pinNumberToSafeLineHandle.TryGetValue(pin, out SafeLineHandle? pinHandle)) - { - pinHandle?.Dispose(); - } - } - - _pinNumberToSafeLineHandle.Clear(); - } - - _chip?.Dispose(); - _chip = null!; - - base.Dispose(disposing); - } - } -} From c96a27d2b5493bdf307ec256ac4e47f47fdb6fde Mon Sep 17 00:00:00 2001 From: Patrick Grawehr Date: Sun, 5 May 2024 16:43:35 +0200 Subject: [PATCH 5/9] Add another test --- src/System.Device.Gpio.Tests/GpioControllerTestBase.cs | 9 +++++++++ src/System.Device.Gpio.Tests/LibGpiodV1DriverTests.cs | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/System.Device.Gpio.Tests/GpioControllerTestBase.cs b/src/System.Device.Gpio.Tests/GpioControllerTestBase.cs index c2fd94c396..2732061a96 100644 --- a/src/System.Device.Gpio.Tests/GpioControllerTestBase.cs +++ b/src/System.Device.Gpio.Tests/GpioControllerTestBase.cs @@ -595,6 +595,15 @@ void Callback(object sender, PinValueChangedEventArgs pinValueChangedEventArgs) } } + [Fact] + public void UsingPinAfterDriverDisposedCausesException() + { + var controller = new GpioController(PinNumberingScheme.Logical, GetTestDriver()); + var pin6 = controller.OpenPin(InputPin, PinMode.Input); + controller.Dispose(); + Assert.Throws(() => pin6.Read()); + } + protected abstract GpioDriver GetTestDriver(); protected abstract PinNumberingScheme GetTestNumberingScheme(); } diff --git a/src/System.Device.Gpio.Tests/LibGpiodV1DriverTests.cs b/src/System.Device.Gpio.Tests/LibGpiodV1DriverTests.cs index a0d69c0076..a2ebf45c19 100644 --- a/src/System.Device.Gpio.Tests/LibGpiodV1DriverTests.cs +++ b/src/System.Device.Gpio.Tests/LibGpiodV1DriverTests.cs @@ -104,4 +104,4 @@ public void LeakingDriverDoesNotCrash() GC.WaitForPendingFinalizers(); } -} \ No newline at end of file +} From 3fa74d6c2eec1841ea7ecb6a8d890d3b43ea38ab Mon Sep 17 00:00:00 2001 From: Patrick Grawehr Date: Sun, 5 May 2024 16:57:22 +0200 Subject: [PATCH 6/9] Adjust build issues, fix tests --- .../GpioControllerTestBase.cs | 21 +- .../Unix/libgpiod/V1/SafeLineHandle.cs | 18 +- .../Gpio/Drivers/Libgpiod/LibGpiodDriver.cs | 2 +- .../Drivers/Libgpiod/V1/LibGpiodV1Driver.cs | 30 +- .../V1/LibGpiodV1DriverEventHandler.cs | 6 +- .../System/Device/Gpio/GpioController.cs | 5 + .../System/Device/Gpio/GpioDriver.cs | 262 +++++++++--------- 7 files changed, 184 insertions(+), 160 deletions(-) diff --git a/src/System.Device.Gpio.Tests/GpioControllerTestBase.cs b/src/System.Device.Gpio.Tests/GpioControllerTestBase.cs index 2732061a96..bec9b3acce 100644 --- a/src/System.Device.Gpio.Tests/GpioControllerTestBase.cs +++ b/src/System.Device.Gpio.Tests/GpioControllerTestBase.cs @@ -601,7 +601,26 @@ public void UsingPinAfterDriverDisposedCausesException() var controller = new GpioController(PinNumberingScheme.Logical, GetTestDriver()); var pin6 = controller.OpenPin(InputPin, PinMode.Input); controller.Dispose(); - Assert.Throws(() => pin6.Read()); + bool correctExceptionSeen = false; + try + { + pin6.Read(); + } + catch (Exception x) when (x is InvalidOperationException || x is ObjectDisposedException) + { + correctExceptionSeen = true; + } + + Assert.True(correctExceptionSeen); + } + + [Fact] + public void UsingControllerAfterDisposeCausesException() + { + var controller = new GpioController(PinNumberingScheme.Logical, GetTestDriver()); + var pin6 = controller.OpenPin(InputPin, PinMode.Input); + controller.Dispose(); + Assert.Throws(() => controller.OpenPin(InputPin, PinMode.Input)); } protected abstract GpioDriver GetTestDriver(); diff --git a/src/System.Device.Gpio/Interop/Unix/libgpiod/V1/SafeLineHandle.cs b/src/System.Device.Gpio/Interop/Unix/libgpiod/V1/SafeLineHandle.cs index 5e71c2eeec..9d87c35682 100644 --- a/src/System.Device.Gpio/Interop/Unix/libgpiod/V1/SafeLineHandle.cs +++ b/src/System.Device.Gpio/Interop/Unix/libgpiod/V1/SafeLineHandle.cs @@ -12,11 +12,6 @@ namespace System.Device.Gpio.Libgpiod.V1; internal sealed class SafeLineHandle : IDisposable { private IntPtr _handle; - public SafeLineHandle() - { - _handle = IntPtr.Zero; - } - public SafeLineHandle(IntPtr handle) { _handle = handle; @@ -29,6 +24,11 @@ public IntPtr Handle { get { + if (_handle == IntPtr.Zero) + { + throw new ObjectDisposedException(nameof(SafeLineHandle)); + } + return _handle; } set @@ -38,21 +38,21 @@ public IntPtr Handle } /// - /// Release the lock on the line handle. + /// Release the lock on the line handle. /// public void ReleaseLock() { // Contrary to intuition, this does not invalidate the handle (see comment on declaration) - Interop.libgpiod.gpiod_line_release(_handle); + Interop.LibgpiodV1.gpiod_line_release(_handle); } - public bool IsInvalid => _handle == IntPtr.Zero || _handle == Interop.libgpiod.InvalidHandleValue; + public bool IsInvalid => _handle == IntPtr.Zero || _handle == Interop.LibgpiodV1.InvalidHandleValue; public void Dispose() { if (_handle != IntPtr.Zero) { - Interop.libgpiod.gpiod_line_release(_handle); + Interop.LibgpiodV1.gpiod_line_release(_handle); _handle = IntPtr.Zero; } } diff --git a/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/LibGpiodDriver.cs b/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/LibGpiodDriver.cs index d9b5c1beb9..6fefb83274 100644 --- a/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/LibGpiodDriver.cs +++ b/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/LibGpiodDriver.cs @@ -144,7 +144,7 @@ public override ComponentInformation QueryComponentInformation() /// protected override void Dispose(bool disposing) { - _driver.Dispose(); + _driver?.Dispose(); base.Dispose(disposing); } } diff --git a/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/V1/LibGpiodV1Driver.cs b/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/V1/LibGpiodV1Driver.cs index b6c4b3804d..734a3cbaa3 100644 --- a/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/V1/LibGpiodV1Driver.cs +++ b/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/V1/LibGpiodV1Driver.cs @@ -116,10 +116,10 @@ private LibGpiodV1DriverEventHandler PopulateEventHandler(int pinNumber) { _pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle); - if (pinHandle is null || (pinHandle is object && !LibgpiodV1.gpiod_line_is_free(pinHandle))) + if (pinHandle is null || (pinHandle is object && !LibgpiodV1.gpiod_line_is_free(pinHandle.Handle))) { pinHandle?.Dispose(); - pinHandle = LibgpiodV1.gpiod_chip_get_line(_chip, pinNumber); + pinHandle = new SafeLineHandle(LibgpiodV1.gpiod_chip_get_line(_chip, pinNumber)); _pinNumberToSafeLineHandle[pinNumber] = pinHandle; } @@ -185,13 +185,13 @@ protected internal override void OpenPin(int pinNumber) return; } - SafeLineHandle pinHandle = LibgpiodV1.gpiod_chip_get_line(_chip, pinNumber); + SafeLineHandle pinHandle = new SafeLineHandle(LibgpiodV1.gpiod_chip_get_line(_chip, pinNumber)); if (pinHandle == null) { throw ExceptionHelper.GetIOException(ExceptionResource.OpenPinError, Marshal.GetLastWin32Error()); } - int mode = LibgpiodV1.gpiod_line_direction(pinHandle); + int mode = LibgpiodV1.gpiod_line_direction(pinHandle.Handle); if (mode == 1) { pinHandle.PinMode = PinMode.Input; @@ -203,7 +203,7 @@ protected internal override void OpenPin(int pinNumber) if (s_isLibgpiodVersion1_5orHigher && pinHandle.PinMode == PinMode.Input) { - int bias = LibgpiodV1.gpiod_line_bias(pinHandle); + int bias = LibgpiodV1.gpiod_line_bias(pinHandle.Handle); if (bias == (int)RequestFlag.GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN) { pinHandle.PinMode = PinMode.InputPullDown; @@ -227,7 +227,7 @@ protected internal override PinValue Read(int pinNumber) { if (_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle)) { - int result = LibgpiodV1.gpiod_line_get_value(pinHandle); + int result = LibgpiodV1.gpiod_line_get_value(pinHandle.Handle); if (result == -1) { throw ExceptionHelper.GetIOException(ExceptionResource.ReadPinError, Marshal.GetLastWin32Error(), pinNumber); @@ -272,12 +272,12 @@ protected internal override void SetPinMode(int pinNumber, PinMode mode) pinHandle.ReleaseLock(); int requestResult = mode switch { - PinMode.Input => LibgpiodV1.gpiod_line_request_input(pinHandle, s_consumerName), - PinMode.InputPullDown => LibgpiodV1.gpiod_line_request_input_flags(pinHandle, s_consumerName, + PinMode.Input => LibgpiodV1.gpiod_line_request_input(pinHandle.Handle, s_consumerName), + PinMode.InputPullDown => LibgpiodV1.gpiod_line_request_input_flags(pinHandle.Handle, s_consumerName, (int)RequestFlag.GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN), - PinMode.InputPullUp => LibgpiodV1.gpiod_line_request_input_flags(pinHandle, s_consumerName, + PinMode.InputPullUp => LibgpiodV1.gpiod_line_request_input_flags(pinHandle.Handle, s_consumerName, (int)RequestFlag.GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP), - PinMode.Output => LibgpiodV1.gpiod_line_request_output(pinHandle, s_consumerName, 0), + PinMode.Output => LibgpiodV1.gpiod_line_request_output(pinHandle.Handle, s_consumerName, 0), _ => -1, }; @@ -304,12 +304,12 @@ protected internal override void SetPinMode(int pinNumber, PinMode mode, PinValu pinHandle.ReleaseLock(); int requestResult = mode switch { - PinMode.Input => LibgpiodV1.gpiod_line_request_input(pinHandle, s_consumerName), - PinMode.InputPullDown => LibgpiodV1.gpiod_line_request_input_flags(pinHandle, s_consumerName, + PinMode.Input => LibgpiodV1.gpiod_line_request_input(pinHandle.Handle, s_consumerName), + PinMode.InputPullDown => LibgpiodV1.gpiod_line_request_input_flags(pinHandle.Handle, s_consumerName, (int)RequestFlag.GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN), - PinMode.InputPullUp => LibgpiodV1.gpiod_line_request_input_flags(pinHandle, s_consumerName, + PinMode.InputPullUp => LibgpiodV1.gpiod_line_request_input_flags(pinHandle.Handle, s_consumerName, (int)RequestFlag.GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP), - PinMode.Output => LibgpiodV1.gpiod_line_request_output(pinHandle, s_consumerName, initialValue == PinValue.High ? 1 : 0), + PinMode.Output => LibgpiodV1.gpiod_line_request_output(pinHandle.Handle, s_consumerName, initialValue == PinValue.High ? 1 : 0), _ => -1, }; @@ -384,7 +384,7 @@ protected internal override void Write(int pinNumber, PinValue value) pin: pinNumber); } - LibgpiodV1.gpiod_line_set_value(pinHandle, (value == PinValue.High) ? 1 : 0); + LibgpiodV1.gpiod_line_set_value(pinHandle.Handle, (value == PinValue.High) ? 1 : 0); _pinValue[pinNumber] = value; } diff --git a/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/V1/LibGpiodV1DriverEventHandler.cs b/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/V1/LibGpiodV1DriverEventHandler.cs index 4f5a3aa0f3..b239830c4c 100644 --- a/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/V1/LibGpiodV1DriverEventHandler.cs +++ b/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/V1/LibGpiodV1DriverEventHandler.cs @@ -36,7 +36,7 @@ public LibGpiodV1DriverEventHandler(int pinNumber, SafeLineHandle safeLineHandle private void SubscribeForEvent(SafeLineHandle pinHandle) { - int eventSuccess = LibgpiodV1.gpiod_line_request_both_edges_events(pinHandle, s_consumerName); + int eventSuccess = LibgpiodV1.gpiod_line_request_both_edges_events(pinHandle.Handle, s_consumerName); if (eventSuccess < 0) { @@ -57,7 +57,7 @@ private Task InitializeEventDetectionTask(CancellationToken token, SafeLineHandl TvNsec = new IntPtr(50_000_000) }; - WaitEventResult waitResult = LibgpiodV1.gpiod_line_event_wait(pinHandle, ref timeout); + WaitEventResult waitResult = LibgpiodV1.gpiod_line_event_wait(pinHandle.Handle, ref timeout); if (waitResult == WaitEventResult.Error) { var errorCode = Marshal.GetLastWin32Error(); @@ -73,7 +73,7 @@ private Task InitializeEventDetectionTask(CancellationToken token, SafeLineHandl if (waitResult == WaitEventResult.EventOccured) { GpioLineEvent eventResult = new GpioLineEvent(); - int checkForEvent = LibgpiodV1.gpiod_line_event_read(pinHandle, ref eventResult); + int checkForEvent = LibgpiodV1.gpiod_line_event_read(pinHandle.Handle, ref eventResult); if (checkForEvent == -1) { throw ExceptionHelper.GetIOException(ExceptionResource.EventReadError, Marshal.GetLastWin32Error()); diff --git a/src/System.Device.Gpio/System/Device/Gpio/GpioController.cs b/src/System.Device.Gpio/System/Device/Gpio/GpioController.cs index fd1ee6a960..f4b078b430 100644 --- a/src/System.Device.Gpio/System/Device/Gpio/GpioController.cs +++ b/src/System.Device.Gpio/System/Device/Gpio/GpioController.cs @@ -233,6 +233,11 @@ public virtual PinMode GetPinMode(int pinNumber) /// The status if the pin is open or closed. public bool IsPinOpen(int pinNumber) { + if (_driver == null) + { + throw new ObjectDisposedException(nameof(GpioController)); + } + return _openPins.ContainsKey(pinNumber); } diff --git a/src/System.Device.Gpio/System/Device/Gpio/GpioDriver.cs b/src/System.Device.Gpio/System/Device/Gpio/GpioDriver.cs index 5837085270..2c9aee12c0 100644 --- a/src/System.Device.Gpio/System/Device/Gpio/GpioDriver.cs +++ b/src/System.Device.Gpio/System/Device/Gpio/GpioDriver.cs @@ -13,149 +13,149 @@ namespace System.Device.Gpio; public abstract class GpioDriver : IDisposable { /// - /// Finalizer to clean up unmanaged resources - /// - ~GpioDriver() - { - Dispose(false); - } + /// Finalizer to clean up unmanaged resources + /// + ~GpioDriver() + { + Dispose(false); + } + + /// + /// The number of pins provided by the driver. + /// + protected internal abstract int PinCount { get; } + + /// + /// Converts a board pin number to the driver's logical numbering scheme. + /// + /// The board pin number to convert. + /// The pin number in the driver's logical numbering scheme. + protected internal abstract int ConvertPinNumberToLogicalNumberingScheme(int pinNumber); + + /// + /// Opens a pin in order for it to be ready to use. + /// The driver attempts to open the pin without changing its mode or value. + /// + /// The pin number in the driver's logical numbering scheme. + protected internal abstract void OpenPin(int pinNumber); + + /// + /// Closes an open pin. + /// + /// The pin number in the driver's logical numbering scheme. + protected internal abstract void ClosePin(int pinNumber); - /// - /// The number of pins provided by the driver. - /// - protected internal abstract int PinCount { get; } - - /// - /// Converts a board pin number to the driver's logical numbering scheme. - /// - /// The board pin number to convert. - /// The pin number in the driver's logical numbering scheme. - protected internal abstract int ConvertPinNumberToLogicalNumberingScheme(int pinNumber); - - /// - /// Opens a pin in order for it to be ready to use. - /// The driver attempts to open the pin without changing its mode or value. - /// - /// The pin number in the driver's logical numbering scheme. - protected internal abstract void OpenPin(int pinNumber); - - /// - /// Closes an open pin. - /// - /// The pin number in the driver's logical numbering scheme. - protected internal abstract void ClosePin(int pinNumber); - - /// - /// Sets the mode to a pin. - /// - /// The pin number in the driver's logical numbering scheme. - /// The mode to be set. - protected internal abstract void SetPinMode(int pinNumber, PinMode mode); - - /// - /// Sets the mode to a pin and sets an initial value for an output pin. - /// - /// The pin number in the driver's logical numbering scheme. - /// The mode to be set. - /// The initial value if the is output. The driver will do it's best to prevent glitches to the other value when - /// changing from input to output. - protected internal virtual void SetPinMode(int pinNumber, PinMode mode, PinValue initialValue) + /// + /// Sets the mode to a pin. + /// + /// The pin number in the driver's logical numbering scheme. + /// The mode to be set. + protected internal abstract void SetPinMode(int pinNumber, PinMode mode); + + /// + /// Sets the mode to a pin and sets an initial value for an output pin. + /// + /// The pin number in the driver's logical numbering scheme. + /// The mode to be set. + /// The initial value if the is output. The driver will do it's best to prevent glitches to the other value when + /// changing from input to output. + protected internal virtual void SetPinMode(int pinNumber, PinMode mode, PinValue initialValue) + { + SetPinMode(pinNumber, mode); + if (mode == PinMode.Output) { - SetPinMode(pinNumber, mode); - if (mode == PinMode.Output) - { - Write(pinNumber, initialValue); - } + Write(pinNumber, initialValue); } + } - /// - /// Gets the mode of a pin. - /// - /// The pin number in the driver's logical numbering scheme. - /// The mode of the pin. - protected internal abstract PinMode GetPinMode(int pinNumber); - - /// - /// Checks if a pin supports a specific mode. - /// - /// The pin number in the driver's logical numbering scheme. - /// The mode to check. - /// The status if the pin supports the mode. - protected internal abstract bool IsPinModeSupported(int pinNumber, PinMode mode); - - /// - /// Reads the current value of a pin. - /// - /// The pin number in the driver's logical numbering scheme. - /// The value of the pin. - protected internal abstract PinValue Read(int pinNumber); - - /// + /// + /// Gets the mode of a pin. + /// + /// The pin number in the driver's logical numbering scheme. + /// The mode of the pin. + protected internal abstract PinMode GetPinMode(int pinNumber); + + /// + /// Checks if a pin supports a specific mode. + /// + /// The pin number in the driver's logical numbering scheme. + /// The mode to check. + /// The status if the pin supports the mode. + protected internal abstract bool IsPinModeSupported(int pinNumber, PinMode mode); + + /// + /// Reads the current value of a pin. + /// + /// The pin number in the driver's logical numbering scheme. + /// The value of the pin. + protected internal abstract PinValue Read(int pinNumber); + + /// /// Toggle the current value of a pin. /// /// The pin number in the driver's logical numbering scheme. protected internal virtual void Toggle(int pinNumber) => Write(pinNumber, !Read(pinNumber)); /// - /// Writes a value to a pin. - /// - /// The pin number in the driver's logical numbering scheme. - /// The value to be written to the pin. - protected internal abstract void Write(int pinNumber, PinValue value); - - /// - /// Blocks execution until an event of type eventType is received or a cancellation is requested. - /// - /// The pin number in the driver's logical numbering scheme. - /// The event types to wait for. - /// The cancellation token of when the operation should stop waiting for an event. - /// A structure that contains the result of the waiting operation. - protected internal abstract WaitForEventResult WaitForEvent(int pinNumber, PinEventTypes eventTypes, CancellationToken cancellationToken); - - /// - /// Async call until an event of type eventType is received or a cancellation is requested. - /// - /// The pin number in the driver's logical numbering scheme. - /// The event types to wait for. - /// The cancellation token of when the operation should stop waiting for an event. - /// A task representing the operation of getting the structure that contains the result of the waiting operation - protected internal virtual ValueTask WaitForEventAsync(int pinNumber, PinEventTypes eventTypes, CancellationToken cancellationToken) - { - return new ValueTask(Task.Run(() => WaitForEvent(pinNumber, eventTypes, cancellationToken))); - } + /// Writes a value to a pin. + /// + /// The pin number in the driver's logical numbering scheme. + /// The value to be written to the pin. + protected internal abstract void Write(int pinNumber, PinValue value); - /// - /// Adds a handler for a pin value changed event. - /// - /// The pin number in the driver's logical numbering scheme. - /// The event types to wait for. - /// Delegate that defines the structure for callbacks when a pin value changed event occurs. - protected internal abstract void AddCallbackForPinValueChangedEvent(int pinNumber, PinEventTypes eventTypes, PinChangeEventHandler callback); - - /// - /// Removes a handler for a pin value changed event. - /// - /// The pin number in the driver's logical numbering scheme. - /// Delegate that defines the structure for callbacks when a pin value changed event occurs. - protected internal abstract void RemoveCallbackForPinValueChangedEvent(int pinNumber, PinChangeEventHandler callback); - - /// - /// Disposes this instance, closing all open pins - /// - public void Dispose() - { - Dispose(true); - GC.SuppressFinalize(this); - } + /// + /// Blocks execution until an event of type eventType is received or a cancellation is requested. + /// + /// The pin number in the driver's logical numbering scheme. + /// The event types to wait for. + /// The cancellation token of when the operation should stop waiting for an event. + /// A structure that contains the result of the waiting operation. + protected internal abstract WaitForEventResult WaitForEvent(int pinNumber, PinEventTypes eventTypes, CancellationToken cancellationToken); - /// - /// Disposes this instance - /// - /// True if explicitly disposing, false if in finalizer - protected virtual void Dispose(bool disposing) - { - // Nothing to do in base class. - } + /// + /// Async call until an event of type eventType is received or a cancellation is requested. + /// + /// The pin number in the driver's logical numbering scheme. + /// The event types to wait for. + /// The cancellation token of when the operation should stop waiting for an event. + /// A task representing the operation of getting the structure that contains the result of the waiting operation + protected internal virtual ValueTask WaitForEventAsync(int pinNumber, PinEventTypes eventTypes, CancellationToken cancellationToken) + { + return new ValueTask(Task.Run(() => WaitForEvent(pinNumber, eventTypes, cancellationToken))); + } + + /// + /// Adds a handler for a pin value changed event. + /// + /// The pin number in the driver's logical numbering scheme. + /// The event types to wait for. + /// Delegate that defines the structure for callbacks when a pin value changed event occurs. + protected internal abstract void AddCallbackForPinValueChangedEvent(int pinNumber, PinEventTypes eventTypes, PinChangeEventHandler callback); + + /// + /// Removes a handler for a pin value changed event. + /// + /// The pin number in the driver's logical numbering scheme. + /// Delegate that defines the structure for callbacks when a pin value changed event occurs. + protected internal abstract void RemoveCallbackForPinValueChangedEvent(int pinNumber, PinChangeEventHandler callback); + + /// + /// Disposes this instance, closing all open pins + /// + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } + + /// + /// Disposes this instance + /// + /// True if explicitly disposing, false if in finalizer + protected virtual void Dispose(bool disposing) + { + // Nothing to do in base class. + } /// /// Query information about a component and its children. From 7c4dcd21f438ba5f224efadbb6eba879c261568c Mon Sep 17 00:00:00 2001 From: Patrick Grawehr Date: Sun, 5 May 2024 20:37:47 +0200 Subject: [PATCH 7/9] An added finalizer seems to throw this false(?) warning --- src/System.Device.Gpio/CompatibilitySuppressions.xml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/System.Device.Gpio/CompatibilitySuppressions.xml b/src/System.Device.Gpio/CompatibilitySuppressions.xml index 75f2976ac2..1fc650add8 100644 --- a/src/System.Device.Gpio/CompatibilitySuppressions.xml +++ b/src/System.Device.Gpio/CompatibilitySuppressions.xml @@ -13,6 +13,12 @@ lib/netstandard2.0/System.Device.Gpio.dll lib/net6.0-windows10.0.17763/System.Device.Gpio.dll + + CP0002 + M:System.Device.Gpio.GpioDriver.Finalize + lib/netstandard2.0/System.Device.Gpio.dll + lib/net6.0-windows10.0.17763/System.Device.Gpio.dll + CP0002 M:System.Device.Gpio.PinEventTypes.#ctor From 32f5d4e96677d10991887cd49999d8ef9fcd46dc Mon Sep 17 00:00:00 2001 From: Patrick Grawehr Date: Thu, 23 May 2024 20:53:16 +0200 Subject: [PATCH 8/9] Review findings --- .../GpioControllerTestBase.cs | 2 +- .../V1/{SafeLineHandle.cs => LineHandle.cs} | 6 ++--- .../Gpio/Drivers/Libgpiod/LibGpiodDriver.cs | 9 +++++-- .../Drivers/Libgpiod/V1/LibGpiodV1Driver.cs | 24 +++++++++---------- .../V1/LibGpiodV1DriverEventHandler.cs | 6 ++--- .../System/Device/Gpio/GpioController.cs | 18 +++++++++++--- 6 files changed, 41 insertions(+), 24 deletions(-) rename src/System.Device.Gpio/Interop/Unix/libgpiod/V1/{SafeLineHandle.cs => LineHandle.cs} (89%) diff --git a/src/System.Device.Gpio.Tests/GpioControllerTestBase.cs b/src/System.Device.Gpio.Tests/GpioControllerTestBase.cs index bec9b3acce..d21b3b51de 100644 --- a/src/System.Device.Gpio.Tests/GpioControllerTestBase.cs +++ b/src/System.Device.Gpio.Tests/GpioControllerTestBase.cs @@ -606,7 +606,7 @@ public void UsingPinAfterDriverDisposedCausesException() { pin6.Read(); } - catch (Exception x) when (x is InvalidOperationException || x is ObjectDisposedException) + catch (ObjectDisposedException) { correctExceptionSeen = true; } diff --git a/src/System.Device.Gpio/Interop/Unix/libgpiod/V1/SafeLineHandle.cs b/src/System.Device.Gpio/Interop/Unix/libgpiod/V1/LineHandle.cs similarity index 89% rename from src/System.Device.Gpio/Interop/Unix/libgpiod/V1/SafeLineHandle.cs rename to src/System.Device.Gpio/Interop/Unix/libgpiod/V1/LineHandle.cs index 9d87c35682..1b606fdad6 100644 --- a/src/System.Device.Gpio/Interop/Unix/libgpiod/V1/SafeLineHandle.cs +++ b/src/System.Device.Gpio/Interop/Unix/libgpiod/V1/LineHandle.cs @@ -9,10 +9,10 @@ namespace System.Device.Gpio.Libgpiod.V1; /// /// Pointer to a pin (Not a real SafeLineHandle, because we need to align its finalization with the owning object) /// -internal sealed class SafeLineHandle : IDisposable +internal sealed class LineHandle : IDisposable { private IntPtr _handle; - public SafeLineHandle(IntPtr handle) + public LineHandle(IntPtr handle) { _handle = handle; PinMode = PinMode.Input; @@ -26,7 +26,7 @@ public IntPtr Handle { if (_handle == IntPtr.Zero) { - throw new ObjectDisposedException(nameof(SafeLineHandle)); + throw new ObjectDisposedException(nameof(LineHandle)); } return _handle; diff --git a/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/LibGpiodDriver.cs b/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/LibGpiodDriver.cs index 6fefb83274..4c20d05c75 100644 --- a/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/LibGpiodDriver.cs +++ b/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/LibGpiodDriver.cs @@ -11,7 +11,7 @@ namespace System.Device.Gpio.Drivers; /// public class LibGpiodDriver : UnixDriver { - private readonly GpioDriver _driver; + private GpioDriver _driver; /// /// Creates an instance of the LibGpiodDriver @@ -144,7 +144,12 @@ public override ComponentInformation QueryComponentInformation() /// protected override void Dispose(bool disposing) { - _driver?.Dispose(); + if (_driver != null && disposing) + { + _driver.Dispose(); + _driver = null!; + } + base.Dispose(disposing); } } diff --git a/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/V1/LibGpiodV1Driver.cs b/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/V1/LibGpiodV1Driver.cs index 734a3cbaa3..912168a698 100644 --- a/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/V1/LibGpiodV1Driver.cs +++ b/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/V1/LibGpiodV1Driver.cs @@ -20,7 +20,7 @@ internal class LibGpiodV1Driver : UnixDriver { private static string s_consumerName = Process.GetCurrentProcess().ProcessName; private readonly object _pinNumberLock; - private readonly ConcurrentDictionary _pinNumberToSafeLineHandle; + private readonly ConcurrentDictionary _pinNumberToSafeLineHandle; private readonly ConcurrentDictionary _pinNumberToEventHandler; private readonly int _pinCount; private readonly ConcurrentDictionary _pinValue; @@ -78,7 +78,7 @@ public LibGpiodV1Driver(int gpioChip = 0) _pinCount = LibgpiodV1.gpiod_chip_num_lines(_chip); _pinNumberToEventHandler = new ConcurrentDictionary(); - _pinNumberToSafeLineHandle = new ConcurrentDictionary(); + _pinNumberToSafeLineHandle = new ConcurrentDictionary(); _pinValue = new ConcurrentDictionary(); } catch (DllNotFoundException) @@ -114,12 +114,12 @@ private LibGpiodV1DriverEventHandler PopulateEventHandler(int pinNumber) { lock (_pinNumberLock) { - _pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle); + _pinNumberToSafeLineHandle.TryGetValue(pinNumber, out LineHandle? pinHandle); if (pinHandle is null || (pinHandle is object && !LibgpiodV1.gpiod_line_is_free(pinHandle.Handle))) { pinHandle?.Dispose(); - pinHandle = new SafeLineHandle(LibgpiodV1.gpiod_chip_get_line(_chip, pinNumber)); + pinHandle = new LineHandle(LibgpiodV1.gpiod_chip_get_line(_chip, pinNumber)); _pinNumberToSafeLineHandle[pinNumber] = pinHandle; } @@ -132,7 +132,7 @@ protected internal override void ClosePin(int pinNumber) { lock (_pinNumberLock) { - if (_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle) && + if (_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out LineHandle? pinHandle) && !IsListeningEvent(pinNumber)) { pinHandle?.Dispose(); @@ -157,7 +157,7 @@ protected internal override PinMode GetPinMode(int pinNumber) { lock (_pinNumberLock) { - if (!_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle)) + if (!_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out LineHandle? pinHandle)) { throw ExceptionHelper.GetInvalidOperationException(ExceptionResource.PinNotOpenedError, pin: pinNumber); @@ -185,7 +185,7 @@ protected internal override void OpenPin(int pinNumber) return; } - SafeLineHandle pinHandle = new SafeLineHandle(LibgpiodV1.gpiod_chip_get_line(_chip, pinNumber)); + LineHandle pinHandle = new LineHandle(LibgpiodV1.gpiod_chip_get_line(_chip, pinNumber)); if (pinHandle == null) { throw ExceptionHelper.GetIOException(ExceptionResource.OpenPinError, Marshal.GetLastWin32Error()); @@ -225,7 +225,7 @@ protected internal override void OpenPin(int pinNumber) /// protected internal override PinValue Read(int pinNumber) { - if (_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle)) + if (_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out LineHandle? pinHandle)) { int result = LibgpiodV1.gpiod_line_get_value(pinHandle.Handle); if (result == -1) @@ -265,7 +265,7 @@ protected internal override void RemoveCallbackForPinValueChangedEvent(int pinNu /// protected internal override void SetPinMode(int pinNumber, PinMode mode) { - if (_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle)) + if (_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out LineHandle? pinHandle)) { // This call does not release the handle. It only releases the lock on the handle. Without this, changing the direction of a line is not possible. // Line handles cannot be freed and are cached until the chip is closed. @@ -297,7 +297,7 @@ protected internal override void SetPinMode(int pinNumber, PinMode mode) /// protected internal override void SetPinMode(int pinNumber, PinMode mode, PinValue initialValue) { - if (_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle)) + if (_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out LineHandle? pinHandle)) { // This call does not release the handle. It only releases the lock on the handle. Without this, changing the direction of a line is not possible. // Line handles cannot be freed and are cached until the chip is closed. @@ -378,7 +378,7 @@ private void WaitForEventResult(CancellationToken sourceToken, CancellationToken /// protected internal override void Write(int pinNumber, PinValue value) { - if (!_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle)) + if (!_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out LineHandle? pinHandle)) { throw ExceptionHelper.GetInvalidOperationException(ExceptionResource.PinNotOpenedError, pin: pinNumber); @@ -416,7 +416,7 @@ protected override void Dispose(bool disposing) { foreach (int pin in _pinNumberToSafeLineHandle.Keys) { - if (_pinNumberToSafeLineHandle.TryGetValue(pin, out SafeLineHandle? pinHandle)) + if (_pinNumberToSafeLineHandle.TryGetValue(pin, out LineHandle? pinHandle)) { pinHandle?.Dispose(); } diff --git a/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/V1/LibGpiodV1DriverEventHandler.cs b/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/V1/LibGpiodV1DriverEventHandler.cs index b239830c4c..c592389558 100644 --- a/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/V1/LibGpiodV1DriverEventHandler.cs +++ b/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/V1/LibGpiodV1DriverEventHandler.cs @@ -24,7 +24,7 @@ internal sealed class LibGpiodV1DriverEventHandler : IDisposable private readonly Task _task; private bool _disposing; - public LibGpiodV1DriverEventHandler(int pinNumber, SafeLineHandle safeLineHandle) + public LibGpiodV1DriverEventHandler(int pinNumber, LineHandle safeLineHandle) { _pinNumber = pinNumber; _cancellationTokenSource = new CancellationTokenSource(); @@ -34,7 +34,7 @@ public LibGpiodV1DriverEventHandler(int pinNumber, SafeLineHandle safeLineHandle public CancellationToken CancellationToken => _cancellationTokenSource.Token; - private void SubscribeForEvent(SafeLineHandle pinHandle) + private void SubscribeForEvent(LineHandle pinHandle) { int eventSuccess = LibgpiodV1.gpiod_line_request_both_edges_events(pinHandle.Handle, s_consumerName); @@ -44,7 +44,7 @@ private void SubscribeForEvent(SafeLineHandle pinHandle) } } - private Task InitializeEventDetectionTask(CancellationToken token, SafeLineHandle pinHandle) + private Task InitializeEventDetectionTask(CancellationToken token, LineHandle pinHandle) { return Task.Run(() => { diff --git a/src/System.Device.Gpio/System/Device/Gpio/GpioController.cs b/src/System.Device.Gpio/System/Device/Gpio/GpioController.cs index f4b078b430..985b41f72e 100644 --- a/src/System.Device.Gpio/System/Device/Gpio/GpioController.cs +++ b/src/System.Device.Gpio/System/Device/Gpio/GpioController.cs @@ -75,7 +75,14 @@ public GpioController(PinNumberingScheme numberingScheme) /// /// The number of pins provided by the controller. /// - public virtual int PinCount => _driver.PinCount; + public virtual int PinCount + { + get + { + CheckDriverValid(); + return _driver.PinCount; + } + } /// /// Returns the collection of open pins @@ -232,13 +239,17 @@ public virtual PinMode GetPinMode(int pinNumber) /// The pin number in the controller's numbering scheme. /// The status if the pin is open or closed. public bool IsPinOpen(int pinNumber) + { + CheckDriverValid(); + return _openPins.ContainsKey(pinNumber); + } + + private void CheckDriverValid() { if (_driver == null) { throw new ObjectDisposedException(nameof(GpioController)); } - - return _openPins.ContainsKey(pinNumber); } /// @@ -249,6 +260,7 @@ public bool IsPinOpen(int pinNumber) /// The status if the pin supports the mode. public virtual bool IsPinModeSupported(int pinNumber, PinMode mode) { + CheckDriverValid(); int logicalPinNumber = GetLogicalPinNumber(pinNumber); return _driver.IsPinModeSupported(logicalPinNumber, mode); } From 3bf0188499a3678ab4c20c1830804ab7d964e11a Mon Sep 17 00:00:00 2001 From: Patrick Grawehr Date: Thu, 23 May 2024 21:25:17 +0200 Subject: [PATCH 9/9] Properly check for disposed drivers --- .../Gpio/Drivers/Libgpiod/LibGpiodDriver.cs | 43 ++++++++++++------- .../System/Device/Gpio/Drivers/SysFsDriver.cs | 23 ++++++++++ 2 files changed, 51 insertions(+), 15 deletions(-) diff --git a/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/LibGpiodDriver.cs b/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/LibGpiodDriver.cs index 4c20d05c75..d36ebc6209 100644 --- a/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/LibGpiodDriver.cs +++ b/src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/LibGpiodDriver.cs @@ -55,90 +55,103 @@ public LibGpiodDriver(int gpioChip, LibGpiodDriverVersion driverVersion) public static LibGpiodDriverVersion[] GetAvailableVersions() => LibGpiodDriverFactory.Instance.DriverCandidates; /// - protected internal override int PinCount => _driver.PinCount; + protected internal override int PinCount => GetDriver().PinCount; /// protected internal override void AddCallbackForPinValueChangedEvent(int pinNumber, PinEventTypes eventTypes, PinChangeEventHandler callback) { - _driver.AddCallbackForPinValueChangedEvent(pinNumber, eventTypes, callback); + GetDriver().AddCallbackForPinValueChangedEvent(pinNumber, eventTypes, callback); } /// protected internal override void ClosePin(int pinNumber) { - _driver.ClosePin(pinNumber); + GetDriver().ClosePin(pinNumber); } /// protected internal override int ConvertPinNumberToLogicalNumberingScheme(int pinNumber) { - return _driver.ConvertPinNumberToLogicalNumberingScheme(pinNumber); + return GetDriver().ConvertPinNumberToLogicalNumberingScheme(pinNumber); } /// protected internal override PinMode GetPinMode(int pinNumber) { - return _driver.GetPinMode(pinNumber); + return GetDriver().GetPinMode(pinNumber); } /// protected internal override bool IsPinModeSupported(int pinNumber, PinMode mode) { - return _driver.IsPinModeSupported(pinNumber, mode); + return GetDriver().IsPinModeSupported(pinNumber, mode); } /// protected internal override void OpenPin(int pinNumber) { - _driver.OpenPin(pinNumber); + GetDriver().OpenPin(pinNumber); } /// protected internal override PinValue Read(int pinNumber) { - return _driver.Read(pinNumber); + return GetDriver().Read(pinNumber); } /// protected internal override void Toggle(int pinNumber) { - _driver.Toggle(pinNumber); + GetDriver().Toggle(pinNumber); } /// protected internal override void RemoveCallbackForPinValueChangedEvent(int pinNumber, PinChangeEventHandler callback) { - _driver.RemoveCallbackForPinValueChangedEvent(pinNumber, callback); + GetDriver().RemoveCallbackForPinValueChangedEvent(pinNumber, callback); } /// protected internal override void SetPinMode(int pinNumber, PinMode mode) { - _driver.SetPinMode(pinNumber, mode); + GetDriver().SetPinMode(pinNumber, mode); } /// protected internal override void SetPinMode(int pinNumber, PinMode mode, PinValue initialValue) { - _driver.SetPinMode(pinNumber, mode, initialValue); + GetDriver().SetPinMode(pinNumber, mode, initialValue); } /// protected internal override WaitForEventResult WaitForEvent(int pinNumber, PinEventTypes eventTypes, CancellationToken cancellationToken) { - return _driver.WaitForEvent(pinNumber, eventTypes, cancellationToken); + return GetDriver().WaitForEvent(pinNumber, eventTypes, cancellationToken); } /// protected internal override void Write(int pinNumber, PinValue value) { - _driver.Write(pinNumber, value); + GetDriver().Write(pinNumber, value); } /// public override ComponentInformation QueryComponentInformation() { - return _driver.QueryComponentInformation(); + var ret = new ComponentInformation(this, "Libgpiod Wrapper driver"); + ret.Properties.Add("Version", Version.ToString()); + ret.AddSubComponent(GetDriver().QueryComponentInformation()); + return ret; + } + + private GpioDriver GetDriver() + { + if (_driver == null) + { + throw new ObjectDisposedException(nameof(LibGpiodDriver)); + } + + return _driver; } /// diff --git a/src/System.Device.Gpio/System/Device/Gpio/Drivers/SysFsDriver.cs b/src/System.Device.Gpio/System/Device/Gpio/Drivers/SysFsDriver.cs index 5bf0d062d7..af6209a016 100644 --- a/src/System.Device.Gpio/System/Device/Gpio/Drivers/SysFsDriver.cs +++ b/src/System.Device.Gpio/System/Device/Gpio/Drivers/SysFsDriver.cs @@ -35,6 +35,8 @@ public class SysFsDriver : UnixDriver private int _pinsToDetectEventsCount; private CancellationTokenSource? _eventThreadCancellationTokenSource; + private bool _isDisposed; + private static int ReadOffset() { IEnumerable fileNames = Directory.EnumerateFileSystemEntries(GpioBasePath); @@ -72,6 +74,8 @@ public SysFsDriver() { throw new PlatformNotSupportedException($"{GetType().Name} is only supported on Linux/Unix."); } + + _isDisposed = false; } /// @@ -108,6 +112,7 @@ internal TimeSpan StatusUpdateSleepTime /// The pin number in the driver's logical numbering scheme. protected internal override void OpenPin(int pinNumber) { + CheckValidDriver(); int pinOffset = pinNumber + s_pinOffset; string pinPath = $"{GpioBasePath}/gpio{pinOffset}"; // If the directory exists, this becomes a no-op since the pin might have been opened already by the some controller or somebody else. @@ -136,6 +141,7 @@ protected internal override void OpenPin(int pinNumber) /// The pin number in the driver's logical numbering scheme. protected internal override void ClosePin(int pinNumber) { + CheckValidDriver(); int pinOffset = pinNumber + s_pinOffset; string pinPath = $"{GpioBasePath}/gpio{pinOffset}"; // If the directory doesn't exist, this becomes a no-op since the pin was closed already. @@ -171,6 +177,7 @@ protected internal override void ClosePin(int pinNumber) /// The mode to be set. protected internal override void SetPinMode(int pinNumber, PinMode mode) { + CheckValidDriver(); if (mode == PinMode.InputPullDown || mode == PinMode.InputPullUp) { throw new PlatformNotSupportedException("This driver is generic so it does not support Input Pull Down or Input Pull Up modes."); @@ -233,6 +240,7 @@ private PinMode ConvertSysFsModeToPinMode(string sysFsMode) /// The value of the pin. protected internal override PinValue Read(int pinNumber) { + CheckValidDriver(); PinValue result = default; string valuePath = $"{GpioBasePath}/gpio{pinNumber + s_pinOffset}/value"; if (File.Exists(valuePath)) @@ -276,6 +284,7 @@ private PinValue ConvertSysFsValueToPinValue(string value) /// The value to be written to the pin. protected internal override void Write(int pinNumber, PinValue value) { + CheckValidDriver(); string valuePath = $"{GpioBasePath}/gpio{pinNumber + s_pinOffset}/value"; if (File.Exists(valuePath)) { @@ -307,6 +316,7 @@ private string ConvertPinValueToSysFs(PinValue value) /// The status if the pin supports the mode. protected internal override bool IsPinModeSupported(int pinNumber, PinMode mode) { + CheckValidDriver(); // Unix driver does not support pull up or pull down resistors. if (mode == PinMode.InputPullDown || mode == PinMode.InputPullUp) { @@ -325,6 +335,7 @@ protected internal override bool IsPinModeSupported(int pinNumber, PinMode mode) /// A structure that contains the result of the waiting operation. protected internal override WaitForEventResult WaitForEvent(int pinNumber, PinEventTypes eventTypes, CancellationToken cancellationToken) { + CheckValidDriver(); int pollFileDescriptor = -1; int valueFileDescriptor = -1; SetPinEventsToDetect(pinNumber, eventTypes); @@ -590,9 +601,18 @@ protected override void Dispose(bool disposing) ClosePin(_exportedPins.FirstOrDefault()); } + _isDisposed = true; base.Dispose(disposing); } + private void CheckValidDriver() + { + if (_isDisposed) + { + throw new ObjectDisposedException(nameof(SysFsDriver)); + } + } + /// /// Adds a handler for a pin value changed event. /// @@ -601,6 +621,7 @@ protected override void Dispose(bool disposing) /// Delegate that defines the structure for callbacks when a pin value changed event occurs. protected internal override void AddCallbackForPinValueChangedEvent(int pinNumber, PinEventTypes eventTypes, PinChangeEventHandler callback) { + CheckValidDriver(); if (!_devicePins.ContainsKey(pinNumber)) { _devicePins.Add(pinNumber, new UnixDriverDevicePin(Read(pinNumber))); @@ -722,6 +743,7 @@ private void DetectEvents() /// Delegate that defines the structure for callbacks when a pin value changed event occurs. protected internal override void RemoveCallbackForPinValueChangedEvent(int pinNumber, PinChangeEventHandler callback) { + CheckValidDriver(); if (!_devicePins.ContainsKey(pinNumber)) { throw new InvalidOperationException("Attempted to remove a callback for a pin that is not listening for events."); @@ -747,6 +769,7 @@ protected internal override void RemoveCallbackForPinValueChangedEvent(int pinNu /// The mode of the pin. protected internal override PinMode GetPinMode(int pinNumber) { + CheckValidDriver(); pinNumber += s_pinOffset; string directionPath = $"{GpioBasePath}/gpio{pinNumber}/direction"; if (File.Exists(directionPath))