Unseal GpioController by pgrawehr · Pull Request #1254 · dotnet/iot

pgrawehr · 2020-11-04T16:43:28Z

As discussed in several tickets and PRs already: this unseals the GpioController and changes its public API to virtual methods.

In addition, I fixed the PinNumberingScheme.Board use case. Now all methods do a proper translation to the logical scheme the driver is using. The bug #974 is still open, though. This requires further thought and possibly an interface change.

Unit testing for all methods provided as well (using a bit of a hacky approach to mocking, but there's no other way given that all the methods of the GpioDriver class are internal protected). At least the MockableGpioDriver class is reusable.

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

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

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

krwq · 2020-11-06T13:23:40Z

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

-        private static GpioDriver GetBestDriverForBoard()
+        /// <returns>An instance of a GpioDriver that best matches the current hardware</returns>
+        /// <exception cref="PlatformNotSupportedException">No matching driver could be found</exception>
+        protected static GpioDriver GetBestDriverForBoard()


I think perhaps this should be just GpioDriver.Create

cc: @joperezr

How would we know which driver to create?

pgrawehr · 2020-11-08T20:09:44Z

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

-
+        /// <returns>An instance of a GpioDriver that best matches the current hardware</returns>
+        /// <exception cref="PlatformNotSupportedException">No matching driver could be found</exception>
        private static GpioDriver GetBestDriverForBoard()


Having this private prevents adding new APIs, but requires copy-pasting the implementation to the Board infrastructure. That's not so big of a deal for now, except that it requires a few ugly try-catch attempts in testing which driver is the right one.
I think we need the identification logic to become public somehow (or at least provide each driver with a IsCompatible check)

Let's refrain from fixing this in this PR and perhaps suggest GpioDriver.Create in a separate PR/issue

I agree to do that later. We should just not forget about it.

krwq · 2020-11-10T12:37:15Z

@pgrawehr can you squash and rebase to take nullability changes?

Ellerbach

Looks very good to me. thanks!

Ellerbach · 2020-11-10T12:43:39Z

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

+        }
+
+        // TODO: This is still broken. See #974
+        ////[Fact]


Only 2 // should be enougth :-) Not blocking, and thanks for adding the comment that this code even if dead should stay here

If only using two // you get StyleCop errors, because then the comment must be immediatelly followed by code. And a /// comment on the next function (if any) must not, on the other hand, be preceeded by a // comment. So //// is the only way to properly comment out an entire function.

/*perhaps there are other ways*/

(I'm fine with quadra comments though)

pgrawehr · 2020-11-10T14:21:28Z

@pgrawehr can you squash and rebase to take nullability changes?

Yes, will do.

Need to implement a derived class later

All public methods must translate the NumberingScheme to identify the correct pin that is meant.

These run purely in software.

Simplifies method implementation

pgrawehr · 2020-11-10T20:06:16Z

/azp run

azure-pipelines · 2020-11-10T20:06:22Z

Commenter does not have sufficient privileges for PR 1254 in repo dotnet/iot

pgrawehr · 2020-11-10T20:07:02Z

@joperezr Can you please restart the build here?

joperezr · 2020-11-11T01:17:16Z

/azp run

azure-pipelines · 2020-11-11T01:17:26Z

Azure Pipelines successfully started running 1 pipeline(s).

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

colombod · 2020-11-12T19:50:35Z

This pr looks very good and promising

colombod · 2020-11-12T19:50:59Z

merging soon?

pgrawehr · 2020-11-12T19:52:32Z

@colombod Yes, this is just awaiting another quick review from joperezr, because he's most familiar with these pieces.

colombod · 2020-11-12T22:20:53Z

Perfect!

krwq · 2020-11-16T11:14:01Z

@joperezr have time to review?

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

joperezr · 2020-11-16T18:46:40Z

        _driver.SetPinMode(logicalPinNumber, mode);

Is there a reason why we added the OpenPinCore and ClosePinCore (where we are doing the validation on the non-virtual ones) in some cases but not added a SetPinModeCore as well that did the validation above? I'm basically just wondering if we should've just made OpenPin and ClosePin virtual as well and remove the Core ones.

Refers to: src/System.Device.Gpio/System/Device/Gpio/GpioController.cs:161 in 41b1c1a. [](commit_id = 41b1c1a, deletion_comment = False)

joperezr · 2020-11-16T18:48:43Z

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

        /// <param name="mode">The mode to check.</param>
        /// <returns>The status if the pin supports the mode.</returns>
-        public bool IsPinModeSupported(int pinNumber, PinMode mode)
+        public virtual bool IsPinModeSupported(int pinNumber, PinMode mode)


The implementation of this is using _openPins list which is private (instead of protected) as well as _driver, which is also private. Should those two fields be protected instead so that subclasses can use them in their implementation?

I also think that we need to have access to the collection maybe via protected methods instead of giving access to the collection itself? But is we can override things like the pincount property and ispinopen there is also the need to have a way to check the collection

joperezr · 2020-11-16T18:51:45Z

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

        public void Dispose()
        {
            Dispose(true);
+            GC.SuppressFinalize(this);


NIT: I know this is the standard implementation of the Dispose pattern, but the class has no finalizer so this will not have any effect. I would just remove the line.

joperezr · 2020-11-16T18:54:05Z

Left some small comments/questions but this is mostly ready to go as soon as those points are finalized.

pgrawehr · 2020-11-16T19:51:38Z

        _driver.SetPinMode(logicalPinNumber, mode);
Is there a reason why we added the OpenPinCore and ClosePinCore (where we are doing the validation on the non-virtual ones) in some cases but not added a SetPinModeCore as well that did the validation above? I'm basically just wondering if we should've just made OpenPin and ClosePin virtual as well and remove the Core ones.

Refers to: src/System.Device.Gpio/System/Device/Gpio/GpioController.cs:161 in 41b1c1a. [](commit_id = 41b1c1a, deletion_comment = False)

Except for the initial validation that the pin is open, the other methods don't do anything except calling into the driver, so adding the Core variant seems a bit to much, but I could life with that as well.

The implementation of this is using _openPins list which is private (instead of protected) as well as _driver, which is also private. Should those two fields be protected instead so that subclasses can use them in their implementation?

I think this is not necessary, since the (base) implementation of IsPinOpen() is always accessible and returns the information you need. Making the _openPins list protected would allow derived classes to tamper with it. And IsPinModeSupported() might on some hardware even work without the pin being open.

The _driver member can^t be made protected, since a derived class can't do anything with it. There are no public members on GpioDriver. If required, a derived class would keep a reference to its special driver instead. (Note: I find it a bit inconsistent that it is not possible to overwrite a protected internal method with protected internal outside the declaring assembly. Only protected is allowed)

joperezr · 2020-11-17T04:35:20Z

Left one last small comment, but that and the finalizer are the only remaining things to clear out.

There's no finalizer present

pgrawehr · 2020-11-17T06:13:35Z

@joperezr : Removed the finalizer line, but which other comment do you mean?

joperezr

Sorry forgot to click send 😀

joperezr · 2020-11-17T04:32:33Z

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

        /// <param name="pinNumber">The pin number in the controller's numbering scheme.</param>
        /// <returns>The status if the pin is open or closed.</returns>
-        public bool IsPinOpen(int pinNumber)
+        public virtual bool IsPinOpen(int pinNumber)


What is the scenario then for making this one virtual? If pin open/closed status is going to stay in base, kind of feels like we should keep this method not virtual

I guess either way is ok.

I would rather keep it non-overridable for now if we don't see a good reason to do it, and in the future change it if we think it is necessary. That would be the non-breaking way to do it. I think if we allow people to override this, they will 100% of the time either call base implementation only, or try to do something different and get it wrong.

colombod · 2020-11-17T11:10:31Z

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

            }

+            int logicalPinNumber = GetLogicalPinNumber(pinNumber);
            return _driver.GetPinMode(logicalPinNumber);


Maybe all this drive access can be exposed via protected methods like *FromDriver, eg GetPinModeFromDriver, OpenPinFromDriver or at least give access to the driver instance via a protected property?

That adds a lot of complexity to the interface. Why would you need that?

Yeah, I agree that those would just add complexity and don't see a good use case for them.

colombod · 2020-11-17T11:15:47Z

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

    /// Represents a general-purpose I/O (GPIO) controller.
    /// </summary>
-    public sealed class GpioController : IDisposable
+    public class GpioController : IDisposable


I don't think that this class can satisfy the scenario that I am using in the pi-top libraries. I need a controller that is just a proxy to a real controller. With this implementation anything conforming with the controller interface will have to call base class constructors that will try to create a drive. This part of the class contracts seems a little bit too restrictive.

can you pass null driver and override all methods?

sounds little bit aggressive if null is allowed then then internal calls should be something like bool TryOpenPin(), that should wrap the driver access. Such protected methods will then potentially address the previous questions about the driver.

I like your suggestion, if we can then protected the access to the driver api via a Try* set of methods that handles the null driver that would be an easier pattern for implementors to adopt.

@colombod Can you elaborate a bit more on your use case?

It is possible to pass null as the driver, or a DummyDriver() [will be provided in one of my next PRs]. All methods accessing the _driver member are virtual, so if you override all of them (without calling the base implementation, of course), you should be fine.

in the pi-top libraries we use a gpio controller factory, this returns a wrapper that tracks it's onw pin open/close logic and also events subscriptions. All work si delegated to the concrete controller.
on dispose such controller "projections" dispose all things that where opened via the proxy. That controller will require to have no driver and track the pins open requests. If passing null is safe in this PR then is good for me. And will be able to adopt this new class and finally get rid of the interfaces we introduced. This is the shared controller class

Yea, I think this should all fit now. You do not need any of your intermediate classes. Even SharedGpioController doesn't seem to do anything more than what is aleady implemented.

colombod · 2020-11-17T11:41:28Z

Left one last small comment, but that and the finalizer are the only remaining things to clear out.

I have some questions about constructor requirements too

* Unseal GpioController Need to implement a derived class later * Fix usage of NumberingScheme All public methods must translate the NumberingScheme to identify the correct pin that is meant. * Add unit tests for GpioController These run purely in software. * Use default NumberingScheme everywhere in Controller Simplifies method implementation * Add missing license header * Remove unnecessary cleanup call There's no finalizer present * Ensure a null driver doesn't cause problems in Dispose * IsPinOpen doesn't need to be virtual

Ellerbach added the area-System.Device.Gpio Contains types for using general-purpose I/O (GPIO) pins label Nov 5, 2020

krwq reviewed Nov 6, 2020

View reviewed changes

src/System.Device.Gpio.Tests/GpioControllerSoftwareTests.cs Outdated Show resolved Hide resolved

krwq reviewed Nov 6, 2020

View reviewed changes

src/System.Device.Gpio.Tests/PinValueTests.cs Show resolved Hide resolved

krwq reviewed Nov 6, 2020

View reviewed changes

src/System.Device.Gpio.Tests/PinValueTests.cs Show resolved Hide resolved

krwq reviewed Nov 6, 2020

View reviewed changes

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

krwq reviewed Nov 6, 2020

View reviewed changes

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

krwq reviewed Nov 6, 2020

View reviewed changes

pgrawehr force-pushed the UnsealGpioController branch 2 times, most recently from c9dfee5 to aa00c42 Compare November 8, 2020 19:31

pgrawehr commented Nov 8, 2020

View reviewed changes

runfoapp bot mentioned this pull request Nov 9, 2020

OSX deprovision jaredpar/runfo#41

Closed

Ellerbach approved these changes Nov 10, 2020

View reviewed changes

pgrawehr added 4 commits November 10, 2020 17:22

Unseal GpioController

1fbf5e5

Need to implement a derived class later

Fix usage of NumberingScheme

eabcd1a

All public methods must translate the NumberingScheme to identify the correct pin that is meant.

Add unit tests for GpioController

b02a63d

These run purely in software.

Use default NumberingScheme everywhere in Controller

4a58f81

Simplifies method implementation

pgrawehr force-pushed the UnsealGpioController branch from aa00c42 to 4a58f81 Compare November 10, 2020 16:48

krwq reviewed Nov 12, 2020

View reviewed changes

src/System.Device.Gpio.Tests/MockableGpioDriver.cs Outdated Show resolved Hide resolved

krwq approved these changes Nov 12, 2020

View reviewed changes

krwq requested a review from joperezr November 12, 2020 12:31

Add missing license header

41b1c1a

joperezr mentioned this pull request Nov 12, 2020

adding disposable, shared controller interface and gpiocontroller interface #1167

Closed

joperezr self-assigned this Nov 12, 2020

joperezr added the needs triage 🔍 label Nov 12, 2020

joperezr reviewed Nov 16, 2020

View reviewed changes

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

joperezr reviewed Nov 16, 2020

View reviewed changes

Remove unnecessary cleanup call

f8c3428

There's no finalizer present

joperezr reviewed Nov 17, 2020

View reviewed changes

colombod reviewed Nov 17, 2020

View reviewed changes

pgrawehr added 2 commits November 17, 2020 14:21

Ensure a null driver doesn't cause problems in Dispose

80100a2

IsPinOpen doesn't need to be virtual

a88081a

joperezr approved these changes Nov 19, 2020

View reviewed changes

joperezr merged commit 4109340 into dotnet:master Nov 19, 2020

github-actions bot locked and limited conversation to collaborators Dec 11, 2023

Conversation

pgrawehr commented Nov 4, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krwq commented Nov 10, 2020

Uh oh!

Ellerbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krwq Nov 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgrawehr commented Nov 10, 2020

Uh oh!

pgrawehr commented Nov 10, 2020

Uh oh!

azure-pipelines bot commented Nov 10, 2020

Uh oh!

pgrawehr commented Nov 10, 2020

Uh oh!

joperezr commented Nov 11, 2020

Uh oh!

azure-pipelines bot commented Nov 11, 2020

Uh oh!

Uh oh!

colombod commented Nov 12, 2020

Uh oh!

colombod commented Nov 12, 2020

Uh oh!

pgrawehr commented Nov 12, 2020

Uh oh!

colombod commented Nov 12, 2020

Uh oh!

krwq commented Nov 16, 2020

Uh oh!

Uh oh!

joperezr commented Nov 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joperezr commented Nov 16, 2020

Uh oh!

pgrawehr commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joperezr commented Nov 17, 2020

Uh oh!

pgrawehr commented Nov 17, 2020

Uh oh!

joperezr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

krwq Nov 12, 2020 •

edited

Loading

pgrawehr commented Nov 16, 2020 •

edited

Loading