[WIP] Board abstraction

[pgrawehr]

Fixes #878 and #974

This is a suggestion/preview how everything should come together for #878.

This PR includes #1039 and #1038, because these three (big) changes somewhat depend on each other. Please leave specific comments on Arduino and Platform-Independent build on those PRs. The relevant changes for this PR are all in the System.Device.Gpio assembly and the base classes defined there:

• New Board class abstracting a piece of hardware with different interfaces (for now: GPIO, SPI, I2C, PWM, Analog In).
• Implementations for Raspberry, generic Unix, Windows and Arduino/Firmata prepared (more TBD, also things like MCP23xxx or ADS1115 possible, and of course FT4222)
• The Board should now be used for creating all interface elements (GpioController, I2cDevice, etc.) This ensures consistent access and allows verification of pin usage - both for error checking (attempting to use a pin for I2C and GPIO at the same time) and setting the proper pin mode, if required (i.e. on the Raspberry).
• This Board class is provided to all interface elements as back-reference, to be able to manage Pins or other common behavior like logical-to-physical pin mappings.
• Therefore added new Ctor to the abstract I2cDevice, SpiDevice, PwmChannel classes
• Marked existing Ctors/Static Create methods obsolete, but should still work.

Some of the advantages:

• Only one place is needed to check whether a board class is compatible with the actual hardware.
• No need to repeat device scans all over the place (especially since all types of drivers are now theoretically accessible regardless of the library target OS).
• Some boards (i.e. Arduino) require one common master component anyway.
• Chaining components (Reading from a DHT11 connection to an MCP23017 to an arduino to a Raspi) is much cleaner to do for a client. (Requires Fixing missing GpioController and ShouldDispose #1016 as well).
• and more...

[shaggygi]

I briefly looked over this and is very interesting. I question if Board should really be named Device instead. Some devices can offer features (GpioController, SPI, etc.) at the component level, as well. Take the MCP2210 as @pgrawehr mentioned for example. In addition, we usually think of hardware, like RPi, BeagleBone, etc. as "boards" since we are usually seeing/touching them as their circuit board is exposed (for hobby/cost reasonings). However, they are "devices". If you bought a plastic case and put an RPi in it... it usually exposes only the pins... just like integrated components (e.g. DIP16, SOIC8, etc.) = bindings (e.g. Mcp2308).

Scenario: Let's say I designed some hardware with a combination of 3 RPi, a few Adafruit modules and other random supporting components. The final module was packaged up in an enclosure and provided some specific features like blink some lights, update an LCD, activate a horn, etc.. But it also exposed some connectors that offer I2C bus and PWM pins. Most likely there would be a "device"-binding API created for this module as a whole. To me, this scenario would be a "device" and not a "board". My point is this module, with a combination of components is no different than a combination of components that make up a RPi. Both are "devices".

Lastly, this core API is called System."Device".Gpio 😄. But as I write this, my brain starts to hurt as you have to start thinking about other possible future System.Device APIs (e.g. System.Device.Bluetooth, System.Device.Hid, etc.). Wouldn't they need to have a respective "Device" (e.g. BleDevice, etc.)? Therefore, this System.Device.Gpio Device might need to be GpioDevice. But then... would you want to create a RaspberryPi that implements GpioDevice as it could implement some of those other APIs. This would not be able to implement multiple abstract classes. 🤕 🍺 🤕

Just my 2 pennies to think about.

[pgrawehr]

@shaggygi Thanks for the feedback. "Board" was the name proposed here, and I think it is a bit less ambiguous than the very generic term "device" and is not used anywhere right now (we already have PwmDevice, SpiDevice, etc), even though it may also not be adequate all the time.

In your scenario, you'd have your "3RpiDevice", consisting of 3 "RpiBoards", with each one of these boards having Gpio Pins, and I2C etc. You could also connect something like an MCP2210 to one of the Rpis, then you have another board, which provides more GPIO pins. So it would be something like:

var uart = firstRpiBoard.CreateUart("/dev/ttyUSB1");
var mcp2210 = new Mcp2210(uart); // Assuming the mcp communicates using uart, but could also use I2C or whatever
mcpGpioPins = mcp2210.CreateGpioController();
mcpGpioPins.Open(1).Write(PinValue.High);

The "board"'s implementation is pretty small, it is mostly a factory instance for other objects, like GpioControllers, I2CControllers etc. So any binding would not implement both I2cDevice and GpioDevice but instead the Board and offer CreateXyz-Methods (which could return internal instances that wrap one's own functionality if needed, of course). This is especially useful for hardware like the MCP (or the FT4222, or the Arduino/Firmata) where there's one common instance that actually talks to the hardware but several logical controllers. And on the Raspi it would be used to verify that only one controller is using a pin at a time.

[shaggygi]

Good points and maybe I'm missing something. You can have the following:

var uart = firstRpiBoard.CreateUart("/dev/ttyUSB1");
var mcp2210 = new Mcp2210(uart); // Assuming the mcp communicates using uart, but could also use I2C or whatever
mcpGpioPins = mcp2210.CreateGpioController();
// Along with:
var rpiPins = firstRpiBoard.CreateGpioController();

I'm thinking we are on the same page for above. I just can't get past implementing mcp2210 as a "board".

[pgrawehr]

I'm thinking we are on the same page for above. I just can't get past implementing mcp2210 as a "board".

I don't know how that piece of hardware actually looks like, but maybe you should just put the drawing of a PCB on top of it ;-) I'm open for better names, but "Device" is just to broad and ambiguous IMHO. And "Controller" is also used all over the place already. Maybe something like "PinManager"?

[shaggygi]

Yup, I believe Device is probably ambiguous, just not sure a better name. The MCP2201 is nothing more than a IC that has USB, GPIO and SPI pins. There are MCP2210 breakout boards (just like Adafruit provides for many ICs). I can see the MCP2210 device binding API implementing this proposal to include methods like CreateGpioController just as I could see the MCP23xxx implementing in the future. But still don't think either would be consider a "board" (understanding that is just a name that is being discussed).

The IoT Plug and Play group is using the word "capability". Not sure if that is any better.
Take a look and see if anything might be better: https://github.com/Azure/IoTPlugandPlay/blob/master/samples/SampleDevice.capabilitymodel.json

[pgrawehr]

Capability isn't the right word we're searching, either, I believe. That would be (like what is described over there) a set of properties describing what a system/piece of hardware can do, not the implementation thereof.

[pgrawehr]

Idea: Maybe we name the base class something like "PinManager", but leave it open for the concrete classes to use matching names. I.e. "ArduinoBoard" would probably be suitable, but for MCP2201 we could still go for "Mcp2201Chip" or so.

[joperezr]

This would be removing the parameterless constructor on PwmChannel which would be a breaking change. If we go with these changes we would have to add that constructor back.

[joperezr]

Same here, this is removing the paramterless constructor which we can't take as it is a breaking change.

[joperezr]

Removing abstract from a member is a breaking change.

[joperezr]

I honestly don't agree with this change. I think that most people using this will still probably not care about the Board abstraction, and what this is doing is probably forcing consumers to use it. Given this Create method is the main entrypoint, I would keep it without Obsolete and simply just add the other option of doing it via a Board.

[joperezr]

One thing that has worked well for Spi and I2c is the factory pattern for creating instances, I would encourage to keep using it specially since we have differnet platform implementations underneath.

[joperezr]

Same comment than with SpiDevice. I think with an introduction of a feature like this, we shouldn't force people down that path every time and instead keep allowing both ways for creating objects so people that don't want to go through the Board abstraction are not force d to.

[joperezr]

I also don't agree too much with this. One problem with this type that we learned in hindsight was that it was leaking implementation details for a very specific implementation (hardware I2c). Adding these would now also leak software implementation. We should talk about this more on how to better approach the connection settings.

[joperezr]

Comment in general for all the Gpio Driver Obsoletes: I don't think that we should go through this path into forcing people to use Board abstraction. One of the features we have in our design is to support people to create new GpioDrivers that internally use 2 or more drivers for cases where you are using pins from different providers. Doing that via the Board now would add an extra layer of complexity that wouldn't be desired.

[joperezr]

Should this be abstract if it is empty?

[pgrawehr]

Basically yes, but then it would be breaking.

[joperezr]

One learning we had from V1 was that it was a wrong design to have the platform on the name of the type as this just increased surface area and complexity. I imagine instead following the factory pattern we have in SpiDevice, where we only have one public class Board, and then have a Create factory method which will in turn decide which one of these implementations to use. Unfortunately we can't fix this aspect on the original API due to compat reasons, but I would vote against making this mistake again.

[pgrawehr]

So you would suggest to make this internal instead? Or change the name?

[joperezr]

Have it internal. So you basically end up having only one public (abstract) Board class without a constructor and with a factory Create method. This Create method, will give you a WindowsBoard when running in Windows and a LinuxBoard when running in Linux. That way Windows* and Linux* is not present anywhere on the public facing API, but we still have the separation logic on our code so stuff still makes sense.

[joperezr]

This should probably be on the Boards namespace

[joperezr]

Why do we need a finalizer?

[pgrawehr]

I added it for completeness, and I think it makes sense here: All of the derived (and aggregated) classes implement IDisposable and potentially allocate unmanaged resources such as device driver handles. These should be cleaned up even if the user forgets to call Dispose.

[joperezr]

In that case the derived and aggregated classes are the ones that should have the finalizer (if applicable), not this one. In general in .NET we actually try to not use finalizers at all when possible, and instead use the Dispose pattern and always using SafeHandles whenever we have a type that holds native resources.

[joperezr]

you may find this page helpful with guidance about Dispose patterns and finalizers: https://docs.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose

[joperezr]

This should only be used for implementation detail, shouldn't be exposed

[pgrawehr]

This is the factory method you suggested to use, so making it internal is not really helpful, I suppose.

[joperezr]

I see, I didn't realize it was returning a Board. In that case the naming is confusing, factory methods are usually called something like Create()

[joperezr]

Sorry it took so long to review the PR. I left some comments specifically on the API, I haven't looked at the implementation yet since I think we should first finalize on the shape before we look into the implementation. I do want to add some overall comments to the feature:

• To be honest, I haven't yet seen much value that the Board abstraction is adding other than pin management, and I think that the fact of now having two ways of creating controllers (or devices) is just going to cause more confusion. I also took a peek on the issue that this is fixing and couldn't find enough examples of why this is needed, so it would be good if either you or @krwq could elaborate more on what are the real advantages and scenarios that this would fix.
• If we did decide to go with it, in my mind this should be less of a disruptive change where you only get the Board abstraction if you want it, but it shouldn't pollute all of the surface area in the rest of S.D.G and it should certainly not obsolete the previous ways of doing things.
• I also think we should probably separate the Analog part from this PR, as that one is a completely new feature that we want to work on regardless, so it would be good to for now just focus on adding the Board onto what we already have.
• Regarding Device vs Board: I don't have a good suggestion here. The only thing to keep in mind is that once we are in agreement in general for what we want to go with here, we will need to take the proposal into API Review (as changes to System.Device.Gpio have to be reviewed by the review board) so they will probably have additional comments here.

[pgrawehr]

Sorry it took so long to review the PR. I left some comments specifically on the API, I haven't looked at the implementation yet since I think we should first finalize on the shape before we look into the implementation. I do want to add some overall comments to the feature:

To be honest, I haven't yet seen much value that the Board abstraction is adding other than pin management, and I think that the fact of now having two ways of creating controllers (or devices) is just going to cause more confusion. I also took a peek on the issue that this is fixing and couldn't find enough examples of why this is needed, so it would be good if either you or @krwq could elaborate more on what are the real advantages and scenarios that this would fix.

There are several reasons for this approach:

• Pin management: There's currently no way of reserving a pin for a certain function or to make sure it is only used for one function at a time. While that part may be a convenience function, the problem denoted in Running SysFsDriverTests or RaspberryPiDriverTests causes PWM to be unusable #874 is not: There's currently no way of setting a pin to a mode other than input or output. Many boards (i.e. Raspi, but also Arduino/Firmata) require a special method call to set a pin to i2c, pwm or spi mode (if it isn't set to that by the bootup configuration).

• Boards requiring a common master interface: Some devices (let's keep calling them "boards" for now) require one common master interface to communicate with them, and the Gpio Controller is only a sub-feature-set of that master interface. These boards are instantiated with a common connection instance (i.e. a serial port number) and all further communication technically goes trough that master interface. Examples for this includes boards such as Arduino, FT4222, MCP2210, but also devices such as I2C port expanders, which are devices that have one primary instance but provide pin controllers. The standard devices (i.e. a Raspi) can benefit from this as well, for instance if it is controlled over a network.

• More obvious control of what happens where: myi2cPortExpander.CreateGpioController() or myArduinoBoardOnCom1.CreateAnalogController() helps the user to understand what device is providing the pins he's going to access.

If we did decide to go with it, in my mind this should be less of a disruptive change where you only get the Board abstraction if you want it, but it shouldn't pollute all of the surface area in the rest of S.D.G and it should certainly not obsolete the previous ways of doing things.

Meanwhile, I mostly agree to that. I think this could simply be done by keeping a global static instance of the main board class (the one the default factory methods would be using).

I also think we should probably separate the Analog part from this PR, as that one is a completely new feature that we want to work on regardless, so it would be good to for now just focus on adding the Board onto what we already have.

I agree, that is basically a separate topic. But it shows that there could be more such extensions coming up (i.e. proper support for one-wire communication pins, UART pins, etc.). I will create a separate PR to discuss that interface.

Regarding Device vs Board: I don't have a good suggestion here. The only thing to keep in mind is that once we are in agreement in general for what we want to go with here, we will need to take the proposal into API Review (as changes to System.Device.Gpio have to be reviewed by the review board) so they will probably have additional comments here.

As long as it's mostly about naming, I don't mind much either.

[joperezr]

While I agree that having better pin management would be beneficial in some scenarios, I find that usually the people that consumes our APIs are also the ones that wire the PCB and sensors/devices, so most of the time they have had to already kind of figure out the pin layout and management themselves which is why at least initially we didn't provide a Board "warpper" class and instead had developers pick and choose the controllers they wanted to use. Also, we expect that most of the final applications that will be using S.D.G won't be doing it directly, but instead would use one (or many) of the Iot.Device.Bindings classes which are in turn using S.D.Gpio. Given that most of those bindings are developed in this repo, we so far haven't found many cases where a Board abstraction was necessary or would have helped a lot. That said, this by no means say that we shouldn't prototype this, but I really want to avoid disrupting the whole S.D.Gpio API surface because of a prototype.

My suggestion for driving this forward is the following: Let's try to come up with a design for that Board class that helps you accomplish pin management and the other issues that it was intended to address, but considering that we want to place that Board class on Iot.Device.Bindings for now. So that means that we want to try not to touch System.Device.Gpio at all for now, and only prototype more or less how this would look like by adding a Board class. The benefits of that is that we don't have to have the right design from the beggining and we can iterate as Board class would be experimental, and we also don't have to go to API Review process because we wouldn't be touching S.D.Gpio. Does that sound like a good plan to you?

[pgrawehr]

That suggestion sounds reasonable, and it would allow the use of the board class for the devices that would clearly benefit from it (and generally from a common interface). FT4222, Arduino, MCP, etc. But it would make it hard (or impossible) to use it on the Raspi, because the GpioController(s) need to have callbacks into the board for the pin management to work properly. That at least requires some kind of (probably small) interface.

[mi-hol]

But it would make it hard (or impossible) to use it on the Raspi, because the GpioController(s) need to have callbacks into the board for the pin management to work properly. That at least requires some kind of (probably small) interface.

Is this great idea & change implementation still trying to overcome the encountered hurdle?

[pgrawehr]

Closing this for now - superseeded by #1128 (@joperezr which could take a review, but I know its a big one)

[joperezr]

(@joperezr which could take a review, but I know its a big one)

Thanks a lot for your patience @pgrawehr we are finally wrapping up dotnet 5 so now we have time to start burning the debt that built up in iot repo, and your board abstraction is definitely something we want to make sure we get reviewed and merged.