Add a default constructor on bindings to take GpioController

[Ellerbach]

When you want to use other GpioController than the default driver one, you can't in any of the binding devices.

With introduction of FT4222 we need to be able to pass a specific GpioController to any class using a GPIO Controller.

Ideal solution would be to add or modify all devices constructor to add the possibility to add your own GpioController if you want. We will need also to add a flag to dispose automatically or not the GpioController at the end.

See usage of the FT4222 GpioController

So for example taking the DHT sensor, we would need to change the constructor and adjust the code from:

        /// <summary>
        /// Create a DHT sensor
        /// </summary>
        /// <param name="pin">The pin number (GPIO number)</param>
        /// <param name="pinNumberingScheme">The GPIO pin numbering scheme</param>
        public DhtBase(int pin, PinNumberingScheme pinNumberingScheme = PinNumberingScheme.Logical)
        {
            _protocol = CommunicationProtocol.OneWire;
            _controller = new GpioController(pinNumberingScheme);
            _pin = pin;

            _controller.OpenPin(_pin);
            // delay 1s to make sure DHT stable
            Thread.Sleep(1000);
        }

to:

private bool _autoDispose;

        /// <summary>
        /// Create a DHT sensor
        /// </summary>
        /// <param name="pin">The pin number (GPIO number)</param>
        /// <param name="pinNumberingScheme">The GPIO pin numbering scheme</param>
        /// <param name="gpioController">A GPIO Controller if not using the default driver one</param>
        /// <param name="autoDispose">True to auto dispose the GPIO Controller when dispose</param>
        public DhtBase(int pin, PinNumberingScheme pinNumberingScheme = PinNumberingScheme.Logical, GpioController gpioController = null, bool autoDispose = true)
        {
            _protocol = CommunicationProtocol.OneWire;
            _autoDispose = autoDispose;
            if (gpioController == null)
            { _controller = new GpioController(pinNumberingScheme); }
            else
            {
                _controller = gpioController;
            }
            _pin = pin;

            _controller.OpenPin(_pin);
            // delay 1s to make sure DHT stable
            Thread.Sleep(1000);
        }

        /// <inheritdoc/>
        public void Dispose()
        {
            if (_autoDispose)
            {
                _controller?.Dispose();
            }
            _i2cDevice?.Dispose();
        }

And also adjust the constructor for the DHTxx:

        public Dht22(int pin, PinNumberingScheme pinNumberingScheme = PinNumberingScheme.Logical, GpioController gpioController = null, bool autoDispose = true)
            : base(pin, pinNumberingScheme, gpioController, autoDispose)
        {
        }

That would be needed for all the devices using a GpioController.

And DHT may not be a device for which the FT4222 would work. But it's to give an example.

Other alternative option: add the FT4222 into System.Device.Gpio. And on the driver side, check if it does exist and pick it if no other driver.

[joperezr]

Actually there are many device bindings today that take in a GpioController in their constructor for this exact reason. The idea is that if you are not using the default controller because you are using a Gpio expander on your board, you can create a custom GpioController using the custom GpioDriver, and then pass that in to many of our device bindings. Just by looking at ILSpy, here is a list of some of the devices that have a constructor that take in a GpioController:

If you find places where we are internally using a GpioController, and we aren't optionally taking one on the constructor, then we should definitely add it to that binding.

[pgrawehr]

@joperezr Note that, while most bindings do take an external GpioController, many are missing the optional shouldDispose argument, which results in the controller being disposed when the binding is disposed. This is often not desirable, especially when using a GpioExpander or an Ft4222 board.

[joperezr]

In that case we should definitely log an issue for that as we would most likely want to add that. If I remember correctly, most PRs where I've seen this is because we don't really expose an option in the constructor, but we just check locally that if the controller was passed in and not instantiated from the binding, then we by default won't dispose it. I may be wrong here, but this is what I've suggested in some PRs at least

[pgrawehr]

I would be fine with that, but it's clearly not the case for some (many?) bindings. I.e. https://github.com/dotnet/iot/blob/master/src/devices/Dhtxx/DhtBase.cs.

An alternative would be to have the controllers bound to a certain set of pins (i.e. create a controller only for pin 37), then it could be disposed together with its binding. Not sure we need another ticket here, because that's one of the main topics that's being discussed in #878.

[Ellerbach]

So as I needed for some devices with the FT4222, I did the change for all the devices.

[joperezr]

For the ShouldDispose, I added some feedback in your PR but in general we probably want to default to false, and then just switch it to true in case that there was no controller passed in.

[Ellerbach]

For the ShouldDispose, I added some feedback in your PR but in general we probably want to default to false, and then just switch it to true in case that there was no controller passed in.

@joperezr I've been fixing quite some time ago the feedback you gave on the PR. What are the next steps for this PR?

[joperezr]

You are totally right @Ellerbach so sorry I missed it. I just assigned myself to it and will take a final look and merge this weekend.

[krwq]

I think there are two ways we can go about this issue:

• go with our initial recommendation for GpioController: make it so that you can create multiple GpioControllers on top of FT4222 and as long as they don't use the same pins this should work fine
• we change recommendation so that GpioController should not be disposed and adjust all the bindings so that it's possible (we initially only gave recommendation that it is ok to do so).

My opinion on this has been bouncing back and forth between which of these we should go (since it's not first time this discussion shows up).

Initially when I just started on IoT my intuition told me to use just one GpioController. Then I saw we already designed it differently and got used to the idea and switched that we should use multiple GpioController.

At this point looking at the fact that most of the new people have similar instinct about how GpioController works (there should be just one) - I think we should just go with that and make it so that it's less likely that a new person will make do it right. My vote goes for option number 2. especially since the work of adding this option to bindings is already in progress. I think we should change device guidelines about GpioController. @joperezr ?

[pgrawehr]

My 2ct: I'm also going back and forth here, and part of Board stuff is just about this problem. When we have multiple GPIO Controllers, but all connected to the same board instance, the pin assignment can be handled there (i.e validated that no two controllers share the same pin). Since we also have pins that are not controlled by GpioController instances (i.e. PWM, I2C), but should not be shared with a GpioController either, just requiring only one GpioController instance is not sufficient.

I would therefore suggest to allow multiple controllers and later, when they're connected to the Board (maybe check https://github.com/dotnet/iot/pull/1044/commits/af25cf898a31ebcffcd8b0db6f99f24c9e871be0#diff-e8b04e879818169e5ed137bc2bb6a620 and https://github.com/dotnet/iot/pull/1044/commits/fc72eee90576b75256f2fc1bffddf43fc0a2d7fd#diff-db1bb3b14a65ed4734e61f6e4e551703 to get the big picture of what I suggest there) it will be guaranteed that each controller (including those for PWM, I2C, SPI) handles its pins exclusively. But, and there I prefer option 2, the GpioControllers should always be provided and not auto-generated inside a binding, because that makes usage consistent for all controllers and all applications (i.e. the user should just be able to replace the RaspberryPi3Controller (or -board) with an ArduinoController to make the binding work on an Arduino, and not need to consider that this requires an extra parameter etc. And it makes it less likely that ever again we have bindings where externally providing the controller was just plain forgotten.

[pgrawehr]

@joperezr I feel that unfortunatelly, we need to reopen this. I've just seen a few cases like (from DhtBase.cs):

public DhtBase(int pin, PinNumberingScheme pinNumberingScheme = PinNumberingScheme.Logical, GpioController gpioController = null, bool shouldDispose = true)
        {
            _protocol = CommunicationProtocol.OneWire;
            _shouldDispose = gpioController == null ? true : shouldDispose;
            _controller = gpioController ?? new GpioController(pinNumberingScheme);
            _shouldDispose = true;
            _pin = pin;

            _controller.OpenPin(_pin); // Opens the Pin for itself!
            // delay 1s to make sure DHT stable
            Thread.Sleep(1000);
        }

with a Dispose of

public void Dispose()
        {
            if (_shouldDispose)
            {
                _controller?.Dispose();
            }
            _i2cDevice?.Dispose();
        }

This should rather be

public void Dispose()
        {
            if (_shouldDispose)
            {
                _controller?.Dispose();
            }
            else
            {
                _controller.ClosePin(_pin);
            }

            _i2cDevice?.Dispose();
        }

because the pin we allocated for this binding needs to be freed, as we "own" it.

I've seen this in at least two bindings (Dhtxx and Mcp2xxx).

[joperezr]

@pgrawehr I would suggest instead to create a new issue with the ones you find and address those specially as the main theme of this issue has been fixed by the PR @Ellerbach added. Any fallbacks we find can be logged as new issues.