Remove platform dependency #1038

pgrawehr · 2020-04-05T18:47:52Z

This makes the System.Device.Gpio library build platform-independent. The drivers to use for a specific platform are now determined at runtime.

The target is to avoid breaking backwards compatibility for existing clients. Whether that is 100% possible will yet need to be determined.

Work in progress. Tasks to be completed:

Check for side effects (i.e. attempts to load inexistent libraries)
Check that the public APIs haven't changed (edited: @joperezr has done this)
Clean up build scripts / Nuget package contents. (edited: @joperezr has done this)
Nuget Symbols packages should include the PDB files (Looks like that hasn't been the case even before) - Debug information type is "Embedded", so this is unneccessary

joperezr · 2020-04-06T19:01:40Z

src/System.Device.Gpio/Directory.Build.targets

@@ -1,60 +1,18 @@
 <Project>
-  <!-- Targets for building the notsupported assembly -->
-  <Target Name="SetupEnvironmentForNotSupportedAssembly">


There is one more problem with doing this which we haven't discussed, and that is what will happen when somebody tries to run this on platforms like OSX. I think that we should perhaps still throw not supported there, but I'm not sure what is the best way to do that.

It will be hard to test things on OSX, since I don't have a Mac. But I believe if we really do all the testing for a specific hardware at runtime, this should work on OSX the same. And things like Arduino or FT4222 should work just fine. It would just throw an Exception if trying to instantiate i.e. the Windows10Driver, but that's very much the same as if you try this on linux.

There's a difference between "the library is not supported on Platform X" vs "there's no hardware available on Platform X for which we have a driver".

src/System.Device.Gpio/Directory.Build.targets

src/System.Device.Gpio/System.Device.Gpio.csproj

src/System.Device.Gpio/System/Device/Gpio/GpioController.Linux.cs

src/System.Device.Gpio/System/Device/Gpio/Drivers/HummingBoardDriver.Linux.cs

pgrawehr · 2020-04-07T13:43:03Z

Fixes #1037 as well

joperezr · 2020-04-20T16:19:09Z

There is a bunch of stuff on Directory.Build.Targets that can also get removed. Same goes for eng\CrossTargetingRIDS.targets. If you are ok with it, I could also commit into this PR and remove a bunch of infra stuff that wouldn't be required after this. Since I was the one that set most of this up, then it'll probably be easier for me to find all these things no longer needed.

src/System.Device.Gpio/System.Device.Gpio.csproj

joperezr · 2020-04-20T16:35:31Z

src/System.Device.Gpio/System.Device.Gpio.csproj

-  <ItemGroup Condition="'$(TargetsLinux)' == 'true'">
-    <!--Excluding Windows implementations-->
-    <Compile Remove="**\*.Windows.cs" />
-    <Compile Remove="Interop\Windows\**\*.cs" />


This does mean that now the Linux implementation will depend on some winmd files, obviously not at runtime but just wanted to note that. I don't expect this to be a problem, but there are some runtimes that will fail to load an assembly when one of the dependencies is not present, that said I think that only applies to AOT so we are most likely safe here.

What is AOT?

Apparently, it runs on all existing platforms (at least all builds are green). For Windows and Linux I tested that the exceptions only pop up when the corresponding (in this case: the wrong) classes are instantiated. But that's the behavior we expect.

AOT is ahead-of-time so basically it is when IL is compiled down to native from compile time. There are several frameworks that work like that, like UWP or Xamarin. Again, .NET Core is not like that so I think we are safe so we probably don't need to do anything special here.

joperezr · 2020-04-20T16:49:52Z

src/System.Device.Gpio/System/Device/Gpio/Drivers/HummingBoardDriver.cs

@@ -7,7 +7,7 @@ namespace System.Device.Gpio.Drivers
    /// <summary>
    /// A GPIO driver for the HummingBoard.
    /// </summary>
-    public partial class HummingBoardDriver // Different base classes declared in HummingboardDriver.Linux.cs and HummingboardDriver.Windows.cs
+    public class HummingBoardDriver : Windows10Driver


This change I don't fully like here, it is definitely less than ideal to have HummingBoardDriver which could be running in Linux to have a base class called Windows10Driver. We should either think on how to either make the HummingBoardDriver extend GpioDriver in Windows as well, or to rename Windows10Driver so that we can make something less platform-named. Both changes would be breaking.

The reason for changing it that way was that the HummingBoardDriver.Linux.cs implementation file previously consisted of only "NotImplementedException" methods, so it newer worked. Therefore I thought it was anyway only supported on Windows.

If this was ever supported on linux, we could just provide a HummingBoardLinuxDriver (and move the magic of choosing the right one to the HummingBoard class or at least the GetBestDriverForBoard() method).

having it never worked in the past is different as have it never work at all in the future. Imagine we eventually decide to add a HummingBoard driver in the future for Linux, and if so and we keep this as is, it means we would have to have an implementation of Windows10Driver on Linux as well so that we don't introduce a breaking change. This is obviosly less than ideal. In hindsight, we probably should have never exposed drivers with platforms name on them, it would be better to have a "GenericDriver" or "PlatformDriver" and have different implementations for it (what today is Wiindows10Driver and UnixDriver) but I guess that boat has sailed and will brings us some trouble now that we are unifying things.

We could make the HummingBoardDriver aggregate its underlying driver (UnixDriver or Windows10Driver) instead of inheriting it. That would make the actual implementation flexible, and, despite it technically being a breaking change, probably nobody would really notice.

joperezr · 2020-04-20T16:51:30Z

public class LibGpiodDriver : UnixDriver

All These drivers that are Linux specific (LibGpiod, RaspberryPi3, UnixDriver, etc) should now have a constructor which first thing it does is to check OS, and if platform is not Unix then throw PNSE. We defenitely don't want to fail late for something we know won't work.

Refers to: src/System.Device.Gpio/System/Device/Gpio/Drivers/LibGpiodDriver.cs:12 in 1dff47a. [](commit_id = 1dff47a, deletion_comment = False)

joperezr · 2020-04-20T16:56:48Z

Left some comments, mostly what I want to see is if you could add logic on most GPIO driver constructors to make sure we throw Platform not supported when running on a OS that the driver won't support. Other than that most things look great, as I said if you are ok I'm happy to contribute to your PR by removing all logic related to packaging and cross rid targeting cleanup, and also to verify that the surface area is still the same. Last thing to discuss here, would be the differences between base clases in Windows vs Linux, since Windows adds a base class to some of their Drivers so that is something we will have to design on how to fix as change will most likely be breaking.

pgrawehr · 2020-04-20T19:24:30Z

There is a bunch of stuff on Directory.Build.Targets that can also get removed. Same goes for eng\CrossTargetingRIDS.targets. If you are ok with it, I could also commit into this PR and remove a bunch of infra stuff that wouldn't be required after this. Since I was the one that set most of this up, then it'll probably be easier for me to find all these things no longer needed.

Sure, go ahead. I was hoping to get some support on this from you, just because you created this setup.

pgrawehr · 2020-04-20T19:46:26Z

Last thing to discuss here, would be the differences between base clases in Windows vs Linux, since Windows adds a base class to some of their Drivers so that is something we will have to design on how to fix as change will most likely be breaking.

Please verify this, but I think it's not, because so far we did not have a driver that actually worked on both platforms. I.e. there was a RaspberryPi3Driver that inherited from WindowsDriver on Windows and UnixDriver on Unix, but this class didn't work on windows (the implementation consisted of exceptions only). So while technically, it might be a breaking change, nobody used these classes on the wrong platform.

joperezr · 2020-04-24T17:59:35Z

Sure, go ahead. I was hoping to get some support on this from you, just because you created this setup.

Sounds good, I'll push some changes today to remove stuff as well as address some nits so that we can get your PR ready to go

joperezr · 2020-04-24T18:36:00Z

Please verify this, but I think it's not, because so far we did not have a driver that actually worked on both platforms. I.e. there was a RaspberryPi3Driver that inherited from WindowsDriver on Windows and UnixDriver on Unix, but this class didn't work on windows (the implementation consisted of exceptions only). So while technically, it might be a breaking change, nobody used these classes on the wrong platform.

Actually I believe that RaspberryPi3Driver did work on Windows, because it was just basically inheriting from WindowsDriver, so it would really just forward the calls to WindowsDriver. HummingBoardDriver should be doing the same, I'll address this in a commit, push it to your branch and see what you think. With my change I'll try to make sure we don't break scenarios that worked in 1.0 and also to not introduce a breaking change, and finally to ensure that we won't be adding base classes to some drivers that are platform specific in case we someday want to add implementation to them.

joperezr · 2020-04-24T20:39:05Z

@pgrawehr @krwq please take a look at ce4568a to see how I'm dealing with making sure we don't introduce breaking changes but at the same time allowing some drivers to use the implementation from other drivers. @tarekgh I'm particularly interested in your take on the changes that went in to the SetRegister and ClearRegister on the RaspberryDriver as I want to make sure that this extra layer of indirection won't hit perf on the LCD Display

krwq · 2020-04-24T20:59:27Z

@joperezr I don't think this will regress it but if it does I think we should fix it separately. Overall this change over-benefits temporarily breaking a single device bindings. We should definitely test this after this is merged

joperezr · 2020-04-27T20:34:43Z

Next step will probably be to have a look trough the samples and clean up those projects.

Oh you are right, I forgot about those, I'll include one more commit to fix that. As for the solutions for all bindings, let's do that in a separate PR since it is really a different concept. It is also easy to do, it is just a matter of creating a script to go into each folder and take care of it for us.

pgrawehr · 2020-04-27T20:43:42Z

It is also easy to do, it is just a matter of creating a script to go into each folder and take care of it for us.

Oh, even better than doing it by hand...

System.Device.Gpio is now fully platform-independent and chooses the required drivers at runtime.

Combine classes into one file, where still needed

Any attempt to instantiate these classes on the wrong HW shall fail

joperezr

Changes look good, thank you so much for your contribution with this @pgrawehr, you have definitely taken a big item off my backlog 😄 so I really appreciate it.

joperezr · 2020-04-27T22:23:51Z

I'll merge after CI passes

joperezr · 2020-04-27T23:00:00Z

Looks like the FastEvent test is very flaky in Windows so I'll go ahead and disable that for now

krwq · 2020-04-27T23:06:41Z

@joperezr is there an issue about this flaky test? if not can you create it and if there is one can you add a comment in the code to point to that issue?

joperezr · 2020-04-27T23:16:05Z

done with #1054

pgrawehr mentioned this pull request Apr 6, 2020

Arduino/Firmata device support #1039

Merged

joperezr reviewed Apr 6, 2020

View reviewed changes

src/System.Device.Gpio/Directory.Build.targets Show resolved Hide resolved

joperezr reviewed Apr 6, 2020

View reviewed changes

src/System.Device.Gpio/System.Device.Gpio.csproj Show resolved Hide resolved

joperezr reviewed Apr 6, 2020

View reviewed changes

src/System.Device.Gpio/System/Device/Gpio/GpioController.Linux.cs Outdated Show resolved Hide resolved

joperezr reviewed Apr 6, 2020

View reviewed changes

src/System.Device.Gpio/System/Device/Gpio/Drivers/HummingBoardDriver.Linux.cs Show resolved Hide resolved

pgrawehr force-pushed the RemovePlatformDependency branch from 7218c30 to 5bc13d0 Compare April 7, 2020 08:49

pgrawehr force-pushed the RemovePlatformDependency branch 3 times, most recently from 4f7acbf to 2728b5a Compare April 7, 2020 19:34

pgrawehr changed the title ~~[WIP] Remove platform dependency~~ Remove platform dependency Apr 7, 2020

pgrawehr marked this pull request as ready for review April 7, 2020 19:50

pgrawehr force-pushed the RemovePlatformDependency branch from 2728b5a to 1dff47a Compare April 7, 2020 19:53

pgrawehr mentioned this pull request Apr 15, 2020

[WIP] Board abstraction #1044

Closed

joperezr self-assigned this Apr 17, 2020

joperezr reviewed Apr 20, 2020

View reviewed changes

src/System.Device.Gpio/System.Device.Gpio.csproj Outdated Show resolved Hide resolved

joperezr reviewed Apr 20, 2020

View reviewed changes

src/System.Device.Gpio/System.Device.Gpio.csproj Show resolved Hide resolved

joperezr reviewed Apr 20, 2020

View reviewed changes

joperezr force-pushed the RemovePlatformDependency branch from 642e323 to ce4568a Compare April 24, 2020 20:36

pgrawehr and others added 17 commits April 27, 2020 15:20

Remove the build-time platform depencency

8245b6f

System.Device.Gpio is now fully platform-independent and chooses the required drivers at runtime.

Remove now wrong solution configuration

bd4d673

Remove now unused parts

13dce74

Remove special handling - no more required

fbc1b5d

Remove some more platform dependency

758eb2a

Add XUnit Runner toolkit

2234df8

Remove unused properties

44df40b

Remove now unused .linux. and .windows. suffixes

bc68c3f

Combine classes into one file, where still needed

Adapt test file names as well and add Traits

4597ab2

Update documentation

92b170a

Make sure PlatFormNotSupportedException is thrown on a wrong ctor

b2dadfc

Any attempt to instantiate these classes on the wrong HW shall fail

Remove unused properties

1375935

Removing CrossRID targetting stuff since it won't be needed any longer.

b7aa660

Adding platform PALs so we dont introduce breaking changes

5b4c91b

Only creating a windowsDriver when running in Windows.

75e3378

Removing extra layer of indirection and using delegates instead

349efa7

Fixing samples projects to remove RuntimeIdentifier

628ae92

joperezr force-pushed the RemovePlatformDependency branch from 1061ca4 to 628ae92 Compare April 27, 2020 22:22

joperezr approved these changes Apr 27, 2020

View reviewed changes

Skip test on WIndows due to flakiness

5d80214

joperezr mentioned this pull request Apr 27, 2020

Disabled test in Windows FastInterruptHandling #1054

Closed

joperezr merged commit 00f9da1 into dotnet:master Apr 27, 2020

pgrawehr mentioned this pull request May 25, 2020

Sample projects fail to properly build on Windows #1037

Closed

github-actions bot locked and limited conversation to collaborators Dec 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove platform dependency #1038

Remove platform dependency #1038

pgrawehr commented Apr 5, 2020 •

edited by joperezr

Loading

joperezr Apr 6, 2020

pgrawehr Apr 7, 2020

pgrawehr Apr 7, 2020

pgrawehr commented Apr 7, 2020

joperezr commented Apr 20, 2020

joperezr Apr 20, 2020

pgrawehr Apr 20, 2020

joperezr Apr 24, 2020

joperezr Apr 20, 2020

pgrawehr Apr 20, 2020

joperezr Apr 24, 2020

pgrawehr Apr 24, 2020

joperezr commented Apr 20, 2020

joperezr commented Apr 20, 2020

pgrawehr commented Apr 20, 2020

pgrawehr commented Apr 20, 2020

joperezr commented Apr 24, 2020

joperezr commented Apr 24, 2020

joperezr commented Apr 24, 2020

krwq commented Apr 24, 2020

joperezr commented Apr 27, 2020

pgrawehr commented Apr 27, 2020

joperezr left a comment

joperezr commented Apr 27, 2020

joperezr commented Apr 27, 2020

krwq commented Apr 27, 2020

joperezr commented Apr 27, 2020

Remove platform dependency #1038

Remove platform dependency #1038

Conversation

pgrawehr commented Apr 5, 2020 • edited by joperezr Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgrawehr commented Apr 7, 2020

joperezr commented Apr 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joperezr commented Apr 20, 2020

joperezr commented Apr 20, 2020

pgrawehr commented Apr 20, 2020

pgrawehr commented Apr 20, 2020

joperezr commented Apr 24, 2020

joperezr commented Apr 24, 2020

joperezr commented Apr 24, 2020

krwq commented Apr 24, 2020

joperezr commented Apr 27, 2020

pgrawehr commented Apr 27, 2020

joperezr left a comment

Choose a reason for hiding this comment

joperezr commented Apr 27, 2020

joperezr commented Apr 27, 2020

krwq commented Apr 27, 2020

joperezr commented Apr 27, 2020

pgrawehr commented Apr 5, 2020 •

edited by joperezr

Loading