adding disposable, shared controller interface and gpiocontroller interface #1167
Conversation
|
Unfortunatelly, there's a big issue: System.Device.Gpio interfaces must be kept backwards compatible. Breaking changes must have very good reasoning. Note also that some interface concept discussions have been open for a long time already (but where postponed due to other tasks and long absences of the responsible maintainers). See #1128 and #878. |
Makes sense, the contract change on the OpenPin and RegisterCallBack is something that could be left out, would it still make the case of having IGpioController interface a big issue with the api backward compatibility? |
Thank you for you feedback, I realized I really just need the IGpioController interface so I can build proxy and adaptors, the other api changes can be done as extension methods so not to make the type too big or break existing contract. Thank you |
I guess so. Interfaces don't scale well (because every change to an interface, including adding methods, is a breaking change). What we're probably going to do (is under discussion now), is to unseal the GpioController class and change its methods to virtual. This (besides other reasons) also allow the mocking support you want. |
|
@pgrawehr the unselaed class would be fine. Any time frame idea for such a change? That would actually enable all api addition I am doing to be an external package that user can adopt if they are interested in some additional pattern and utilities. |
Ellerbach
added
the
area-System.Device.Gpio
| /// <param name="controller">The GPioController.</param> | ||
| /// <param name="pinNumber">The pin number in the controller's numbering scheme.</param> | ||
| /// <returns>A disposable that will close the pin if disposed.</returns> | ||
| public static IDisposable OpenPinAsDisposable(this IGpioController controller, int pinNumber) |
There was a problem hiding this comment.
we're actually thinking of having GpioPin class instead, see #215
| /// <param name="eventTypes">The event types to wait for.</param> | ||
| /// <param name="callback">The callback method that will be invoked.</param> | ||
| /// <returns>A disposable object that will remove the added callback </returns> | ||
| public static IDisposable RegisterCallbackForPinValueChangedEventAsDisposable(this IGpioController controller, int pinNumber, |
There was a problem hiding this comment.
I'd rather have us introduce the real abstraction for this (i.e. CallbackForPinValueChanged) implementing IDisposable
|
I do like confirmation from here that adding GpioPin is the right direction and also looking at this PR seems like another person trying to bypass GpioDriver abstraction which makes me thinking to perhaps just deprecate it and make everything implement GpioController directly (so unseal and virtualize it as proposed by @pgrawehr as well and just not have GpioDriver - and of course leave compat GpioController to make old code still work) |
I think that extra driver class is fine to keep, even if the Controller is unsealed - it separates responsibilities. The Controller handles pin management, while the driver handles the low-level operations on individual pins. |
Yes, this really help to clearly see the need of abstraction, separating drivers from the controller itself and having GpioPin as well.
I agree, this would perfectly work.
And the good news is that with all this, I feel we're all ready to move forward. Once the new model will be present, the trendy topic shouldn't be GpioController anymore on the various issues :-) |
|
Will close this or as I have different things based on your nee library |
With this PR I am trying to propose a discussion around some interfaces used during the development of the piTop bindings