-
Notifications
You must be signed in to change notification settings - Fork 606
Fixing DHT issues, adding and adjusting documentation #1164
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
Changes from all commits
4cb18cb
8b713f1
a9437ca
ec3835b
109fd24
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -30,19 +30,19 @@ public abstract class DhtBase : IDisposable | |||||||
protected readonly int _pin; | ||||||||
|
||||||||
/// <summary> | ||||||||
/// I2C device used to communicate with the device | ||||||||
/// True to dispose the Gpio Controller | ||||||||
/// </summary> | ||||||||
protected readonly I2cDevice _i2cDevice; | ||||||||
protected readonly bool _shouldDispose; | ||||||||
|
||||||||
/// <summary> | ||||||||
/// <see cref="GpioController"/> related with the <see cref="_pin"/>. | ||||||||
/// I2C device used to communicate with the device | ||||||||
/// </summary> | ||||||||
protected readonly GpioController _controller; | ||||||||
protected I2cDevice _i2cDevice; | ||||||||
|
||||||||
/// <summary> | ||||||||
/// True to dispose the Gpio Controller | ||||||||
/// <see cref="GpioController"/> related with the <see cref="_pin"/>. | ||||||||
/// </summary> | ||||||||
protected readonly bool _shouldDispose; | ||||||||
protected GpioController _controller; | ||||||||
|
||||||||
// wait about 1 ms | ||||||||
private readonly uint _loopCount = 10000; | ||||||||
|
@@ -58,7 +58,7 @@ public abstract class DhtBase : IDisposable | |||||||
/// Get the last read temperature | ||||||||
/// </summary> | ||||||||
/// <remarks> | ||||||||
/// If last read was not successfull, it returns <code>default(Temperature)</code> | ||||||||
/// If last read was not successful, it returns <code>default(Temperature)</code> | ||||||||
/// </remarks> | ||||||||
public virtual Temperature Temperature | ||||||||
{ | ||||||||
|
@@ -73,7 +73,7 @@ public virtual Temperature Temperature | |||||||
/// Get the last read of relative humidity in percentage | ||||||||
/// </summary> | ||||||||
/// <remarks> | ||||||||
/// If last read was not successfull, it returns <code>default(Ratio)</code> | ||||||||
/// If last read was not successful, it returns <code>default(Ratio)</code> | ||||||||
/// </remarks> | ||||||||
public virtual Ratio Humidity | ||||||||
{ | ||||||||
|
@@ -280,9 +280,21 @@ public void Dispose() | |||||||
if (_shouldDispose) | ||||||||
{ | ||||||||
_controller?.Dispose(); | ||||||||
_controller = null; | ||||||||
} | ||||||||
else | ||||||||
{ | ||||||||
if (_controller != null) | ||||||||
{ | ||||||||
if (_controller.IsPinOpen(_pin)) | ||||||||
{ | ||||||||
_controller.ClosePin(_pin); | ||||||||
} | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
_i2cDevice?.Dispose(); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if it's readonly, what is the advantage of making it non readonly and setting it up to null? Also if setup to null, it will have to go to line 287, not here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because if it is not reset to null, the change to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But in this case, we want to close the pin and not dispose the controller There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, I didn't say it should be disposed in the second case. It should be set to null, but in both cases. |
||||||||
_i2cDevice = null; | ||||||||
} | ||||||||
} | ||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,29 +15,109 @@ internal class Program | |
public static void Main(string[] args) | ||
{ | ||
Console.WriteLine("Hello DHT!"); | ||
Console.WriteLine("Select the DHT sensor you want to use:"); | ||
Console.WriteLine(" 1. DHT10 on I2C"); | ||
Console.WriteLine(" 2. DHT11 on GPIO"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. :+1, Good example code |
||
Console.WriteLine(" 3. DHT12 on GPIO"); | ||
Console.WriteLine(" 4. DHT21 on GPIO"); | ||
Console.WriteLine(" 5. DHT22 on GPIO"); | ||
var choice = Console.ReadKey(); | ||
Console.WriteLine(); | ||
if (choice.KeyChar == '1') | ||
{ | ||
Console.WriteLine("Press any key to stop the reading"); | ||
// Init DHT10 through I2C | ||
I2cConnectionSettings settings = new I2cConnectionSettings(1, Dht10.DefaultI2cAddress); | ||
I2cDevice device = I2cDevice.Create(settings); | ||
|
||
// Init DHT10 through I2C | ||
I2cConnectionSettings settings = new I2cConnectionSettings(1, Dht10.DefaultI2cAddress); | ||
I2cDevice device = I2cDevice.Create(settings); | ||
using (Dht10 dht = new Dht10(device)) | ||
{ | ||
Dht(dht); | ||
} | ||
|
||
using (Dht10 dht = new Dht10(device)) | ||
return; | ||
} | ||
|
||
Console.WriteLine("Which pin do you want to use in the logical pin schema?"); | ||
var pinChoise = Console.ReadLine(); | ||
int pin; | ||
try | ||
{ | ||
while (true) | ||
{ | ||
var tempValue = dht.Temperature; | ||
var humValue = dht.Humidity; | ||
pin = Convert.ToInt32(pinChoise); | ||
} | ||
catch (Exception ex) when (ex is FormatException || ex is OverflowException) | ||
{ | ||
Console.WriteLine("Can't convert pin number."); | ||
return; | ||
} | ||
|
||
Console.WriteLine("Press any key to stop the reading"); | ||
|
||
switch (choice.KeyChar) | ||
{ | ||
case '2': | ||
Console.WriteLine($"Reading temperature and humidity on DHT11, pin {pin}"); | ||
using (var dht11 = new Dht11(pin)) | ||
{ | ||
Dht(dht11); | ||
} | ||
|
||
break; | ||
case '3': | ||
Console.WriteLine($"Reading temperature and humidity on DHT12, pin {pin}"); | ||
using (var dht12 = new Dht12(pin)) | ||
{ | ||
Dht(dht12); | ||
} | ||
|
||
Console.WriteLine($"Temperature: {tempValue.DegreesCelsius:0.#}\u00B0C"); | ||
Console.WriteLine($"Relative humidity: {humValue:0.#}%"); | ||
break; | ||
case '4': | ||
Console.WriteLine($"Reading temperature and humidity on DHT21, pin {pin}"); | ||
using (var dht21 = new Dht21(pin)) | ||
{ | ||
Dht(dht21); | ||
} | ||
|
||
break; | ||
case '5': | ||
Console.WriteLine($"Reading temperature and humidity on DHT22, pin {pin}"); | ||
using (var dht22 = new Dht22(pin)) | ||
{ | ||
Dht(dht22); | ||
} | ||
|
||
break; | ||
default: | ||
Console.WriteLine("Please select one of the option."); | ||
break; | ||
} | ||
} | ||
|
||
private static void Dht(DhtBase dht) | ||
{ | ||
while (!Console.KeyAvailable) | ||
{ | ||
var temp = dht.Temperature; | ||
var hum = dht.Humidity; | ||
// You can only display temperature and humidity if the read is successful otherwise, this will raise an exception as | ||
// both temperature and humidity are NAN | ||
if (dht.IsLastReadSuccessful) | ||
{ | ||
Console.WriteLine($"Temperature: {temp.DegreesCelsius}\u00B0C, Relative humidity: {hum.Percent}%"); | ||
|
||
// WeatherHelper supports more calculations, such as saturated vapor pressure, actual vapor pressure and absolute humidity. | ||
Console.WriteLine( | ||
$"Heat index: {WeatherHelper.CalculateHeatIndex(tempValue, humValue).DegreesCelsius:0.#}\u00B0C"); | ||
$"Heat index: {WeatherHelper.CalculateHeatIndex(temp, hum).DegreesCelsius:0.#}\u00B0C"); | ||
Console.WriteLine( | ||
$"Dew point: {WeatherHelper.CalculateDewPoint(tempValue, humValue).DegreesCelsius:0.#}\u00B0C"); | ||
|
||
Thread.Sleep(2000); | ||
$"Dew point: {WeatherHelper.CalculateDewPoint(temp, hum).DegreesCelsius:0.#}\u00B0C"); | ||
} | ||
else | ||
{ | ||
Console.WriteLine("Error reading DHT sensor"); | ||
} | ||
|
||
// You must wait some time before trying to read the next value | ||
Thread.Sleep(2000); | ||
} | ||
} | ||
} | ||
|
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.
Sorry to insist: this needs to be outside the if (or in both cases). Otherwise, if
shouldDispose
is false, this still bombs on the secondDispose
call.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.
If
_shouldDispose
is false, the key principal is not to dispose. As an example, with the FT4222, you can only create 1 GPIO controller. If you have 2 devices which are using GPIO, and the first one dispose it, then the second one will raise an exception.So it's important not to dispose at all the GPIO controller if the ```shouldDispose```` flag is false.
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.
True. I'm not talking about disposing, I'm talking about setting the controller to null. This doesn't do an implicit dispose or something.
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.
@pgrawehr I think I really miss something then. Can you please then write the full dispose class how you think it should be for me to understand what you mean.
Again, here is the principle:
To prevent some mistakes:
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'll do that. It really looks like we're talking different things. But give me some time, as you probably know, I'm traveling around, and good WiFi is hard to find.
Uh oh!
There was an error while loading. Please reload this page.
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.
Here's a patch file for your branch. I've also fixed a few minor other issues I just saw. In addition to your list above (which are all sattisfied), this
0001-Make-sure-Dispose-can-be-safely-called-multiple-time.txt
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.
Thanks, I understand now. So then, rather than raising an exception, why not then use the other pattern, for example present in some of the classes like the Explorer Hat:
iot/src/devices/ExplorerHat/Motors.cs
Line 87 in 3c40347
So having a variable to avoid double disposing? Advantage: it doesn't raise an exception. I'm not much in favor of raising exceptions in the Dispose function. Those are exceptions usually no one really trap.
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.
The exception is not thrown in Dispose (thats what we try to avoid), but if the instance is otherwise used after Disposal, i.e when trying to read from the sensor after disposing.
An ObjectDisposedException is rightly (almost) never handled in user code, since that is a programming error, and they should stop the application typically in the debugger.
Of course the same as what I did can be achieved by an extra bool variable, but that doesn't change much conceptionally.
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.
OK, so I adjusted only the dispose part to make sure that is multiple dispose are called, it won't create any issue.
In case of a programming error and the class is called once disposed, an exception will be raised anyway.
So it keeps the solution simple enough and easy to read. I hope it's a good compromise.
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 would still set it to null, but I'm ok with this as well.