Board approach, iteration 2 #1128
Conversation
|
This set of abstraction and changes are key to simplify and consolidate other boards design like the piTop 4 module. hope this is merged soon. Very excellent work. |
src/devices/Board/Board.cs
Outdated
| /// <returns>An I2cDevice connection</returns> | ||
| protected abstract I2cDevice CreateSimpleI2cDevice(I2cConnectionSettings connectionSettings, int[] pinAssignment); | ||
|
|
||
| public I2cDevice CreateI2cDevice(I2cConnectionSettings connectionSettings, int[] pinAssignment, PinNumberingScheme pinNumberingScheme) |
There was a problem hiding this comment.
this interface will always create a device manager even if the same connection settings are used. it would be more interesting if there was the option here to track the connections setting used so far, that would make the board something that can be inspected like the pitop boards and plates in this repo .
There was a problem hiding this comment.
So you would want to make sure that each device (here: every instance that uses the same address) can be created only once? Could indeed further reduce usage errors, since most devices won't like it if they're being used i.e. from multiple bindings at the same time.
There was a problem hiding this comment.
Yes you are spot on. In my simple implementation reusing a connection settings yield to the already built device. the interesting part is that the device managers can then auto remove themselves if disposed. Do you think this could imply the need for some sort of reference counting? there are some useful disposables classes in rx.Net that could be very useful like refcount disposables, serial disposables and disposable factories.
There was a problem hiding this comment.
I'll consider this for later, I think. Adding a test that no two I2cDevice instances with the same ConnectionSettings can be done without changing the API later.
Ellerbach
added
the
area-System.Device.Gpio
| /// </summary> | ||
| /// <param name="pinNumber">Pin number</param> | ||
| /// <returns>(Alternate) Pin mode. 0 = Alt0, 1= Alt1... -1 Gpio Input, -2 Gpio Output</returns> | ||
| protected internal int GetAlternatePinMode(int pinNumber) |
There was a problem hiding this comment.
should this be an enum (flags)? I assume there might be more than one mode, right?
There was a problem hiding this comment.
Ah, this is about current pin mode, not all possible options... (still.. comment about enum applies)
There was a problem hiding this comment.
I would really like to use an enum here. Unfortunately, the set of alternate pin modes is hardware dependent. So if I go with the names that are most common here, they would be Raspberry Pi-specific and be confusing to use on some other hardware (the Arduino binding for instance uses a similar semantics, but with different values)
Maybe return a class instead? (That would be handled much like a handle for most of what the client does)?
There was a problem hiding this comment.
if you have some decent design with struct (class is too heavy to be used instead of enum) then fine, otherwise I'm fine with leaving as is
There was a problem hiding this comment.
Used a class with static members now. This syntactically looks identical to using an enum, but is extensible.
src/System.Device.Gpio/System/Device/Gpio/Drivers/RaspberryPi3LinuxDriver.cs
Outdated
Show resolved
Hide resolved
| /// <param name="pinNumber">The pin number, in the boards default numbering scheme</param> | ||
| /// <param name="usage">Intended usage of the pin</param> | ||
| /// <param name="owner">Class that owns the pin (use "this")</param> | ||
| public virtual void ReservePin(int pinNumber, PinUsage usage, object owner) |
There was a problem hiding this comment.
I'd prefer this was not part of the initial version of abstraction, this needs more thought. Perhaps this is needed long term and this is how the API would look like but I'd prefer we did it separately as this PR is already large enough for this rather niche concept
There was a problem hiding this comment.
I have to check whether it can be removed easily. I think the method is internally used already.
There was a problem hiding this comment.
This is one of the main features of the new approach, actually. It manages the pin usages and keeps track of who uses which pin for what purpose. This is public to also allow reserving pins directly from a client (i.e. for UART, which is not currently managed here)
| /// </summary> | ||
| /// <param name="pinNumber">The logical pin number to use.</param> | ||
| /// <param name="usage">The intended usage</param> | ||
| protected virtual void ActivatePinMode(int pinNumber, PinUsage usage) |
There was a problem hiding this comment.
if anything this should be throwing by default but I'd rather keep this concept out of the initial abstraction
There was a problem hiding this comment.
This also belongs to the main feature: It allows (see the RaspberryPiBoard specific implementation) to set the alternate pin mode for a pin. This internally calls SetAlternatePinMode on the Raspi
pgrawehr
force-pushed
the
BoardApproach3
branch
from
3c3f6c0 to
1f97c5d
Compare
…s and driver-specific implementation
pgrawehr
force-pushed
the
BoardApproach3
branch
from
ccaa009 to
54a33be
Compare
src/devices/Board/Board.cs
Outdated
| return board; | ||
| } | ||
|
|
||
| throw new PlatformNotSupportedException("Could not find a matching board driver for this hardware"); |
There was a problem hiding this comment.
Is there a reason why we don't try as well with CustomBoard?
There was a problem hiding this comment.
Because the CustomBoard requires custom drivers. It's "default" implementation would be equivalent to the GenericBoard.
| /// <summary> | ||
| /// A generic board class. Uses generic implementations for GPIO, I2C etc | ||
| /// </summary> | ||
| public class GenericBoard : Board |
There was a problem hiding this comment.
Is there a specific reason why this is public? I would rather have this (and the RaspberryPiBoard and CustomBoard) internal, and just have the Create method on the base class to create the right one for you. We could also even have an overload of Create that takes in an Enum so that we allow callers to specify which board they want to create. In the past on dotnet/iot we have found that using factory pattern is better (like in SPI and I2C) for future refactorings since it was way easier to adapt and develop that when compared to GPIO (which doesn't have the factory pattern)
There was a problem hiding this comment.
This is the one you would use if you can't use or don't want to use the RaspberryPiBoard. It uses all the default implementations.
Adds complexity to the API for a rarely used feature.
|
@joperezr I think I have addressed the open issues. Should be ready to go now. |
Adds a Board base class to simplify handling common features of a piece of hardware.
Also adds support for proper pin mode handling on the Raspberry Pi. I.e. it is now possible to use pins interchangeably as PWM and GPIO, or switch between software and hardware SPI on the same set of pins.