PwmController/PwmDevice should be bound to specific pin

[krwq]

Currently the PwmController class requires passing channel and chip every time.

There are couple of disadvantages of such approach:

• APIs are inconvenient and require user to store pin value and pass it on every call
• PWM settings along with pin have to be dragged along to all depending classes which also need to access PWM settings
• it is not known if channel is open or not
• other PWM implementation (i.e. SoftwarePwm does not use channel and chip and uses pin instead)

my proposition is to have simpler design which looks more or less like this:

public abstract PwmDevice : IDisposable
{
  protected PwmDevice() {}
  public abstract double FrequencyHz { get; } // note: can't change after setting up
  public abstract double DutyCyclePercentage { get; set; } // 0.0 - 1.0
  public static PwmDevice Create(int chip, int channel, double frequencyHz = 50.0, double dutyCyclePercentage = 0.0) { /* create default pwm device depending on the OS, default values are arbitrary and TBD */ }
}

Notes:

• change range of DutyCycle from 0 - 100 to 0-100% because % in math equals to 0.01 and 100%=1.0. Also it combines with any other math done in the app much better. I.e.: dutyCycle = a/b and not dutyCycle = a/b*100
• channel will be open when constructed by default, when dutyCycle is 0 it technically does nothing anyway

[shaggygi]

If this is implemented, my vote would be to create IPwmPin/PwmPin similar to what is suggested for IGpioPin/GpioPin.

[shaggygi]

I was thinking about this a little while we were working on I2C/SPI and wanted to see your thoughts before attempting to prototype.

Ultimately the plan would be to have a final structure below. I understand this includes big breaking changes... but better now than never.

Pwm
  Channels                        
    PwmChannel.cs                      // All classes have namespace of 'namespace System.Device.Pwm'
    UnixPwmChannel.Linux.cs            // Internal
    Windows10PwmChannel.Windows.cs     // Internal
    Windows10PwmChip.Windows.cs        // Internal
  IPwmController.cs
  PwmController.cs

Thoughts

• You'd have the option of using a PwmChannel or PwmController.

PwmChannel

Similar to what you mentioned above, but a PwmChannel.

public abstract PwmChannel : IDisposable
{
  protected PwmChannel() {}
  public abstract double FrequencyHz { get; }
  public abstract double DutyCyclePercentage { get; set; } // 0.0 - 1.0
  public static PwmChannel Create(int chip, int channel, double frequencyHz = 50.0, double dutyCyclePercentage = 0.0)
  {
    /* create default pwm device depending on the OS, default values are arbitrary and TBD */
  }
}

PwmChannel.Linux.cs and PwmChannel.Windows.cs would include similar code from UnixPwmDriver.Linux.cs and Windows10PwmDriver.Linux.cs respectively and those related files would be deprecated.

// Create a PWM channel.
PwmChannel pwmChannel = PwmChannel.Create(0, 1);

PwmChip

I haven't studied this too much, but seems reasonable to keep Windows10PwmChip.Windows.cs where it is renamed from Windows10PwmDriverChip.Windows.cs. There might be a need for PwmChip.cs and UnixPwmChip.Linux.cs, as well. Need to review more.

PwmController

Since the OS specific logic is pushed down to the PwmChannel, it seems like you wouldn't need UnixPwmController.Linux.cs or Windows10PwmController.Windows.cs. If needed, users could implement other PWM controllers with IPwmController.

Similar to I2cDevice/SpiDevice, PwmChannel could function without the need of a controller, but I still think one makes sense for pins/channel types. Since PwmChannel is abstract, a user could create their own. For example, there are I2C chips with PWM I/O.

NOTE: I understand the disposing of something that relies on something else, but hopefully the pattern will be determined with #419.

// Very simple example.
var cy8C95xx = new Cy8C95xx(I2cDevice.Create(new I2cConnectionSettings(0, 0x20)));
PwmChannel pwmChannel = cy8C95xx.CreatePwmChannel(1);  Default DC and Freq.

I like the controller in this case as you could make the CY8C95xx implement IGpioController and IPwmController.

var cy8C95xx = new Cy8C95xx(I2cDevice.Create(new I2cConnectionSettings(0, 0x20)));
cy8C95xx.OpenChannel(0, 1);  // Chip and channel.
cy8C95xx.OpenChannel(1); // Or could have it overloaded with just channel.
cy8C95xx.StartWriting(1, 4000, 0.25);
// And GPIO stuff...
cy8C95xx.OpenPin(8, PinMode.Output);
cy8C95xx.Write(8, PinValue.High);

Questions

[Note to self] - As far as I got tonight. Need sleep 😄 😴

[joperezr]

Hey @shaggygi, stepping back a bit I would like to understand what is the big advantage of having PwmChannel class over PwmController. In order to create one, you still need to pass in very similar things, and although I do see that you would benefit of not needing to pass both chip and channel every time that you write or stop writing, I don't see that big of an advantage of having PwmChannel. This is very similar case to the Gpio case. We didn't add a GpioPin class because we thought it didn't carry much state that would be valuable, so we decided to go with the approach of just having the controller and a bit more of a C-like API for it. With PWM we went with the same decision as it doesn't carry much additional state. With I2c and Spi on the other hand, we do have much more state so it does make sense to interact with the device directly instead of the controller. That is at least what we thought of when we designed the API.

[krwq]

@joperezr here is what I think about what should happen here:

• unify all PWM classes into one called PwmChannel/PwmDevice
• make constructor protected and add Create method similarly to SPI
• this class should contain all the info to do PWM without a need for passing anything
• in other words, PWM class should contain only frequency/period and duty cycle

pros of that:

• abstraction is clearer: PWM channel is responsible for controlling pulse modulation on a single pin, there is not much benefit here of having more than 1 PWM, in case of pins you usually use more than 1 so it's more justified
• SoftwarePwm can share identical abstraction - currently SoftPwm implementation is very awkward as base class contains stuff which are irrelevant for SoftPwm - it only needs a pin, frequency and duty cycle - nothing else

[joperezr]

One thing we could do as well, is to keep things as is and simply abstract the concept of channel and chip. that way, SoftwarePWM and HardwarePWM can both just deal with pins, so they would have a common abstraction layer (that may become just an interface). We can then add logic to the hardware PWM to provide mapping between a given pin to a pwm channel and chip.

[shaggygi]

@krwq I think we are somewhat on the same page here when it comes to PwmDevice and PwmChannel. Let's disregard the IPwmController for now (even though my logic may not work when we get back to the controller).

I now think of PwmChannel as the same as I2cDevice and SpiDevice. I like the name PwmChannel a little better as "channel" appears to be a common name in different APIs including WinRT. Channel also seems to fit as you could have a device with multiple channels. This is just an opinion.

When it comes to code, I2cDevice and SpiDevice each have an abstract along with a respective method to Create and respective OS internal class. I'm thinking we could get rid of PwmDriver files and have all the code behind PwmChannel files in a similar fashion.

Therefore, you could have...

// Like
I2cDevice i2cDevice = I2cDevice.Create(new I2cConnectionSettings(1, 1));

// Yes, regarding chip/channel... I don't like having to state I2C bus everytime either :)
PwmChannel pwmChannel = PwmChannel.Create(0, 1, 1000, 0.5);  // Chip 0, channel 1, 1KHz; 50% DC

// Side note, we could even have a PwmSettings and pass it to PwmChannel constructor.
PwmSettings settings = new PwmSettings(0, 1, 1000, 0.5);
PwmChannel pwmChannel = PwmChannel.Create(settings);

And the SoftPwmChannel would be similar as future SoftI2cDevice and SoftSpiDevice. I'm not sure what that would look like (except for additional pin and GpioController), but it would inherit from the PwmChannel. This would be able to pass to bindings and such.

Here's what the structure would look like (without IPwmController files).

Pwm
  Channels                        
    PwmChannel.cs                      // All classes have namespace of 'namespace System.Device.Pwm'
    PwmChannel.Linux.cs
    PwmChannel.Windows.cs
    SoftPwmChannel.cs                  // Future
    UnixPwmChannel.Linux.cs            // Internal
    Windows10PwmChannel.Windows.cs     // Internal
    Windows10PwmChip.Windows.cs        // Internal
  PwmSettings.cs                       // Possible option

@krwq Besides the name, Device vs Channel, (and disregarding the IPwmController for now) does this seem to be inline to your thinking?

If so, we can focus on if/how the IPwmController could be used.

[shaggygi]

@joperezr

big advantage of having PwmChannel class over PwmController.

Honestly, I think I like having the PwmController over PwmChannel. This is more of a discussion on possible solutions if we did create a new PwmDevice or PwmChannel.

One option is to add overloads to the IPwmController without the chip parameter and add a DefaultChip property. Therefore, you'd have the additional...

int DefaultChip { get; }

void OpenChannel(int pwmChannel);
void CloseChannel(int pwmChannel);
void ChangeDutyCycle(int pwmChannel, double dutyCyclePercentage);
void StartWriting(int pwmChannel, double frequencyInHertz, double dutyCyclePercentage);
void StopWriting(int pwmChannel);

I still wonder if the code files/structure could be simplified described above. I need to review some more 🤕

[joperezr]

@krwq and I chatted offline and have come to a decision here in order to make PWM more easy to use and also similar to the approach we took with I2c and Spi. It will also support the Software implementation once we add it in the future. Basically, we would want the API to look like this:

public abstract class PwmChannel
{
  public static PwmChannel Create(int pwmChip, int pwmChannel, double dutyCyclePercentage, double frequency);
  // public static PwmChannel Create(IGpioController controller, int pin, double dutyCyclePercentage, double frequency);    -> This will be added in the future, args can change

  public abstract double DutyCycle { get; set; }
  public abstract double Frequency { get; private set; }

  public abstract void StartWriting();
  public abstract void StopWriting();
}

Apart from this, we would want to change all the rest of the Pwm classes to be internal, similar to what we did with I2c. That way, the Create method will generate a UnixPwmDriver in Linux and a Windows10PwmDriver in Windows. We will want to take this change for 3.0 since we want the new Apis before we ship.

[shaggygi]

@joperezr agreed, i think it basically sums up what i was trying to explain above. #204 (comment)

[shaggygi]

So is the plan to not have a PwmController at all? I could see the CY8C95xx binding implementing an IPwmController. I was partially holding off on #225 until this was determined.

If we don't go with a controller, what would be some options in order to use PwmChannel within a binding like this? Understanding it doesn't have to be answer right away.... just something to think about.

[joperezr]

Yes the idea was to remove PwmController. I'm not familiar with CY8C95xx, is it a PWM expander over I2c? Or is it just a Gpio expander over I2c?

[shaggygi]

It's a mixture of both with I/O, PWM, EEPROM, etc. I started to update the Pca9685 binding when we added the IPwmController, but also held off since this issue came up. Pca9685 basically contains 8 PWM channels.

[joperezr]

I see, well, the plan with the above proposed plan, is that PwmChannel would always represent one specific channel only, so a concept of an expander of several channels wouldn't implement this. That said, the channels that this binding provides could inherit from our PwmChannel class, and they would implement Start and Stop in their own way in order to support PWM on those channels. Would that make sense?

[shaggygi]

I believe so. Let's focus on the initial PwmChannel for now since we think that approach is good. We can circle back on how it will work with bindings when I get some time to focus on them. Related note... I'm not going to mess with the PWM files while working on #565 since they are going to change.

[krwq]

@shaggygi note that DCMotor and SoftPwm might need a bit of re-design as part of this. cc me when you hit any issues, I can fix them up in your PR (assuming you take it).

btw. I think in general we should not have Start/StopWriting methods but I prefer to have this conversation separately after we do initial pass on stuff we agree on

[microhobby]

Sorry I know this is already closed, but I would just like to know if we have something similar to GPIO access, something that returns a "pin" object that have the bind between the bank and line ...

[krwq]

@microhobby unfortunately not at the moment and also not sure if we will have time to address it in 3.0 timeline (also we know of reasonably many people taking dependency on that so not sure it will be a good idea to change it at the moment). We are aware of the problem with multiple gpio controllers touching the same device and we are already planning to show how this can be done.

Specifically I'm planning to create a tiny demo showing how to use i.e. LcdCharacter which has some pins on the GPIO extender and some on RaspberryPi directly.

[microhobby]

@krwq Ok, I understand, if you need any help on this, or when we have any proposal notify me if possible, I will be happy to help