-
Notifications
You must be signed in to change notification settings - Fork 586
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add libgpiodv2 support #2184
Add libgpiodv2 support #2184
Conversation
…talled libgpiod version
@dotnet-policy-service agree |
Thanks for the submission! Can you please specify on what system you ran your tests and what systems you expect to work with the new library? Are there any systems where the old driver is no longer supported and only the new one will work? |
src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/V1/LibGpiodV1Driver.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/V1/LibGpiodV1Driver.cs
Outdated
Show resolved
Hide resolved
All right, I will be patient then :) If you ask me then the answer is Raspberry Pi. It was also Pi's 3,4 and 5 on which I ran the tests. On the question of whether the V2 driver can replace the V1 driver: It depends :-) It's also not the goal of this PR to replace the v1 driver, rather to "prepare for the future" and have a v2 implementation in place, whose features can be used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work overall!
src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/V1/LibGpiodV1Driver.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/Interop/Unix/libgpiod/V1/SafeChipHandle.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/Interop/Unix/libgpiod/V2/GpiodException.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/Interop/Unix/libgpiod/V2/Binding/Enums/GpiodEdgeEventType.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/Interop/Unix/libgpiod/V2/Binding/Handles/EdgeEventSafeHandle.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/Interop/Unix/libgpiod/V2/Binding/Handles/LineInfoSafeHandle.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/Interop/Unix/libgpiod/V2/Proxies/Chip.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/Interop/Unix/libgpiod/V2/Proxies/LibGpiodProxyBase.cs
Show resolved
Hide resolved
src/System.Device.Gpio/Interop/Unix/libgpiod/V2/Proxies/Chip.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Gpio/Drivers/RaspberryPi3LinuxDriver.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/LibGpiodDriverFactory.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/LibGpiodDriverFactory.cs
Show resolved
Hide resolved
Thanks a bunch for the contribution @huesla, I went through it and added some feedback (might seem like a lot but most are just either NITs or questions) but this is overall looking really good. Thanks a lot again for taking the time to contribute this. |
src/System.Device.Gpio/System/Device/Gpio/Drivers/UnixDriver.cs
Outdated
Show resolved
Hide resolved
/azp run dotnet.iot |
Azure Pipelines successfully started running 1 pipeline(s). |
Open points:
|
They're rather strange and look like false positives
/azp run dotnet.iot |
Azure Pipelines successfully started running 1 pipeline(s). |
@huesla I've tried to fix the remaining issues, but the thing with the library search path is complicated. On my Rpi4 (with 64 bit Raspberry Pi OS) the correct path for |
@pgrawehr thanks for your investigation.
I think on my previous system setup the libraries ended up in "/usr/lib" because I may have compiled and installed them manually only. But under Debian there is multi arch support. |
Sounds like a plan. I was first thinking about parsing the output of ldconfig, but that's quite ugly to do. Of course, recursively iterating the subfolders of /usr/lib could theoretically find the wrong library (e.g. the 32bit library for a 64 bit process), but normally you would have either both or none, I suppose. |
@pgrawehr the search is now recursive. Should we enable tests again? |
/azp run dotnet.iot |
Azure Pipelines successfully started running 1 pipeline(s). |
@pgrawehr the V1 tests succeeded, can we run V2 aswell? (they are still disabled) |
@huesla No, because the devices have not been upgraded yet. That may take some time, but I will push that further. I don't have the necessary permissions myself. When the old driver works, it's fine for me to merge anyway. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work and great collaboration in this PR. I've been following the different threads and progress.
/// <summary> | ||
/// Driver factory for different versions of libgpiod. | ||
/// </summary> | ||
internal sealed class LibGpiodDriverFactory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's see if we can find some existing pattern in dotnet/runtime or other repo for searching for right .so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as long as we open P0 issue to improve .so searching logic
Thanks to all for the collaboration aswell. If there is anything else to improve, let me know |
@huesla We had a call on Thursday and decided to merge this now and then address some of the remaining issues separately. The two points that were still disputed was the library search algorithm and the best way to expose this new functionality. For this, I have created #2271 and #2272. In no way, this should be criticism on your work, this is just trying to allign it with large scale standards. |
/azp run dotnet.iot |
Azure Pipelines successfully started running 1 pipeline(s). |
Fixes: #2179
Hi there,
This PR aims to integrate version 2 of libgpiod
Structure
Basically, my strategy was to extend as much functionality as possible while touching as little existing code as possible.
A lot has changed in the API between v1 and v2 of libgpiod. For example, the control of GPIOs is purely via so-called request objects. If you want to learn more about the concepts, see:
As the new functions are very different from the existing ones, I have decided to differentiate between V1 and V2 in the namespace or class name. (What changes together should go together). This concerns the Interop class, as well as the driver and event handler.
The new structure from the lower to the higher level is shown as follows:
Binding
Interop.libgpiod
Maps all methods of the current libgpiod API, structured in the same way as in the docs.
Proxies
libgpiod offers so-called "opaque data classes". These are objects whose fields are not visible and can only be changed via methods. libgpiod divides the API logically according to objects (or modules) see docs. To represent these in C#, there are now so-called proxy classes. Such a class offers the same methods as the controlled gpiod object, but in a more C#-friendly way.
LibGpiodProxyBase
is the base class and provides methods for thread security.(more information on the threading contract of libgpiod) and wraps any exception from the native call in
GpiodException
(easier for clients to handle).I think when you know the concept of a libgpiod module, looking at the corresponding proxy class should give a clear picutre in what it does (Basically it only forwards stuff).
Handles
Each proxy class works with a
SafeHandle
, the object that holds the pointer to the gpiod object. SafeHandle comes from the framework and makes sure to callReleaseHandle()
once, which frees the gpiod object.Enums
Constants see docs
LibGpiodV2Driver
This class is the "bridge" between
GpioController
and the binding. Each driver instance manages one gpiochip.It implements the usual methods and manages proxies, mainly the
LineRequest
to control GPIOs. The methods of the driver were implemented to have as little traffic as possible, e.g. reuse existingLineRequest
objects etc.LibGpiodV2EventObserver
Is closely coupled with LibGpiodV2Driver and manages EdgeEvent subscriptions / callbacks.
GpioController
Existing class that instantiates the driver. LibGpiodDriverFactory helps to load the correct version suitable for the system. If libgpiodv2 is available, v2 is loaded, otherwise v1 or v0.
Testing
Tested with manual tests and
GpioControllerTestBase
tests on Raspberry Pi 3/4/5 with latest RPI OS (lite).I had to adapt the gpiochip number with Pi5 to 4, instead of 0 on Pi3/4. I am not sure how you treat different boards in unit tests, so I left 0 as default, because Pi5 is not yet as popular.
Testing (continued)
The current tests of
GpioControllerTestBase
use delays, which provokes flakyness and makes the tests slow.I have an improvement branch that uses reset events instead of delays. Again I tested it on the Pi 3/4/5... the time got improved alot and I did not encounter flakyness anymore:
Pi3: 50.2 sec -> 32.2 sec
Pi4: 42.8s -> 19.3s
Pi5: 40.3s -> 13.2s
(measured only once)
I could make a seperate PR for this after this one, depending on how the integration of this will go.
The real question is whether other drivers can keep up... would probably have to be tested with your CI pipeline.
Commits
The commits are chronologically logically ordered.
Advanced use
It should be noted that the current GpioController/Driver architecture imposes restrictions on the libgpiodv2 functionality. For example, information such as ChipInfo, LineInfo, EventInfo, Event sequence numbers etc. are not available. Also functions like setting Debounce Period, EventBuffer size etc. are not available. (One of my use cases is, for example, the time-accurate detection of edges, and the time info is in the edge event as Ns timestamp).
In order not to change the existing architecture, I have stuck to it and made proxies etc.
internal
.However, this also means that this functionality cannot be used externally.
My suggestion is therefore to make it public. This allows advanced users extended functionality if they need it. Normal users can continue to use the GpioController API.
Since it would probably be a bit tedious (even for advanced users) to manage proxy objects directly, and to use every fine grained function, it would make sense to create an abstraction that would simplify this and cover the most relevant use cases. The code could look much nicer then, for example:
(By the way, this example is inspired by the Python wrapper for libgpiodv2)
I would of course also make documentation for a few examples.
I look forward to comments and opinions :)
Microsoft Reviewers: Open in CodeFlow
EDIT (@krwq): adding link to the issue