Create New GpioPin and IGpioPin

[shaggygi]

It would be nice to have the ability to provide actual GPIO pin objects to bindings instead of pin numbers. Currently, we usually have to provide a GPIO controller along with pin numbers or just the pin numbers and the binding will create the controller and configure the pins. This is a limitation as the pins are usually all on the same controller where I might want to mix it up between dev board, bindings, etc.

Examples

// Can remove the GpioDriver in constructor as each pins knows what controller/driver it uses.
public Mcp23xxx(int deviceAddress, IGpioPin? reset = null, IGpioPin? interruptA = null, IGpioPin? interruptB = null)

// public class GpioPort(params IGpioPin[] bitIndexes)
GpioPort port = new GpioPort(IGpioPin one, IGpioPin two, IGpioPin three, IGpioPin four);

Referencing #125 (comment)
Also referencing #204 as it appears we would want something similar to PwmPins.

[shaggygi]

/cc @krwq

[krwq]

@shaggygi I actually thought about it a bit and we don't need to change existing design. We will need to add new types of controllers which will map to other controllers.

Example would be something like VirtualController where you can:

• map IGpioController to pin address space
• you will need to have a way of getting virtual address given sub-controller and address in sub-controller address space
• we will need to make sure all devices have a way of providing controller
• settings should be specified up-front meaning address space cannot be modified after making it (this will allow us to make optimizations so that VirtualController with sub VirtualControllers can flatten the graph which technically will make it always a linear mapping to physical controllers except when you ask for virtual addresses of virtual sub-controllers)

but if we are ever going to provide a pin abstraction I think this should be a simple struct of IGpioController and a number

[UncleSamSwiss]

Is there any decision/progress on this proposal?

In our application code handling multiple GPIOs in different (unrelated) classes, we see a pattern evolving which would be much easier to implement with GpioPin and IGpioPin. Also, this API could probably help #347 as you could dispose or not dispose the IGpioPin object depending on your needs.

IMHO the current API is a bit "C-like"; in .NET we are used to having objects for everything. The only place where I see an advantage for the current API (OpenPin(...), RegisterCallbackForPinValueChangedEvent(...), ...) is the Read(Span<PinValuePair> pinValuePairs) method which allows to have multiple pins read at the same time; but the GpioPort proposed above would solve this as well.

[joperezr]

@UncleSamSwiss this was actually one of the main topics we discussed when coming up with the original API Proposal, and after weighing the pros and cons we decided that a C-like approach would be better here because of several reasons including the life-cycle of objects and other things. For reference, you can take a look at all three of our API discussions which are linked here. For those reasons, we have pretty much all of our API with the existing shape, and moving it back to exposing a GpioPin would be very costly so there would have to be a big enough reason why we would want to do this. Is there a specific scenario where you think GpioPin is a much better approach such that it would make it feasible to change our API shape?

[UncleSamSwiss]

@joperezr thanks for your explanation. I can understand your reasoning; and of course changing it now would be a huge endeavor.

Just to explain my use case, here is more or less a copy&paste of our code (won't compile):

    public class OurService : IHostedService
    {
        private int buttonPin;

        public async Task StartAsync(CancellationToken cancellationToken) {
            if (!this.gpioService.HasGpio) {
                return;
            }

            var pinNumber = this.config.ButtonPin;
            this.gpioService.Controller.OpenPin(pinNumber, PinMode.InputPullUp);
            this.buttonPin = pinNumber;
            this.gpioService.Controller.RegisterCallbackForPinValueChangedEvent(
                pinNumber, PinEventTypes.Rising | PinEventTypes.Falling, this.HandleButtonInterrupt);
        }

        public async Task StopAsync(CancellationToken cancellationToken) {
            if (this.buttonPin > 0) {
                this.gpio.Controller.UnregisterCallbackForPinValueChangedEvent(this.buttonPin, this.HandleButtonInterrupt);
                this.gpioService.Controller.ClosePin(this.buttonPin);
            }
        }
    }

Note: this.gpioService.Controller is a GpioController.

As you can see in the StartAsync() method, we take the pinNumber and then assign it to the instance variable buttonPin if we were able to open the pin.

In the StopAsync() method we check this integer and then call ClosePin() on the GpioController if needed.

This would be much nicer (i.e. more object oriented) if we had an IGpioPin : IDisposable:

    public class OurService : IHostedService
    {
        private IGpioPin buttonPin;

        public async Task StartAsync(CancellationToken cancellationToken) {
            if (!this.gpioService.HasGpio) {
                return;
            }

            var pinNumber = this.config.ButtonPin;
            this.buttonPin = this.gpioService.Controller.OpenGpioPin(pinNumber, PinMode.InputPullUp);
            this.buttonPin.RegisterCallbackForPinValueChangedEvent(
                PinEventTypes.Rising | PinEventTypes.Falling, this.HandleButtonInterrupt);
        }

        public async Task StopAsync(CancellationToken cancellationToken) {
            this.buttonPin?.Dispose();
        }
    }

Looking at the given solution, it would also be possible to provide both APIs, but I guess to the inexperienced developer it would be confusing to have two ways to accomplish the same.

[joperezr]

In your concrete example, yes you have to do two calls instead of one, that said, without knowing a bit more about your service I believe that one other thing you could/should do, is to dispose the controller itself, which will do the cleanup for you. Because the Controllers are disposable, then having a pin object might be dangerous since you could have the case where a pin's controller has been disposed but the pin object itself is not yet, so that might cause weird life cycle problems.

BTW, thanks for bringing your scenario up and starting this interesting discussion, I believe that these types of discussions will make our library much better for library and app developers!

[asasine]

I started using this library expecting a much more IGpioPin approach. I was surprised with how C-like it was and ended up writing my own simple abstraction for LEDs and buttons that takes in a GpioController in the constructor and opens/closes the pin as appropriate. It has simple methods to read values (for a button/input) and write values (for an LED/output). It's not perfect but I found it easier to write logic around objects like this.

[krwq]

We've talked about this today and we need to give this more though before we agree to have this. The specific scenarios which need more considerations:

• APIs which take multiple pins and might come from different controllers (it's currently doable but it's a bit of a hassle)
• LED Matrix scenario where GpioPin might be implemented faster than with GpioController (by bypassing GpioController and using direct memory access) - currently LED Matrix works only on Raspberry PI 3, with this it might be extendable to other devices
• should this be an interface, abstract class or struct?
• what other APIs need to change?
• should we adjust all APIs to take GpioPin or as is right now (GpioController + int-s)? if we stick with GpioController what about scenarios which take multiple pins?
• should we preserve old APIs and have new ones or replace them with GpioPin?
• which way of accessing pins do we recommend? we would have two ways of doing things now - would the benefits outweigh confusion of having both APIs? should we deprecate old APIs or treat it as different abstraction layer (kinda like I2cBus vs I2cDevice)?
• what specific APIs do we want to expose on GpioPin? Just set/read value? Do we want eventing?

We need some design doc which covers answers for these questions/thoughts

[pgrawehr]

One possibility I considered in my board concept was to create GpioController(s) that act only on a subset of the available pins. This doesn't solve the performance problem, but it may prevent some errors like accidentally operating on a pin that belongs to something completelly unrelated to the current binding.

Would it be possible to open the wiki feature on the repo, or do we have another way of collaboratelly creating design documents?

[Ellerbach]

An interesting implementation is the one we can find in nanoFrmaework: https://github.com/nanoframework/lib-System.Device.Gpio/tree/develop/System.Device.Gpio
You can get the GpioPin when you open the pin on the controller.

[krwq]

@pgrawehr for design documents temporarily you can create .md file under i.e. Documentation/design-docs.

[krwq]

@Ellerbach one way to not change current APIs perhaps could be to leave OpenPin/ClosePin and add something like CreatePin although I personally like OpenPin returning GpioPin a lot and am in favor of doing this binary breaking but source compatible change.

[joperezr]

How can it be source compatible? it didn't use to return anything and now we would want to return a pin and you'd have to call write on that, instead of the controller.

[Ellerbach]

How can it be source compatible?

Anything existing will continue to work. No change on the GpioController methods except the OpenPin which does return a GpioPin.
You keep the GpioPin if you're interested or not. That's your choice. If you have something advance to do or faster read/write operations, you just keep the GpioPin, otherwise, you just don't get it.

[krwq]

yes, how I see that is GpioPin would automatically call _gpioController.ClosePin on Dispose but if you call ClosePin as you used to there would be no change in your code.

One thing to consider to get more benefit of potential GpioPin would be to allow option for pin to not be managed by controller after openning in order to reduce overhead of having the dictionary

[josesimoes]

I upvote for this improvement too. Having GpioPin is so much more convenient and it feels more .NETish.
Otherwise you end up to always have to go through the controller to do anything on a pin.

As @Ellerbach suggested, please take a look at the implementation we have on .NET nanoFramework, here.
You can look at some sample code using it here.
Note the difference on "Gpio+Events IoT Style" and the proposed way in "Gpio+Events". The latter is much cleaner and .NETish.

There are a few other improvements that we have there that can be considered like DebounceTimeout, Toggle and most important the ValueChanged right on the pin instead of doing through the controller.

As for "freeing" the Gpio from the controller as @krwq suggested, that's something to consider too.
It removes the burden of having to keep the dictionary with the pins and go through it on every operation.
The simplest way would be to move all methods acting on the pin to the new GpioPin (except Open and Close, of course).

[krwq]

ref: #124

[krwq]

[Triage] Closing this in favor of: #1671