Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make LibGpiodDriver V2 driver public #2386

Merged
merged 44 commits into from
Apr 3, 2025
Merged

Make LibGpiodDriver V2 driver public #2386

merged 44 commits into from
Apr 3, 2025

Conversation

pgrawehr
Copy link
Contributor

@pgrawehr pgrawehr commented Mar 1, 2025 •
edited by krwq
Loading

Fixes: #2382
Fixes: #2366
Fixes: #2384

@krwq krwq changed the title Provide Chip info for most of our drivers Make LibGpiodDriver V2 driver public Mar 6, 2025
@krwq krwq added the Priority:0 Work that we can't release without label Mar 6, 2025
@krwq krwq added this to the v4.0.0 milestone Mar 6, 2025
krwq
krwq reviewed Mar 13, 2025
View reviewed changes
src/System.Device.Gpio/Interop/Unix/libgpiod/V1/SafeChipIteratorHandle.cs
LibgpiodV1.gpiod_chip_iter_free(handle);
// We can't close the chip here, as this would possibly result in it being freed twice, which causes a crash
LibgpiodV1.gpiod_chip_iter_free_noclose(handle);
handle = IntPtr.Zero;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use SetHandle or https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.safehandle.sethandleasinvalid?view=net-9.0

krwq
krwq reviewed Mar 13, 2025
View reviewed changes
src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/LibGpiodDriver.cs Outdated
public LibGpiodDriver(int gpioChip, LibGpiodDriverVersion driverVersion)
private static bool s_isLibgpiodVersion1_5orHigher = IsLibgpiodVersion1_5orHigher();

private enum RequestFlag : ulong
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be in the interop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's only used here. And it's private, so no change necessary.

krwq
krwq reviewed Mar 13, 2025
View reviewed changes
src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/LibGpiodDriver.cs Outdated
/// <inheritdoc/>
protected internal override int PinCount => GetDriver().PinCount;
int numLines = LibgpiodV1.gpiod_chip_num_lines(chip);
string name = Marshal.PtrToStringAnsi(LibgpiodV1.gpiod_chip_name(chip)) ?? string.Empty;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try using string marshalling attribute for that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(and for one below)

joperezr
joperezr reviewed Mar 13, 2025
View reviewed changes
src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/V2/LibGpiodV2Driver.cs
/// Returns the list of available chips.
/// </summary>
/// <returns>A list of available chips. Can be used to determine the chipNumber when calling the constructor</returns>
public static IList<GpioChipInfo> GetAvailableChips()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need all of this API public? All of this sounds like implementation details that I'm n ot sure we have a valid use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you need these. There might be a reason to select a non-default chip for various reasons. Also, we now know that our automatic selection of the default chip can fail when the hardware/operating system changes, so this allows the user to make it work correctly for his board, regardless of our possibly flaky detection algorithm.

krwq
krwq reviewed Mar 13, 2025
View reviewed changes
src/System.Device.Gpio/System/Device/Gpio/Drivers/SysFsDriver.cs
/// This can be used to determine the correct gpio chip for constructor calls to <see cref="LibGpiodDriver"/>
/// </summary>
/// <returns>A list of chips detected</returns>
public static IList<GpioChipInfo> GetAvailableChips()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you mark this as Experimental? Do we want 2 entries even though there is 1 chip?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As seen in the issue #2366 gpiodetect does the same and also (sometimes) reports the same chip twice. So I mimiked the behavior of the LibGpiod driver here.

krwq
krwq reviewed Mar 20, 2025
View reviewed changes
src/System.Device.Gpio/System/Device/Gpio/Drivers/LibGpiodDriver.cs
@@ -398,14 +447,20 @@ public override ComponentInformation QueryComponentInformation()
return self;
}

/// <inheritdoc />
public override GpioChipInfo GetChipInfo()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You called this MakeSnapshot in one place but GetChipInfo here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MakeSnapShot is part of the internal implementation of the V2 driver and has a different purpose. It's never exposed.

krwq
krwq reviewed Mar 27, 2025
View reviewed changes
src/System.Device.Gpio/System/Device/Gpio/Drivers/UnixDriver.cs
@@ -32,18 +34,39 @@ public static UnixDriver Create()
}

UnixDriver? driver = null;

if (TryCreate(() => new LibGpiodDriver(0), out driver))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to pass in 0 here? shouldn't this be () and below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should consider moving this logic to some other place. We shouldn't have detection logic all over the place that works differently each time.

I would consider obsoleting this, but we can discuss.

krwq
krwq reviewed Mar 27, 2025
View reviewed changes
src/System.Device.Gpio/System/Device/Gpio/Drivers/RaspberryPi3LinuxDriver.cs
@@ -628,7 +627,7 @@ private void InitializeInterruptDriver()
{
try
{
_interruptDriver = new LibGpiodV1Driver(0);
_interruptDriver = new LibGpiodDriver(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be ()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's more clear to keep it explicit here. This is for RPI3 and 4 only.

@krwq krwq mentioned this pull request Mar 27, 2025
Open
@pgrawehr
Copy link
Contributor Author

/azp run dotnet.iot

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@joperezr
Copy link
Member

joperezr commented Apr 1, 2025

/azp run dotnet.iot

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

pgrawehr and others added 26 commits April 3, 2025 08:45
@pgrawehr @joperezr
Disable this test, because it seems it does now crash
cca6685
@pgrawehr @joperezr
Something crashes very ugly here
9356a87
@pgrawehr @joperezr
Make sure this test is the culprit
accbef1
@pgrawehr @joperezr
Revert previous change
e934b50
@pgrawehr @joperezr
Maybe not closing the stuff avoids the crash
278edb4
@pgrawehr @joperezr
No crashes any more?
896a074
@pgrawehr @joperezr
Chip info for V2 completed
dd9fc5f
@pgrawehr @joperezr
Add Test for V2
0ecf0ff
@pgrawehr @joperezr
Minor fix
ef00529
@pgrawehr @joperezr
Fix dotnet#2384: Avoid exception when toggling a pin whose current va…
f6d1082 
…lue is not known
@pgrawehr @joperezr
Stylecop error fixed
4b3525c
@pgrawehr @joperezr
Improve parsing
9d33c73
@pgrawehr @joperezr
This should be the right pattern
3c76e01
@pgrawehr @joperezr
Try to make it work on RPI3, too
d7f7012
@pgrawehr @joperezr
Something behaves differently on the RPI3
8d627b9
@pgrawehr @joperezr
Stupid me
6d34f83
@pgrawehr @joperezr
Is a directory
4d611d9
@pgrawehr @joperezr
Cleanup
761af06
@pgrawehr @joperezr
Use new method to fix a TODO
fef36a8
@pgrawehr @joperezr
Make it more clear how this handle is used
a9f1a7d
@pgrawehr @joperezr
Review findings
18f51af
@pgrawehr @joperezr
Adjust namespaces
119e847
@pgrawehr @joperezr
That didn't compile
d13bde4
@pgrawehr @joperezr
Mark new stuff experimental for now
a921b74
@pgrawehr @joperezr
Include ChipInfo in QueryComponentInformation
e56f9fb
@pgrawehr @joperezr
Some more review findings
4a32169
@joperezr joperezr enabled auto-merge (squash) April 3, 2025 15:55
@joperezr joperezr merged commit 1190b6f into dotnet:main Apr 3, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raffaeler raffaeler raffaeler left review comments

@krwq krwq krwq approved these changes

@joperezr joperezr joperezr approved these changes

Assignees
No one assigned
Labels
Priority:0 Work that we can't release without
Projects
None yet
Milestone
v4.0.0
4 participants
@pgrawehr @joperezr @krwq @raffaeler