-
Notifications
You must be signed in to change notification settings - Fork 277
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
New Dallas temperature sensor example #22
base: master
Are you sure you want to change the base?
Conversation
After some useful feedback and co-authoring on the forum, the sketch is now even better. - Conforms better to MySensors coding standards. - Added option to choose sensor precision (also useful starting point for other sensors). - Clearer option to choose between Celsius and Fahrenheit. Old features remain: - non-blocking - option to use sleep to preserve battery - Now compatible with latest OneWire and Dallas libraries - Lots more explanatory/educational comments, and cleaner code than before.
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.
Added a few comments.
Cheers
Henrik
|
||
|
||
// Next, let's calculate and send the temperature | ||
if(isCalculating == true && currentMillis > previousMeasurementMillis + conversionTime ) |
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.
This won't work when millis wrap(). Do the same type of check as above.
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 didn't know that, interesting. I had read that during a millis wrap the subtract function still works. But I now realise that that won't work in this case. Very sharp! fixed.
sendSketchInfo("Temperature Sensor", "1.2"); // Send the sketch version information to the gateway and Controller | ||
numSensors = sensors.getDeviceCount(); // Fetch the number of attached temperature sensors | ||
for (int i=0; i<numSensors && i<maxAttachedDS18B20; i++) | ||
{ |
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.
Bracket placement does not confirm with coding standard.
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.
Yep, misunderstood the standard. fixed.
for (int i=0; i<numSensors && i<maxAttachedDS18B20; i++) // Loop through all the attached temperature sensors. | ||
{ | ||
|
||
#ifdef FAHRENHEIT // Arduino's pre-processor selects which code gets added to the sensor based on which define is set in the settings at the top of this sketch. |
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.
This would hard-code conversion setting. The examples normally uses
getControllerConfig().isMetric
Which allows you to change setting from controller.
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.
Ah, that explains why that was in there. I'm putting it back. Fixed.
Out of curiosity: the original sketch used the booleans receivedConfig and metric
Are they required by Mysensors? Because the ReceivedConfig variable doesn't seem to be actually used in the sketch. I've left them in for now.
Serial.print(" says it is "); | ||
Serial.print(temperature); | ||
Serial.print(" degrees\n"); | ||
if(temperature != -127.00 && temperature != 85.00) // Avoids working with measurement errors. |
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.
Maybe we should use:
DEVICE_DISCONNECTED_C
or
DEVICE_DISCONNECTED_F
defined in DallasTemperature.h
which one depends on if getTempCByIndex / getTempFByIndex was called.
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 don't know what you mean. Would it be ok not to? :-D
// You should not change these variables: | ||
static boolean isMeasuring = true; // Used to indicate when the time is right for a new measurement to be made. | ||
static boolean isCalculating = false; // Used to bridge the time that is needed to calculate the temperature values by the Dallas library. | ||
static unsigned long currentMillis = 0; // The millisecond clock in the main loop. |
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.
No need to make this static as it will be initialized to millis() a few lines down.
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.
Not my code :-) but fixed .
Also note that you don't need to close/open new PR. You can amend the current one by pushing to the same branch. That way we keep all the comments and history related to the update in one place. If the DallasTemperature library is no longer needed you can remove it as well. |
OK, I'll try adding it to the current request. So.. much.. learning! |
Added one space :-) Forgot to mention details about the previous commit: it changed a number of things based on feedback from Hek. Including spotting a millis wrap bug, very impressive.
Do I need to set this back to "please review" somehow? |
this follows some conventions which may make it slightly easier to integrate it with other sensor modules.
Could we perhaps make a new example with your changes? I.e. DallasTemperatureSensorNoneBlocking.ino (or shorter if you can come up with a nifty name ;) ) I'd like to keep the current one as simple as possible. |
After some useful feedback and co-authoring on the forum (Thanks Scalz!), the sketch is now even better.
Old features remain:
Forum thread:
https://forum.mysensors.org/topic/6395/new-non-blocking-temperature-sensor-code/7