New Dallas temperature sensor example#22
New Dallas temperature sensor example#22flatsiedatsie wants to merge 4 commits intomysensors:masterfrom flatsiedatsie:patch-2
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.
henrikekblad
left a comment
henrikekblad
left a comment
There was a problem hiding this comment.
Added a few comments.
Cheers
Henrik
There was a problem hiding this comment.
This won't work when millis wrap(). Do the same type of check as above.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Bracket placement does not confirm with coding standard.
There was a problem hiding this comment.
Yep, misunderstood the standard. fixed.
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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.
I don't know what you mean. Would it be ok not to? :-D
There was a problem hiding this comment.
No need to make this static as it will be initialized to millis() a few lines down.
There was a problem hiding this comment.
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. |
2 participants
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