-
Notifications
You must be signed in to change notification settings - Fork 585
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
Better abstraction for PWM: PwmChannel #576
Conversation
src/System.Device.Gpio/System/Device/Pwm/Channels/UnixPwmChannel.Linux.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Pwm/Channels/UnixPwmChannel.Linux.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Pwm/Channels/UnixPwmChannel.Linux.cs
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Pwm/Channels/Windows10PwmChannel.Windows.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Pwm/Channels/PwmChannel.Linux.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Pwm/Channels/PwmChannel.Linux.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Pwm/Channels/PwmChannel.Windows.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Pwm/Channels/UnixPwmChannel.Linux.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Pwm/Channels/UnixPwmChannel.Linux.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Pwm/Channels/Windows10PwmChannel.Windows.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Pwm/Channels/Windows10PwmChannel.Windows.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Pwm/Channels/PwmChannel.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Pwm/Channels/PwmChannel.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Pwm/Channels/UnixPwmChannel.Linux.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Pwm/Channels/UnixPwmChannel.Linux.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Pwm/Channels/UnixPwmChannel.Linux.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Pwm/Channels/UnixPwmChannel.Linux.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Pwm/Channels/UnixPwmChannel.Linux.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Pwm/Channels/UnixPwmChannel.Linux.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Pwm/Channels/Windows10PwmChannel.Windows.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Pwm/Channels/UnixPwmChannel.Linux.cs
Show resolved
Hide resolved
Build is failing because of the chicken an egg problem again. To fix it, you could either do the temporary fix that we did in SPI and I2C with making the bindings package use a package reference to the main library like here: iot/src/Iot.Device.Bindings/Iot.Device.Bindings.csproj Lines 21 to 24 in d6274e4
Or alternatively you could have the projects of the bindings that are failing to use the live version of System.Device.Gpio, and fix all the namespace/class issues upfront. |
I will be off the grid most of the next 2 weeks. I'll try to check in every now and then if time allows. If you can send me some docs on how to prepare and install the lastest Win10 IoT for RPi 3B+, I will try to at least prepare the device to work on the Windows10PwmChannel for when I return. |
@joperezr do you have instructions for Win10 IoT Core? I've only used Raspbian/Ubuntu with RPi. @shaggygi did you try https://docs.microsoft.com/en-us/windows/iot-core/downloads ? What errors are you seeing? Do you need anyone to pick up this PR? |
If that's ok with you @shaggygi I will finish this PR since we do want to take it in sooner rather than later :). As to instructions on how to set up Windows IoT core, I have usually just downloaded the Windows IoT Core Dashboard and follow the steps from that in order to flash my SD card. |
@joperezr that is fine. Sorry on delay |
1 similar comment
@joperezr that is fine. Sorry on delay |
@shaggygi no worries at all, it wasn't a delay, you got us in a very great place with a head start 😉. We just want to finish this in less than two weeks and don't want to interfere at all with your plans 😄 |
Code is ready after my last change, I just want to run some more tests to make sure everything is behaving as expected in both Unix and Windows. |
src/System.Device.Gpio/System/Device/Pwm/Channels/UnixPwmChannel.Linux.cs
Outdated
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Pwm/Channels/Windows10PwmChannel.Windows.cs
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Pwm/Channels/Windows10PwmChannel.Windows.cs
Outdated
Show resolved
Hide resolved
int chip, | ||
int channel, | ||
int frequency = 400, | ||
double dutyCyclePercentage = 0.5) => |
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.
I think we should have Create and CreateInternal, Create should have defaults and CreateInternal should be explicit so that we don't accidentally have different defaults on both platforms
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.
I think having Create is fine for now since that is what we have in other similar APIs (like I2cDevice.Create or how the GpioController picks a driver), I'll keep it as is and we can discuss later if we want to change to CreateInternal
src/System.Device.Gpio/System/Device/Pwm/Channels/UnixPwmChannel.Linux.cs
Show resolved
Hide resolved
src/System.Device.Gpio/System/Device/Pwm/Channels/UnixPwmChannel.Linux.cs
Show resolved
Hide resolved
Great work guys, by the way this solve a multi channel access issue that was in the old code ... thanks |
Fixes #204
This PR is based on the new
PwmChannel
concept. See #204 for more details.cc: @krwq @joperezr This only includes the code for Linux. I wanted to make sure the feature fits what was expected before adding files for Windows. I started some review comments to point out a few thoughts/issues found along the way.