Skip to content
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.

Sign up for GitHub

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

Fixing missing GpioController and ShouldDispose #1016

Merged
merged 6 commits into from
Apr 27, 2020
Merged

Fixing missing GpioController and ShouldDispose #1016

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2f4d54b
Fixing missing GpioController and shouldDispose
Ellerbach Mar 19, 2020
bcf9769
Adjusting DC Motor
Ellerbach Mar 20, 2020
a4972d2
Adjusting and fixing dispose elements based on feedbacks
Ellerbach Mar 26, 2020
a8d9352
Adjusting dispose in Led, Lights and Motors after PR comments
Ellerbach Mar 30, 2020
e80bd1a
Adding off in dispose for LedArray
Ellerbach Apr 1, 2020
871318c
Adjusting dispose in Lights
Ellerbach Apr 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/devices/CharacterLcd/Lcd1602.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,17 @@ public class Lcd1602 : Hd44780
/// <param name="backlightBrightness">The brightness of the backlight. 0.0 for off, 1.0 for on.</param>
/// <param name="readWritePin">The optional pin that controls the read and write switch.</param>
/// <param name="controller">The controller to use with the LCD. If not specified, uses the platform default.</param>
public Lcd1602(int registerSelectPin, int enablePin, int[] dataPins, int backlightPin = -1, float backlightBrightness = 1.0f, int readWritePin = -1, GpioController controller = null)
: base(new Size(16, 2), LcdInterface.CreateGpio(registerSelectPin, enablePin, dataPins, backlightPin, backlightBrightness, readWritePin, controller))
/// <param name="shouldDispose">True to dispose the Gpio Controller</param>
public Lcd1602(int registerSelectPin, int enablePin, int[] dataPins, int backlightPin = -1, float backlightBrightness = 1.0f, int readWritePin = -1, GpioController controller = null, bool shouldDispose = true)
: base(new Size(16, 2), LcdInterface.CreateGpio(registerSelectPin, enablePin, dataPins, backlightPin, backlightBrightness, readWritePin, controller, shouldDispose))
{
}

/// <summary>
/// Constructs a new HD44780 based 16x2 LCD controller with integrated I2c support.
/// </summary>
/// <remarks>
/// This is for on-chip I2c support. For connecting via I2c GPIO expanders, use the GPIO constructor <see cref="Lcd1602(int, int, int[], int, float, int, GpioController)"/>.
/// This is for on-chip I2c support. For connecting via I2c GPIO expanders, use the GPIO constructor <see cref="Lcd1602(int, int, int[], int, float, int, GpioController, bool)"/>.
/// </remarks>
/// <param name="device">The I2c device for the LCD.</param>
/// <param name="uses8Bit">True if the device uses 8 Bit commands, false if it handles only 4 bit commands.</param>
Expand Down
7 changes: 4 additions & 3 deletions src/devices/CharacterLcd/Lcd2004.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,17 @@ public class Lcd2004 : Hd44780
/// <param name="backlightBrightness">The brightness of the backlight. 0.0 for off, 1.0 for on.</param>
/// <param name="readWritePin">The optional pin that controls the read and write switch.</param>
/// <param name="controller">The controller to use with the LCD. If not specified, uses the platform default.</param>
public Lcd2004(int registerSelectPin, int enablePin, int[] dataPins, int backlightPin = -1, float backlightBrightness = 1.0f, int readWritePin = -1, GpioController controller = null)
: base(new Size(20, 4), LcdInterface.CreateGpio(registerSelectPin, enablePin, dataPins, backlightPin, backlightBrightness, readWritePin, controller))
/// <param name="shouldDispose">True to dispose the Gpio Controller</param>
public Lcd2004(int registerSelectPin, int enablePin, int[] dataPins, int backlightPin = -1, float backlightBrightness = 1.0f, int readWritePin = -1, GpioController controller = null, bool shouldDispose = true)
: base(new Size(20, 4), LcdInterface.CreateGpio(registerSelectPin, enablePin, dataPins, backlightPin, backlightBrightness, readWritePin, controller, shouldDispose))
{
}

/// <summary>
/// Constructs a new HD44780 based 16x2 LCD controller with integrated I2c support.
/// </summary>
/// <remarks>
/// This is for on-chip I2c support. For connecting via I2c GPIO expanders, use the GPIO constructor <see cref="Lcd1602(int, int, int[], int, float, int, GpioController)"/>.
/// This is for on-chip I2c support. For connecting via I2c GPIO expanders, use the GPIO constructor <see cref="Lcd1602(int, int, int[], int, float, int, GpioController, bool)"/>.
/// </remarks>
/// <param name="device">The I2c device for the LCD.</param>
/// <param name="uses8Bit">True if the device uses 8 Bit commands, false if it handles only 4 bit commands.</param>
Expand Down
11 changes: 9 additions & 2 deletions src/devices/CharacterLcd/LcdInterface.Gpio.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,10 @@ private class Gpio : LcdInterface
private bool _useLastByte;

private GpioController _controller;
private bool _shouldDispose;
private PinValuePair[] _pinBuffer = new PinValuePair[8];

public Gpio(int registerSelectPin, int enablePin, int[] dataPins, int backlightPin = -1, float backlightBrightness = 1.0f, int readWritePin = -1, GpioController controller = null)
public Gpio(int registerSelectPin, int enablePin, int[] dataPins, int backlightPin = -1, float backlightBrightness = 1.0f, int readWritePin = -1, GpioController controller = null, bool shouldDispose = true)
{
_rwPin = readWritePin;
_rsPin = registerSelectPin;
Expand All @@ -65,6 +66,7 @@ public Gpio(int registerSelectPin, int enablePin, int[] dataPins, int backlightP
throw new ArgumentException($"The length of the array given to parameter {nameof(dataPins)} must be 4 or 8");
}

_shouldDispose = controller == null ? true : shouldDispose;
_controller = controller ?? new GpioController(PinNumberingScheme.Logical);

Initialize();
Expand Down Expand Up @@ -242,7 +244,12 @@ private void WriteBits(byte bits, int count)

protected override void Dispose(bool disposing)
{
_controller?.Dispose();
if (_shouldDispose)
{
_controller?.Dispose();
_controller = null;
}

base.Dispose(disposing);
}
}
Expand Down
7 changes: 4 additions & 3 deletions src/devices/CharacterLcd/LcdInterface.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,16 +121,17 @@ public void Dispose()
/// <param name="backlightBrightness">The brightness of the backlight. 0.0 for off, 1.0 for on.</param>
/// <param name="readWritePin">The optional pin that controls the read and write switch.</param>
/// <param name="controller">The controller to use with the LCD. If not specified, uses the platform default.</param>
public static LcdInterface CreateGpio(int registerSelectPin, int enablePin, int[] dataPins, int backlightPin = -1, float backlightBrightness = 1.0f, int readWritePin = -1, GpioController controller = null)
/// <param name="shouldDispose">True to dispose the Gpio Controller</param>
public static LcdInterface CreateGpio(int registerSelectPin, int enablePin, int[] dataPins, int backlightPin = -1, float backlightBrightness = 1.0f, int readWritePin = -1, GpioController controller = null, bool shouldDispose = true)
{
return new Gpio(registerSelectPin, enablePin, dataPins, backlightPin, backlightBrightness, readWritePin, controller);
return new Gpio(registerSelectPin, enablePin, dataPins, backlightPin, backlightBrightness, readWritePin, controller, shouldDispose);
}

/// <summary>
/// Create an integrated I2c based interface for the LCD.
/// </summary>
/// <remarks>
/// This is for on-chip I2c support. For connecting via I2c GPIO expanders, use the GPIO interface <see cref="CreateGpio(int, int, int[], int, float, int, GpioController)"/>.
/// This is for on-chip I2c support. For connecting via I2c GPIO expanders, use the GPIO interface <see cref="CreateGpio(int, int, int[], int, float, int, GpioController, bool)"/>.
/// </remarks>
/// <param name="device">The I2c device for the LCD.</param>
/// <param name="uses8Bit">True if the device uses 8 Bit commands, false if it handles only 4 bit commands.</param>
Expand Down
43 changes: 29 additions & 14 deletions src/devices/DCMotor/DCMotor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,16 @@ namespace Iot.Device.DCMotor
public abstract class DCMotor : IDisposable
{
private const int DefaultPwmFrequency = 50;
private bool _shouldDispose;

/// <summary>
/// Constructs generic <see cref="DCMotor"/> instance
/// </summary>
/// <param name="controller"><see cref="GpioController"/> related with operations on pins</param>
protected DCMotor(GpioController controller)
/// <param name="shouldDispose">True to dispose the Gpio Controller</param>
protected DCMotor(GpioController controller, bool shouldDispose)
{
_shouldDispose = shouldDispose;
Controller = controller;
}

Expand Down Expand Up @@ -57,8 +60,11 @@ protected virtual void Dispose(bool disposing)
{
if (disposing)
{
Controller?.Dispose();
Controller = null;
if (_shouldDispose)
{
Controller?.Dispose();
Controller = null;
}
}
}

Expand All @@ -79,21 +85,22 @@ public static DCMotor Create(PwmChannel speedControlChannel)
throw new ArgumentNullException(nameof(speedControlChannel));
}

return new DCMotor2PinNoEnable(speedControlChannel, -1, null);
return new DCMotor2PinNoEnable(speedControlChannel, -1, null, true);
}

/// <summary>
/// Creates <see cref="DCMotor"/> instance which allows to control speed in one direction.
/// </summary>
/// <param name="speedControlPin">Pin used to control the speed of the motor with software PWM (frequency will default to 50Hz)</param>
/// <param name="controller"><see cref="GpioController"/> related to the <paramref name="speedControlPin"/></param>
/// <param name="shouldDispose">True to dispose the Gpio Controller</param>
/// <returns><see cref="DCMotor"/> instance</returns>
/// <remarks>
/// <paramref name="speedControlPin"/> can be connected to either enable pin of the H-bridge.
/// or directly to the input related with the motor (if H-bridge allows inputs to change frequently).
/// Connecting motor directly to GPIO pin is not recommended and may damage your board.
/// </remarks>
public static DCMotor Create(int speedControlPin, GpioController controller = null)
public static DCMotor Create(int speedControlPin, GpioController controller = null, bool shouldDispose = true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait with your latest changes you are not really disposing anything right? It looks like you are only taking in the extra parameter but not really assigning to anything, and also not checking it before disposing.

{
if (speedControlPin == -1)
{
Expand All @@ -104,7 +111,8 @@ public static DCMotor Create(int speedControlPin, GpioController controller = nu
return new DCMotor2PinNoEnable(
new SoftwarePwmChannel(speedControlPin, DefaultPwmFrequency, 0.0, controller: controller),
-1,
controller);
controller,
shouldDispose);
}

/// <summary>
Expand All @@ -113,14 +121,15 @@ public static DCMotor Create(int speedControlPin, GpioController controller = nu
/// <param name="speedControlChannel"><see cref="PwmChannel"/> used to control the speed of the motor</param>
/// <param name="directionPin">Pin used to control the direction of the motor</param>
/// <param name="controller"><see cref="GpioController"/> related to the <paramref name="directionPin"/></param>
/// <param name="shouldDispose">True to dispose the Gpio Controller</param>
/// <returns><see cref="DCMotor"/> instance</returns>
/// <remarks>
/// <paramref name="speedControlChannel"/> can be connected to either enable pin of the H-bridge.
/// or directly to the input related with the motor (if H-bridge allows inputs to change frequently).
/// <paramref name="directionPin"/> should be connected to H-bridge input corresponding to one of the motor inputs.
/// Connecting motor directly to GPIO pin is not recommended and may damage your board.
/// </remarks>
public static DCMotor Create(PwmChannel speedControlChannel, int directionPin, GpioController controller = null)
public static DCMotor Create(PwmChannel speedControlChannel, int directionPin, GpioController controller = null, bool shouldDispose = true)
{
if (speedControlChannel == null)
{
Expand All @@ -132,7 +141,7 @@ public static DCMotor Create(PwmChannel speedControlChannel, int directionPin, G
throw new ArgumentOutOfRangeException(nameof(directionPin));
}

return new DCMotor2PinNoEnable(speedControlChannel, directionPin, controller);
return new DCMotor2PinNoEnable(speedControlChannel, directionPin, controller, shouldDispose);
}

/// <summary>
Expand All @@ -141,14 +150,15 @@ public static DCMotor Create(PwmChannel speedControlChannel, int directionPin, G
/// <param name="speedControlPin">Pin used to control the speed of the motor with software PWM (frequency will default to 50Hz)</param>
/// <param name="directionPin">Pin used to control the direction of the motor</param>
/// <param name="controller">GPIO controller related to <paramref name="speedControlPin"/> and <paramref name="directionPin"/></param>
/// <param name="shouldDispose">True to dispose the Gpio Controller</param>
/// <returns><see cref="DCMotor"/> instance</returns>
/// <remarks>
/// PWM pin <paramref name="speedControlPin"/> can be connected to either enable pin of the H-bridge.
/// or directly to the input related with the motor (if H-bridge allows inputs to change frequently).
/// <paramref name="directionPin"/> should be connected to H-bridge input corresponding to one of the motor inputs.
/// Connecting motor directly to GPIO pin is not recommended and may damage your board.
/// </remarks>
public static DCMotor Create(int speedControlPin, int directionPin, GpioController controller = null)
public static DCMotor Create(int speedControlPin, int directionPin, GpioController controller = null, bool shouldDispose = true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as the CharacterLCd here, we should probably have a Create Overload that doesn't take a shouldDispose, and set it to false in case controller was passed in. Another way to do this is to default it to false instead, and only set it to true if the controller that was passed in was null.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment then for the previous one, it's more about consistency.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I'm fine with keeping it as is for consistency

{
if (speedControlPin == -1)
{
Expand All @@ -164,7 +174,8 @@ public static DCMotor Create(int speedControlPin, int directionPin, GpioControll
return new DCMotor2PinNoEnable(
new SoftwarePwmChannel(speedControlPin, DefaultPwmFrequency, 0.0, controller: controller),
directionPin,
controller);
controller,
shouldDispose);
}

/// <summary>
Expand All @@ -174,6 +185,7 @@ public static DCMotor Create(int speedControlPin, int directionPin, GpioControll
/// <param name="directionPin">First pin used to control the direction of the motor</param>
/// <param name="otherDirectionPin">Second pin used to control the direction of the motor</param>
/// <param name="controller"><see cref="GpioController"/> related to <paramref name="directionPin"/> and <paramref name="otherDirectionPin"/></param>
/// <param name="shouldDispose">True to dispose the Gpio Controller</param>
/// <returns><see cref="DCMotor"/> instance</returns>
/// <remarks>
/// When speed is non-zero the value of <paramref name="otherDirectionPin"/> will always be opposite to that of <paramref name="directionPin"/>.
Expand All @@ -182,7 +194,7 @@ public static DCMotor Create(int speedControlPin, int directionPin, GpioControll
/// <paramref name="otherDirectionPin"/> should be connected to H-bridge input corresponding to the remaining motor input.
/// Connecting motor directly to GPIO pin is not recommended and may damage your board.
/// </remarks>
public static DCMotor Create(PwmChannel speedControlChannel, int directionPin, int otherDirectionPin, GpioController controller = null)
public static DCMotor Create(PwmChannel speedControlChannel, int directionPin, int otherDirectionPin, GpioController controller = null, bool shouldDispose = true)
{
if (speedControlChannel == null)
{
Expand All @@ -203,7 +215,8 @@ public static DCMotor Create(PwmChannel speedControlChannel, int directionPin, i
speedControlChannel,
directionPin,
otherDirectionPin,
controller);
controller,
shouldDispose);
}

/// <summary>
Expand All @@ -213,6 +226,7 @@ public static DCMotor Create(PwmChannel speedControlChannel, int directionPin, i
/// <param name="directionPin">First pin used to control the direction of the motor</param>
/// <param name="otherDirectionPin">Second pin used to control the direction of the motor</param>
/// <param name="controller"><see cref="GpioController"/> related to <paramref name="speedControlPin"/>, <paramref name="directionPin"/> and <paramref name="otherDirectionPin"/></param>
/// <param name="shouldDispose">True to dispose the Gpio Controller</param>
/// <returns><see cref="DCMotor"/> instance</returns>
/// <remarks>
/// When speed is non-zero the value of <paramref name="otherDirectionPin"/> will always be opposite to that of <paramref name="directionPin"/>
Expand All @@ -221,7 +235,7 @@ public static DCMotor Create(PwmChannel speedControlChannel, int directionPin, i
/// <paramref name="otherDirectionPin"/> should be connected to H-bridge input corresponding to the remaining motor input.
/// Connecting motor directly to GPIO pin is not recommended and may damage your board.
/// </remarks>
public static DCMotor Create(int speedControlPin, int directionPin, int otherDirectionPin, GpioController controller = null)
public static DCMotor Create(int speedControlPin, int directionPin, int otherDirectionPin, GpioController controller = null, bool shouldDispose = true)
{
if (speedControlPin == -1)
{
Expand All @@ -243,7 +257,8 @@ public static DCMotor Create(int speedControlPin, int directionPin, int otherDir
new SoftwarePwmChannel(speedControlPin, DefaultPwmFrequency, 0.0, controller: controller),
directionPin,
otherDirectionPin,
controller);
controller,
shouldDispose);
}
}
}
5 changes: 3 additions & 2 deletions src/devices/DCMotor/DCMotor2PinNoEnable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@ internal class DCMotor2PinNoEnable : DCMotor
public DCMotor2PinNoEnable(
PwmChannel pwmChannel,
int pin1,
GpioController controller)
: base(controller ?? ((pin1 == -1) ? null : new GpioController()))
GpioController controller,
bool shouldDispose)
: base(controller ?? ((pin1 == -1) ? null : new GpioController()), controller == null ? true : shouldDispose)
{
_pwm = pwmChannel;

Expand Down
5 changes: 3 additions & 2 deletions src/devices/DCMotor/DCMotor3Pin.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@ public DCMotor3Pin(
PwmChannel pwmChannel,
int pin0,
int pin1,
GpioController controller)
: base(controller ?? new GpioController())
GpioController controller,
bool shouldDispose)
: base(controller ?? new GpioController(), controller == null ? true : shouldDispose)
{
if (pwmChannel == null)
{
Expand Down
6 changes: 4 additions & 2 deletions src/devices/Dhtxx/Devices/Dht11.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@ public class Dht11 : DhtBase
/// </summary>
/// <param name="pin">The pin number (GPIO number)</param>
/// <param name="pinNumberingScheme">The GPIO pin numbering scheme</param>
public Dht11(int pin, PinNumberingScheme pinNumberingScheme = PinNumberingScheme.Logical)
: base(pin, pinNumberingScheme)
/// <param name="gpioController"><see cref="GpioController"/> related with operations on pins</param>
/// <param name="shouldDispose">True to dispose the Gpio Controller</param>
public Dht11(int pin, PinNumberingScheme pinNumberingScheme = PinNumberingScheme.Logical, GpioController gpioController = null, bool shouldDispose = true)
: base(pin, pinNumberingScheme, gpioController, shouldDispose)
{
}

Expand Down
Loading