arvydas / BlinkStickDotNet

BlinkStick .NET interface to control devices connected to the computer
GNU General Public License v3.0
32 stars 28 forks source link

Restructured code & Nuget support #7

Open KeesCBakker opened 7 years ago

KeesCBakker commented 7 years ago

Hi!

When I saw the BlinkStick code I saw it was very hard to maintain, that’s why I ended up restructuring its code. One thing led to another and I ended up with doing a lot of structural changes. I’ve tried to keep the methods as backwards compatible as possible.

Are you interested in merging the project?

I've removed my animations project. It can be a separate project (unless you're interested in async animations for the BlinkStick as well, happy to work together).

USB Monitoring The project uses two dependencies, implemented by forked projects. I removed them for their NuGet packages. The code of these packages (the device and the stream) was highly coupled with BlinkStick. I ended up writing a IUsbDevice and IUsbStream abstractions so that packages can be easier swapped out in the future. I also split the monitor in a central monitor and a monitor for devices based on VendorId and ProductId. Note: I don’t have Linux/Mono so I cannot test it. (Had to disable the signing otherwise it wouldn't work. You might want to look into that...)

Meta information Created a wrapper for the meta information so it doesn’t clutter up the BlinkStick class. BlinkStick class still has the properties but reroutes it through the Meta property (decorated with obsoletes).

Animations I’ve separated the animation logic by using an extension class. The user can still use the methods through the BlinkStick class, but the implementation now has its own class (Animations\AnimationExtensions.cs).

Colors I did the same for the colors (Colors\ColorExtensions.cs). All SetColor methods are variations on the SetColor(channel, index, r, g, b).

Integration Testing I had the need to for some integration tests (to test the USB monitor, test if the moved code still worked). That’s why I created a special project that helps debugging scenarios. Project is based on Visual Studio testing. Don’t know if Linux/Mono has something for it.

GetLedCount() Now returns the right number of leds when a Square or other non-pro device is connected.

NuGet For my own project I needed Nuget packages, so I implemented Continuous Integration with MyGet that helps in the creation of Nuget packages. Works like a charm… got the packages up. Love to transfer them to you.

KeesCBakker commented 7 years ago

Packages can be found here if you want to try them out: https://www.nuget.org/packages?q=blinkstick

KeesCBakker commented 7 years ago

Any news on the matter?

drewnoakes commented 7 years ago

@KeesCBakker I'm hoping this project isn't dead. It'd be good to at least hear a response from the maintainer about your PR.

Thanks for making the project available on NuGet. Are you using UsbMonitor? I'm not seeing events being raised and am wondering if it's related to your changes. My code resembles:

var monitor = new UsbMonitor();
monitor.Connected += (s, e) => Console.WriteLine("Connected");
monitor.Disconnected += (s, e) => Console.WriteLine("Disconnected");
monitor.Start();
drewnoakes commented 7 years ago

Took a few minutes to look through this PR. My $0.02 is that too much has changed for this to reasonably be merged. I get wide-reaching PRs from time to time against my own projects and generally they don't get merged. Even if there's some good stuff in there, it's too hard to see what's changed and whether it's correct.

I think you'd have more luck with smaller PRs. A minimal PR that added NuGet support would be great.

drewnoakes commented 7 years ago

Perhaps you could also grant @arvydas access to the NuGet package.

KeesCBakker commented 7 years ago

Thanks for the review. @arvydas can have all the rights to the project and the way packages are deployed. I'm also happy to work on a better way of merging the project.