Adding virtual GpioController and virtual GpioPin #2180

Ellerbach · 2023-11-25T11:55:20Z

Adding virtual GpioController and virtual GpioPin. This allows to recreate a GpioController from different GpioPin created form potentially very different GpioController.

Microsoft Reviewers: Open in CodeFlow

pgrawehr

Good start. I've found some possible pitfalls you might need to look into.

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

src/devices/Gpio/Drivers/VirtualGpioController.cs

src/devices/Gpio/README.md

joperezr · 2023-11-27T22:34:14Z

Not sure if this proposal was discussed in a meeting I wasn't part of, but I have some questions about the reason why this would be needed. I thought in general we agreed on two models:

Cases were all of the pins for a specific device come from the same controller: These cases should just be solved by having the device take in a gpioController in one of its constructors, that has access to all of those pins.
Cases were pins used by a device come from different controlelrs: These cases should just be sulved by having the device also have a constructor that takes in individual GpioPins.

The whole reason why we added GpioPin was so that we wouldn't need something like this (which seems to be an approach that uses an emmulation to mix the two scenarios above) since devices can just take individual pins, and we also agreed all devices would get constructors that took Gpio Pins in favor of this. Am I missing something here?

Ellerbach · 2023-11-28T09:36:35Z

@joperezr yes, we discussed in one of the call. As we have a lot of bindings to adjust, idea is to have a transparent way to use a virtual GPIO controller! So having a virtual controller will solve this issue of having to add all those new constructors and adjust all the internal codes to support GpioPin. Because, it's not just adding ability to have a GpioPin in the constructor, it's about adjusting all the logic in all the bondings to replace GpioController.Write, Read, etc by the GpioPin.Write, Read, etc.

…-gpio-controller

krwq · 2024-04-18T15:34:43Z

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

@@ -416,7 +416,7 @@ protected virtual void Dispose(bool disposing)
    }

    /// <inheritdoc/>
-    public void Dispose()
+    public virtual void Dispose()


this shouldn't be virtual (only the one with bool flag should)

krwq · 2024-04-18T15:40:26Z

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

@@ -131,7 +131,7 @@ protected virtual void OpenPinCore(int pinNumber)
    /// </summary>
    /// <param name="pinNumber">The pin number in the controller's numbering scheme.</param>
    /// <param name="mode">The mode to be set.</param>
-    public GpioPin OpenPin(int pinNumber, PinMode mode)
+    public virtual GpioPin OpenPin(int pinNumber, PinMode mode)


[Triage] Let's try to reduce number of virtual OpenPin methods, if possible only OpenPinCore

I've been looking at overwriting only the OpenPinCore but the issue is that the core logic with adding the pin into the lopen list and adding the gpio pin as well is done in the OpenPin. And that is what I want to override. And as the function returns a pin that won't be store properly otherwise, I don't really have a nice option for that!

Fine for me. I mean if there's an use if this is virtual, I don't bother changing it.

I think I found a good compromise and reviewed a bit the internal elements. So, I only made one of the OpenPin virtual, removed the others, reused some of the logic from the GpioPin dictionary. So it's cleaner.

krwq · 2024-04-18T15:45:32Z

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

@@ -8,13 +8,40 @@ namespace System.Device.Gpio
    /// </summary>
    public class GpioPin
    {
-        private readonly int _pinNumber;
-        private readonly GpioDriver _driver;
+        private readonly int _externalPinNumber;


Can you add a comment explaining how's this different to DriverPinNumber and why we need this here and not just i.e. on the virtual pin?

pgrawehr

Some comments and questions still.

Some unit tests would be nice.

pgrawehr · 2024-06-13T16:13:31Z

src/devices/Gpio/Drivers/VirtualGpioController.cs

+        /// <summary>
+        /// Closes an open pin.
+        /// If allowed by the driver, the state of the pin is not changed.
+        /// </summary>
+        /// <param name="pinNumber">The pin number in the controller's numbering scheme.</param>
+        public override void ClosePin(int pinNumber)


Implementation or description is missleading, as you don't actually close the underlying pin.

pgrawehr · 2024-06-13T16:14:40Z

src/devices/Gpio/Drivers/VirtualGpioController.cs

+        /// <summary>
+        /// Disposes this instance and closes all open pins associated with this controller.
+        /// </summary>
+        /// <param name="disposing">True to dispose all instances, false to dispose only unmanaged resources</param>
+        protected override void Dispose(bool disposing)


Update documentation to better match the behavior

src/devices/Gpio/Drivers/VirtualGpioController.cs

pgrawehr · 2024-06-13T16:22:27Z

src/devices/Gpio/Drivers/VirtualGpioController.cs

+        /// <inheritdoc/>
+        public override WaitForEventResult WaitForEvent(int pinNumber, PinEventTypes eventTypes, CancellationToken cancellationToken)
+        {
+            throw new NotImplementedException();


This should be straight-forward, shouldn't it?

no because I don't have access to the Driver neither. If we want to expose it, then yes, it can indeed be done

Oh, got it. Would need the Driver property to be at least protected.

I've been playing with this as well and it's not that straight forward. I may continue a bit but it's not easy and requires to create a virtual driver as well, things like this.

src/devices/Gpio/Drivers/VirtualGpioPin.cs

raffaeler · 2024-06-13T19:22:09Z

src/devices/Gpio/Drivers/VirtualGpioController.cs

+        /// <param name="pinNumber">The pin number.</param>
+        /// <param name="gpioPin">The <see cref="GpioPin"/> to add.</param>
+        public void Add(int pinNumber, GpioPin gpioPin) => _pins.TryAdd(pinNumber, gpioPin);
+


Considering the complex numbering of the Beaglebone black discussed recently, I would consider two alternatives to the use of integer for the virtual pin numbering.

Option 1: A string

Option 2: An enum. This could be done by making the controller generic and using Enum as constraint for the generic type (which is new in C# 7.3)

In both cases this could help identifying the pins with any friendly name, which is in use on any board.
Going beyond, you can also define a json map loaded in the project with all the pins getting customized names, similar to this example.

I love the idea of virtualizing pins, but IMO it should/can be more versatile.

I've checked all this and if we want to do it we have to keep in mind few things if we want to have done all to the bottom:

changing int to string will have an impact on performances as comparing strings and int is slower

adjustments can be done and it will require to change the GpioPin itself to store the string instead of the pin number otherwise it will complicated much more everything. And indeed will complicated the internal manipulation

Alternative:

if we don't want to use string at the lower level, the possibility is to generate a random, positive, non already existing pin number from the list

For this implementation, I will then have to add a new constructor, a new list and new methods using the string on top of the int (getting the int will also be possible anyway)

With regards to the "alternative" solution, one possibility is to provide the GpioPin an optional IDictionary<string, int> to decode the string into the number that makes sense for the board.
As it is optional, if you want pure performance, you can just skip it.
If you want to use it, the GpioPin can use the overload that takes a string and try resolving the pin through the dictionary.

Does this make sense to you?

The core issue with this is that it can then be used only when in your own code you are calling this. As all bindings takes a GpioController and pin numbers.
So the solution with alternative is the one compatible with everything in that case. It will indeed require a dictionary and few more functions but ultimately, what is going to be manipulated are just in numbers. So you'll still be able to keep the string in the virtual controller but under the hook and for compatibility reasons, it's going to be pure uint.
As an example on how it can looks like:

// Note that we can add couple of other constructors var virtualGpio = new VirtualGpioController(); virtualGpio.Add("A42", gpioPinA); virtualGpio.Add("B26", gpioPinB); var myButtonPin = virtualGpio.GetPinNumber("A42"); var button = New Button(buttonPin: myButtonPin, gpio = virtualGpio);

In all cases, I would make those adjustments in a separate PR.

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

joperezr · 2024-06-14T17:28:56Z

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

+        /// </summary>
+        /// <param name="pinNumber">The GPIO pin number.</param>
+        /// <param name="driver">The GpioDriver.</param>
+        protected internal GpioPin(int pinNumber, GpioDriver driver)


These two APIs should also be Experimental.

Adding virtual gpio controller basic

2531b88

Ellerbach requested review from pgrawehr and joperezr November 25, 2023 11:55

add basic eventing

fcae355

pgrawehr reviewed Nov 25, 2023

View reviewed changes

joperezr reviewed Nov 27, 2023

View reviewed changes

src/devices/Gpio/README.md Outdated Show resolved Hide resolved

Ellerbach added 4 commits December 11, 2023 10:13

Adjusting GpioPin internals to take a GpioPin

2c2dda0

Merge branch 'main' of https://github.com/dotnet/iot into add-virtual…

1662256

…-gpio-controller

Adjusting based on discussions

ac5ac45

Adjusting based on PR feedback

14b991b

Ellerbach marked this pull request as ready for review April 8, 2024 09:11

krwq reviewed Apr 18, 2024

View reviewed changes

adjusting logic and removing some complexity

652895a

pgrawehr reviewed Jun 13, 2024

View reviewed changes

raffaeler reviewed Jun 13, 2024

View reviewed changes

joperezr reviewed Jun 14, 2024

View reviewed changes

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

joperezr reviewed Jun 14, 2024

View reviewed changes

FredMurphy mentioned this pull request Jun 14, 2024

Plan to remove PinNumberingScheme.Board #2326

Open

joperezr added this to the v4.0.0 milestone Jun 20, 2024

Ellerbach added 2 commits July 1, 2024 16:48

adjusting based on PR feedback

ee0ee9c

adjusting name

87033c7

krwq added the Priority:0 Work that we can't release without label Nov 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding virtual GpioController and virtual GpioPin #2180

Adding virtual GpioController and virtual GpioPin #2180

Ellerbach commented Nov 25, 2023 •

edited by ghost

Loading

pgrawehr left a comment

joperezr commented Nov 27, 2023

Ellerbach commented Nov 28, 2023

krwq Apr 18, 2024

krwq Apr 18, 2024

Ellerbach Jun 10, 2024

pgrawehr Jun 11, 2024

Ellerbach Jun 13, 2024

krwq Apr 18, 2024

pgrawehr left a comment

pgrawehr Jun 13, 2024

pgrawehr Jun 13, 2024

pgrawehr Jun 13, 2024

Ellerbach Jul 1, 2024

pgrawehr Jul 1, 2024

Ellerbach Jul 8, 2024

raffaeler Jun 13, 2024

Ellerbach Jul 1, 2024

raffaeler Jul 2, 2024

Ellerbach Jul 4, 2024

joperezr Jun 14, 2024

Adding virtual GpioController and virtual GpioPin #2180

Are you sure you want to change the base?

Adding virtual GpioController and virtual GpioPin #2180

Conversation

Ellerbach commented Nov 25, 2023 • edited by ghost Loading

Microsoft Reviewers: Open in CodeFlow

pgrawehr left a comment

Choose a reason for hiding this comment

joperezr commented Nov 27, 2023

Ellerbach commented Nov 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgrawehr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ellerbach commented Nov 25, 2023 •

edited by ghost

Loading