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.

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

New Dallas temperature sensor example #22

wants to merge 4 commits into from

Conversation

flatsiedatsie
Copy link
Contributor

After some useful feedback and co-authoring on the forum (Thanks Scalz!), 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). Thanks Awi!
  • 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.

Forum thread:
https://forum.mysensors.org/topic/6395/new-non-blocking-temperature-sensor-code/7

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.
Copy link
Member

@henrikekblad henrikekblad left a 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 )
Copy link
Member

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.

Copy link
Contributor Author

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++)
{
Copy link
Member

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.

Copy link
Contributor Author

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.
Copy link
Member

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.

Copy link
Contributor Author

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.
Copy link
Member

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.

Copy link
Contributor Author

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.
Copy link
Member

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.

Copy link
Contributor Author

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 .

@henrikekblad
Copy link
Member

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.

@flatsiedatsie
Copy link
Contributor Author

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.
@flatsiedatsie
Copy link
Contributor Author

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.
@henrikekblad
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants