Adding GpioPin #1895

Ellerbach · 2022-07-23T15:46:31Z

Adding GpioPin. Following design discussions here: #1671

This PR now implements what's been discussed during the API proposal call and is summarized here: #1920

Microsoft Reviewers: Open in CodeFlow

src/System.Device.Gpio/System/Device/Gpio/GpioController.cs

joperezr · 2022-07-24T06:25:34Z

I was expecting this to be a draft PR, or a PR against a feature branch. What is the intention of this @Ellerbach? This would have to go through APIReview first before it can get merged.

Ellerbach · 2022-07-28T14:33:46Z

This would have to go through APIReview first before it can get merged

That's fine. I can mark the PR as Draft then. If you prefer, I can just push the branch directly to the repo so it will be its own feature branch.

krwq · 2022-07-28T14:42:23Z

@joperezr can we do small API review during triage meeting? This API is not part of the framework and we didn't go through API review for other smaller things

src/System.Device.Gpio/System/Device/Gpio/GpioPin.cs

Ellerbach · 2022-07-28T14:46:57Z

If the breaking change is acceptable for 3.0 and well documented, it's fine to wait for that version to be out. In the mean time, it does allow time to align on the design.

joperezr · 2022-07-28T14:47:12Z

I don't think this is a small thing to be honest. This was one of the biggest design decisions made on the original design, and I do think that getting this reviewed by more people would help as most of us are already a bit biased.

src/System.Device.Gpio/System/Device/Gpio/GpioPin.cs

krwq · 2022-08-19T10:14:32Z

Some notes (+ my after-thoughts) from the offline conversation between me and @Ellerbach today:

abstract class/interface has benefits of being able to remove all validation code and do some pre-calculations and really save time
interface can be implemented with struct which has extra benefit of being able to efficiently store list of pins if used directly
class implementing interface/abstract class when used directly rather than base type it will not have virtual cost (although such usage might be much harder)
two interesting opposite use cases: LED Matrix (stretch of the perf), LCD character screen (many pins which might need to come from different controllers)
non-abstract/no-interface can also remove extra validation if i.e. using non-validating driver overload and since we pre-validate that's safe
pin management makes things much safer so non-validating pin might not always be a good idea
if we cannot make specialized abstract class work fast enough with i.e. LED Matrix then why bother making it abstract if it doesn't solve largest use case for that?
LED matrix is currently using registers directly, perhaps improving that interface to work with other controllers than just RPi3 might be a better idea for perf scenarios than Pin class - possibly register should always return a pointer and be part of GpioController/GpioDriver - this way pref-interested parties can use that, for drivers which don't support that it would either throw or return null sentinel
in general we should do some experimentation here and we're in agreement that low-level concepts are really hard to get right in high level APIs and solving both perf and safety at the same time might not be possible so we should find a good sweet spot between or shift to one side

Ellerbach · 2022-08-22T09:51:50Z

Few elements to take into consideration:

The changes are mainly about removing the sealed so it will allow to create other GpioPin based on a driver for example if needed
From the conversation mentioned up, having an abstract class does not need to be necessary as we will use the controller in almost all cases

src/System.Device.Gpio/System/Device/Gpio/GpioPin.cs

src/System.Device.Gpio/System/Device/Gpio/GpioController.cs

pgrawehr

Looks good overall. What's not so nice IMHO is that the code to keep the current state of the pin for the Toggle operation is duplicated all over the place.

Please also add a test case for this functionality.

I'll have to take a look whether the now valid duplicate open causes some problems in the pin management for the board. I don't expect so, as this should have triggered a test then.

pgrawehr · 2022-11-12T08:20:00Z

src/System.Device.Gpio/System/Device/Gpio/GpioDriver.cs

+    /// Toggle the current value of a pin.
+    /// </summary>
+    /// <param name="pinNumber">The pin number in the driver's logical numbering scheme.</param>
+    protected internal abstract void Toggle(int pinNumber);


That's a real breaking change (binary as well as source). I think this should have a default implementation instead.

Why not PinValue Toggle(int pinNumber)?
I don't know if the hw implementation provides the value, but in the other cases it is certainly available.
If the hardware driver does not provide the value, we can decide to either return null or read the value and return it.
I prefer this signature to avoid future breaking changes in case the hw driver will provide the value in the next releases.

That's a real breaking change (binary as well as source). I think this should have a default implementation instead.

Some hardware provide this feature, so does not. We agreeded that the braking change is ok as it then surfaced on the GpioController following the API call. And the best way to handle the change is to move it to the driver because it's the only place tracking really the state of the pins.

raffaeler

Great work @Ellerbach!

raffaeler · 2022-11-12T10:26:04Z

src/System.Device.Gpio.Tests/GpioPinTests.cs

+        }
+
+        [Fact]
+        public void TestOpnPin()


Typo: TestOpenPin in place of TestOpnPin

raffaeler · 2022-11-12T10:30:42Z

src/System.Device.Gpio/System/Device/Gpio/GpioDriver.cs

+    /// Toggle the current value of a pin.
+    /// </summary>
+    /// <param name="pinNumber">The pin number in the driver's logical numbering scheme.</param>
+    protected internal abstract void Toggle(int pinNumber);


Why not PinValue Toggle(int pinNumber)?
I don't know if the hw implementation provides the value, but in the other cases it is certainly available.
If the hardware driver does not provide the value, we can decide to either return null or read the value and return it.
I prefer this signature to avoid future breaking changes in case the hw driver will provide the value in the next releases.

Ellerbach · 2022-11-12T14:57:32Z

Why not PinValue Toggle(int pinNumber)?

Hardware does not provide. I know that for all current implementation we keep track, so could be provided but in real hardware, it's not.

raffaeler

At this point I guess we can go on with this :)

raffaeler · 2022-11-17T16:47:58Z

We were discussing in the triage to shape the api as PinValue? Toggle(int pinNumber).
My point was the following:

In the software implementation returning the value is free because we track the values
In the hardware implementation providing the value, we return it
In the hardware implementation not providing the value, we can return null (telling that is not available)

@joperezr Do you have an opinion on this?

src/System.Device.Gpio/System/Device/Gpio/GpioDriver.cs

src/System.Device.Gpio/System/Device/Gpio/Drivers/HummingBoardDriver.cs

krwq · 2022-11-18T08:50:41Z

src/System.Device.Gpio/System/Device/Gpio/Drivers/LibGpiodDriver.cs

    private readonly ConcurrentDictionary<int, SafeLineHandle> _pinNumberToSafeLineHandle;
    private readonly ConcurrentDictionary<int, LibGpiodDriverEventHandler> _pinNumberToEventHandler;
    private readonly int _pinCount;
+    private readonly ConcurrentDictionary<int, PinValue> _pinValue;


I'm looking at this and wondering - perhaps GpioDriver could have virtual CreateGpioPin with default implementation - then in here you'd return LibGpiodPin and move CurrentPinValue, EventHandler, SafeLineHandle to LibGpiodPin - this way you could have just one ConcurrentDictionary here and using pin directly would have advantage of not touching ConcurrentDictionary

we talked offline and we can do this as a separate change most likely without any API changes (only addition suggested here)

krwq · 2022-11-18T10:12:03Z

src/System.Device.Gpio.Tests/GpioPinTests.cs

+            GpioPin pin = ctrl.OpenPin(PinNumber, PinMode.Input);
+            // Assert
+            Assert.True(pin.PinNumber == PinNumber);
+            Assert.True(pin.GetPinMode() == PinMode.Input);


nit: Assert.Equal

krwq · 2022-11-18T11:01:04Z

src/System.Device.Gpio/System/Device/Gpio/Drivers/Windows10Driver.cs

@@ -14,6 +14,7 @@ public class Windows10Driver : GpioDriver
 {
    private static readonly WinGpio.GpioController s_winGpioController = WinGpio.GpioController.GetDefault();
    private readonly Dictionary<int, Windows10DriverPin> _openPins = new Dictionary<int, Windows10DriverPin>();
+    private readonly Dictionary<int, PinValue> _pinValues = new Dictionary<int, PinValue>();


why does LibGpiodDriver need ConcurrentDictionary while this one is ok with regular Dictionary?

(it's an existing issue anyway so don't worry about this in this PR)

krwq · 2022-11-18T11:05:52Z

src/devices/Board/KeyboardGpioDriver.cs

+                // Simulate a key press
+                Interop.keybd_event((byte)virtualKey,
+                    0x45,
+                    Interop.KEYEVENTF_EXTENDEDKEY | 0,


nit: | 0 is a nop so Interop.KEYEVENTF_EXTENDEDKEY | 0 is the same as Interop.KEYEVENTF_EXTENDEDKEY

krwq · 2022-11-18T11:09:19Z

src/devices/Mcp23xxx/Mcp23xxx.cs

@@ -519,12 +532,14 @@ private void ThrowBadPin(string argument)
        protected override void OpenPin(int pinNumber)
        {
            // No-op
+            _pinValues.TryAdd(pinNumber, PinValue.Low);


nit: comment above is no longer true

krwq · 2022-11-18T11:10:10Z

src/devices/Pcx857x/tests/Pcx857xTest.cs

@@ -161,6 +161,8 @@ protected override PinValue Read(int pinNumber)
                return PinValue.Low;
            }

+            protected override void Toggle(int pinNumber) => Write(pinNumber, !Read(pinNumber));


can be removed given current virtual implementation

krwq · 2022-11-18T11:12:06Z

src/devices/Seesaw/SeesawGpioDriver.cs

        }

+        /// <inheritdoc/>
+        protected override void Toggle(int pinNumber) => Write(pinNumber, !_pinValues[pinNumber]);


I'm not sure dictionary actually makes it faster for those low level drivers - reading/writing pin is supposedely fastest operation you can make if you think about it. It might be currently faster this way because of OS but in principal that shouldn't be the case

We should measure this later on. Also some drivers do not support reading when mode is output.

krwq · 2022-11-18T11:12:29Z

tools/ArduinoCsCompiler/Hal/ArduinoNativeGpioDriver.cs

@@ -65,6 +65,8 @@ protected override PinValue Read(int pinNumber)
            return _hardwareLevelAccess.ReadPin(pinNumber);
        }

+        protected override void Toggle(int pinNumber) => Write(pinNumber, !Read(pinNumber));


nit: can be removed

krwq

LGTM with couple of nits. feel free to ignore or fix separately

* Adding GpioPin * adjusting based on PR feedback * Adjusting to have natively drivers. * make methods virtual * adding Toggle to System.Device drivers * Board and Mcp23xxx adjustments * adding FT4222, FT232H and Pcx857x * adding Arduino and Seasaw * adjsting file encoding after conflict merge * Adding missing Toggle implementation in Arduino native * Adjusting test for new pattern with GpioPin * adjusting PR feedback * typo in test * fixing nit * fixing test * fixing test

* Add IS31FL3731 LED matrix binding * Update sample * Make program more generic * Add specific bindings * Add Phat 17x7 binding * Add Phat 17x7 binding * Add LED Shim 28x1 driver * Update switch expression * Update TPN * Add README * Add Led Matrix Breakout 11x7 binding * Update README for led matrix breakout * Add Led RGB Matrix 5x5 binding * Add 5x5 RGB Matrix sample * Switch integers to decimals * Fix syntax * Rename type * Update per feedback * Update per feedback * Switch to enums for const data * Switch from enum to static class * Switch to ROS * Fix flipped field * Update brightness * Update TFMs * Adding GpioPin (#1895) * Adding GpioPin * adjusting based on PR feedback * Adjusting to have natively drivers. * make methods virtual * adding Toggle to System.Device drivers * Board and Mcp23xxx adjustments * adding FT4222, FT232H and Pcx857x * adding Arduino and Seasaw * adjsting file encoding after conflict merge * Adding missing Toggle implementation in Arduino native * Adjusting test for new pattern with GpioPin * adjusting PR feedback * typo in test * fixing nit * fixing test * fixing test * Bump Microsoft.CodeAnalysis.CSharp.Scripting in /tools/DevicesApiTester (#1981) Bumps [Microsoft.CodeAnalysis.CSharp.Scripting](https://github.com/dotnet/roslyn) from 4.3.1 to 4.4.0. - [Release notes](https://github.com/dotnet/roslyn/releases) - [Changelog](https://github.com/dotnet/roslyn/blob/main/docs/Breaking%20API%20Changes.md) - [Commits](https://github.com/dotnet/roslyn/commits/Visual-Studio-2019-Version-16.0-Preview-4.4) --- updated-dependencies: - dependency-name: Microsoft.CodeAnalysis.CSharp.Scripting dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump Microsoft.Net.Compilers.Toolset in /tools/DevicesApiTester (#1980) Bumps [Microsoft.Net.Compilers.Toolset](https://github.com/dotnet/roslyn) from 4.3.1 to 4.4.0. - [Release notes](https://github.com/dotnet/roslyn/releases) - [Changelog](https://github.com/dotnet/roslyn/blob/main/docs/Breaking%20API%20Changes.md) - [Commits](https://github.com/dotnet/roslyn/commits/Visual-Studio-2019-Version-16.0-Preview-4.4) --- updated-dependencies: - dependency-name: Microsoft.Net.Compilers.Toolset dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Upgrade to .NET 7 GA (#1988) * Upgrade to .NET 7 GA * Update global.json Co-authored-by: Jose Perez Rodriguez <joperezr@microsoft.com> * Update blinking * Add 'Port of' comments * Update sample * Update sample * Update comment * Update per feedback * Add 'Port of' comment * Move samples to directories * Update README * Change exception type Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Laurent Ellerbach <laurelle@microsoft.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Jose Perez Rodriguez <joperezr@microsoft.com>

Ellerbach added 2 commits July 23, 2022 17:44

Adding GpioPin

16a35c4

Merge branch 'main' of https://github.com/dotnet/iot into ad-gpio-pin

37de849

pgrawehr reviewed Jul 23, 2022

View reviewed changes

src/System.Device.Gpio/System/Device/Gpio/GpioController.cs Show resolved Hide resolved

raffaeler reviewed Jul 23, 2022

View reviewed changes

src/System.Device.Gpio/System/Device/Gpio/GpioController.cs Outdated Show resolved Hide resolved

krwq reviewed Jul 28, 2022

View reviewed changes