Add DC motor controller #162
Conversation
|
Seems there is one more way to control DC motor which more correct for high amperage: use enable pin to feed PWM and IN1 and IN2 for controlling direction. Need to fix up this PR (https://www.teachmemicro.com/use-l298n-motor-driver/) |
|
|
||
| DC motors can be controlled with 1 or 2 pins: | ||
| - When 1 pin is used the speed is controlled with PWM, second pin is connected to the ground | ||
| - When 2 pins are used 1 of the pins is controlled with PWM and the second controlls the direction (PWM signal has to be inverted when direction is inverted) |
|
Instead of naming it MotorController, can we use the actual chip name/number that the binding supports? Eventually we’ll have mor motor controllers so we should be specific to which ones each binding supports. |
| using System.Device.Pwm; | ||
| using System.Device.Pwm.Drivers; | ||
|
|
||
| namespace Iot.Device |
There was a problem hiding this comment.
Apart from changing the DCMotor name to an actual device that is supported by this binding, we need to include that class name as part of the namespace since all of the drivers go into one bindings dll where we keep them apart by using namespaces.
There was a problem hiding this comment.
DCMotorController works the same for all motors and the only difference is how you connect them but I'm planning to handle this in this class. With Servo I believe we can have both specific and generic version since Servo has some more params
| public class DCMotorController : IDisposable | ||
| { | ||
| private const double PwmFrequency = 50; // Hz | ||
| private PwmController _pwm; |
There was a problem hiding this comment.
readonly? same goes for the GpioController
|
|
||
| public void Dispose() | ||
| { | ||
| _pwm.Dispose(); |
There was a problem hiding this comment.
Add a null check and set them to null after disposing so that multiple Dispose calls on this object would work.
There was a problem hiding this comment.
I thought we already do that inside of the _pwm class but can add that
| /// Initialize a DCMotorController class | ||
| /// </summary> | ||
| /// <param name="pin0">The GPIO pin number used for controlling the speed of the motor.</param> | ||
| /// <param name="pin1">The optional GPIO pin number used for controlling the direction of the motor.</param> |
There was a problem hiding this comment.
Should we add some instruction as well mentioning what would a PinValue.High or a PinValue.Low would mean in terms of direction? Also, should we provide what is the default when null is used?
There was a problem hiding this comment.
I'll actually change this completely. It'll be more like:
int? d0, int? d1, int? enable and this basically will be basically asking the user how did they connect the motor to the H-bridge (naming matches the pins on H-bridge) and depending on how they connected we will internally change behavior to work with that
|
|
||
| _speed = 0; | ||
| _controller = new GpioController(); | ||
| _pwm = new PwmController(new SoftPwm()); |
There was a problem hiding this comment.
Should we allow using hardware PWM as well?
There was a problem hiding this comment.
Feels wasteful considering this will usually work with around 50Hz. Is there any way to detect current pin is hardware PWM? Maybe we should have some kind of AutoPWM class which will auto-pick
There was a problem hiding this comment.
this should be addressed already
|
|
||
| ## Summary | ||
|
|
||
| This is a generic class to control any DC motor. |
There was a problem hiding this comment.
Will it really work for any DC Motor? should we provide a list of devices where we have tested the binding and ensured that it works?
There was a problem hiding this comment.
any motor connected to the H-bridge yes except the stepper motors which work completely differently but uniform across all stepper motors
|
|
||
| It is not recommended to connect any motors directly to the controller and to use H-Bridge instead (i.e. L298N) | ||
|
|
||
| ## Sample |
There was a problem hiding this comment.
This PR should include a Samples project that has this code and it also has a README.md file and a fzz diagram
| /// </summary> | ||
| public class DCMotorSettings | ||
| { | ||
| public int? Pin0 { get; set; } |
There was a problem hiding this comment.
Should Pin0 and Pin1 have more descriptive names? Like what are they connected to for example, or what should each control? In other words, how do I know if one of the pins I'm using is Pin0 or Pin1? Having doc comments on the properties would help with this.
There was a problem hiding this comment.
Swapping data pins will swap the rotation direction and there is not really a good name for that (depending which way the current flows and how much electrons flow you get different direction and speed). Currently it will work regardless of how you connect it (at least that was the intention).
To give more background:
The H-bridge you connect DC motor has 3 pins per motor
- enable where you connect PWM, this disables the output but allowing the motor to still roll without stopping which makes it suitable for controlling the speed
- 2x data which controls the output using potentially different power source - so D0=0; D1=1 will make the motor move one way and D0=1; D1=0 will make it move the other.
optionally you can skip using enable pin (enable=1) and use 2x data pins and use one of them as PWM - that may cause the H-bridge to get a bit warm for larger engines and causes the engine to virtually stop when there is no voltage between PWM and the other pin.
I find this settings class a bit confusing as well but it is as clean as I could come up with until #204 is addressed. If this was fixed the way I proposed I could remove half of the options here and make this much more readable.
I'll make sure to document this in the sample/README but don't have ideas for better name.
|
@joperezr any chance you could take another look? |
| _controller = controller ?? new GpioController(); | ||
| } | ||
|
|
||
| public abstract double Speed { get; set; } |
There was a problem hiding this comment.
Can we add summary comments to all these public facing API? Specifically it would be nice if the comments for properties like this one had the units so we know what we the value of this double means.
There was a problem hiding this comment.
Same comment about adding comment summaries to the public API applies to the rest of your classes, like the settings, etc.
| _controller = null; | ||
| } | ||
|
|
||
| public static DCMotor Create(DCMotorSettings settings) |
There was a problem hiding this comment.
Any particular reason why you wanted to go with the factory method instead of using a constructor that gets the settings passed in?
There was a problem hiding this comment.
There are currently 2 different implementations and implementation depends on the settings (I was wondering if there should be 3 at one point but ended up joining 2 together). I'm almost sure there will be eventually IDCMotor since @Ellerbach's motor implementation abstracted over Spi is a bit similar.
In order to have a constructor I'd have to have a layer of indirection which does not seem necessary since there is very little inconvenience here but a tiny per call perf gain from less indirection
There was a problem hiding this comment.
I would go more for an interface for those kind of classes than code where we'll pass specific elements (GPIO, SPI, I2C, specific). One of the Reason is that in the case of the implementation I've done, the bellow classes even is thay are using SPI and I2C are specific. Calling a function to move a motor is not the same way.
Now, once we have our motors with a common interface, it is pretty easy to have higher level calsses like vehicle which can be fully standardized.
And we can Apply the exact same discussion on Sensors. What should be the interface all sensors should implement so it makes it easier for higher level integration.
There was a problem hiding this comment.
let's have these conversations separately, I think this is out of scope for this PR
|
@joperezr any more feedback or can I merge? |
| /// <summary> | ||
| /// Controller related with operations on pins | ||
| /// </summary> | ||
| protected GpioController Controller; |
There was a problem hiding this comment.
Why is this protected? Do you use it anywhere? If not, this should probably be private and readonly
There was a problem hiding this comment.
Yes, this is used in two of the implementations.
|
this is just rebase (merge conflict) |
No description provided.