5
\$\begingroup\$

Python Beginner building an automation irrigation system in Raspberry Pi.

The Python has two functions:

  1. Single wire temperature sensor (DS18B20) that prints/monitors temperature
  2. Turn on LED (will be relay switch)

The idea is when the temperature gets too hot, it will turn on the switch/Led that will turn on the air conditioning.

Looking on advice in two capacities:

  1. To make it cleaner/better code and more efficient
  2. How can I use "Temperature" as a global variable so I can use it on other things like a screen/LCD
#import modules

import time
import board
import busio
import digitalio
from board import *

from datetime import date


#DigitalIO and Pin setup
tempLed = digitalio.DigitalInOut(D17)                  #PIN LED for too hot sensor. 
tempLed.direction = digitalio.Direction.OUTPUT

tempSensor = digitalio.DigitalInOut(D14)              #Temp sensor DS18B20 as configured in terminal
tempSensor.switch_to_input(pull=digitalio.Pull.UP)
tempSensor.pull = digitalio.Pull.UP


#main loop
try:
    while True:
        tempStore = open("/sys/bus/w1/devices/28-3cffe076cfcf/w1_slave")    #change this number to the Device ID of your sensor
        data = tempStore.read()
        tempStore.close()
        tempData = data.split("\n")[1].split(" ")[9]
        temperature = float(tempData[2:])
        temperature = temperature/1000
        print(temperature)

        if temperature > 24:    #change this value to adjust the 'too hot' threshold
            tempLed.value = True
        else: 
            tempLed.value = False

        time.sleep(1)


except KeyboardInterrupt:
    digitalio.cleanup() 
    print ("Program Exited Cleanly")

Sample data:

70 01 55 05 7f a5 81 66 3d : crc=3d YES
70 01 55 05 7f a5 81 66 3d t=23000
\$\endgroup\$
3
  • 2
    \$\begingroup\$ Your data.split("\n")[1].split(" ")[9] operation extracts the 10th space-separated word from the second line of data. We can't tell how robust that extraction process is without seeing what the sensor returns. Could you provide a sample of the actual data that gets returned from the sensor? \$\endgroup\$
    – AJNeufeld
    Aug 10 at 15:08
  • \$\begingroup\$ @AJNeufeld - thanks for info; this is data extracted for temperature = 23. First example is data = 70 01 55 05 7f a5 81 66 3d : crc=3d YES 70 01 55 05 7f a5 81 66 3d t=23000 and this is tempData t=23000 \$\endgroup\$
    – GlennB
    Aug 11 at 14:20
  • \$\begingroup\$ I’ve added the sample data to the question post. Please confirm it was formatted properly, by editing it if it was not. \$\endgroup\$
    – AJNeufeld
    Aug 12 at 1:19
10
\$\begingroup\$

I'm going to assume that your device has a sysfs interface identical to that described in this guide, which based on your code is likely the case.

How can I use "Temperature" as a global variable

is the opposite direction to what you should do. Currently all of your code is global, but you should put some effort into making it re-entrant.

Other than your global code, one thing that stands out is the non-guaranteed close call, which should be replaced by a with statement.

You should not need to read() the entire file. Instead, read line-by-line until you find a match.

    if temperature > 24:    #change this value to adjust the 'too hot' threshold
        tempLed.value = True
    else: 
        tempLed.value = False

should be reduced to

temp_led.value = temperature > threshold

where threshold is a parameter instead of being hard-coded, and the variable names are PEP8-compliant.

except KeyboardInterrupt:
    digitalio.cleanup() 
    print ("Program Exited Cleanly")

should instead be

except KeyboardInterrupt:
    pass
finally:
    digitalio.cleanup() 
    print ("Program Exited Cleanly")

In other words, you should clean up regardless of why and how the program exited.

Don't hard-code the device ID - if you can assume that there's only one such sensor, use glob-matching instead.

Suggested (not tested)

class TempSensor:
    def __init__(self):
        sensor = digitalio.DigitalInOut(D14)  # Temp sensor DS18B20 as configured in terminal
        sensor.switch_to_input(pull=digitalio.Pull.UP)
        sensor.pull = digitalio.Pull.UP

        parent, = Path('/sys/bus/w1/devices/').glob('28-*')
        self.path = parent / 'w1_slave'

    def read(self) -> float:
        with self.path.open() as f:
            for line in f:
                parts = line.split('t=', 1)
                if len(parts) == 2:
                    return float(parts[1].rstrip()) / 1000
            raise ValueError('Temperature not found')


def polling_loop(interval: float=1, threshold: float=24) -> None:
    sensor = TempSensor()
    led = digitalio.DigitalInOut(D17)  # PIN LED for too hot sensor. 
    led.direction = digitalio.Direction.OUTPUT

    while True:
        temperature = sensor.read()
        print(temperature)
        led.value = temperature > threshold
        time.sleep(interval)


def main() -> None:
    try:
        polling_loop()
    except KeyboardInterrupt:
        pass
    finally:
        digitalio.cleanup() 
        print("Program Exited Cleanly")

if __name__ == '__main__':
    main()
\$\endgroup\$
6
\$\begingroup\$

Introduce hysteresis

In addition to the excellent comments reviewing your code, I want to address something in the algorithm being used. The current algorithm that uses a single threshold will cause frequent starts and stops of the unit. In the very worst case, noise in the temperature sensor could cause the unit to cycle on and off many times per minute or even per second, leading to its rapid destruction. Even if that doesn't happen, starts are a major source of wear on the cooling unit, so it is desirable to reduce the frequency with which it is started. This can be done by introducing hysteresis to the algorithm. So instead of there being a single threshold:

tempLed.value = temperature > 24

there would be one threshold for turning the cooling unit on, and a separate threshold for turning it off:

threshold = 24
hysteresis = 1
# If the unit is running, then require a lower temperature before
# turning it off
if tempLed.value:
    threshold = threshold - hysteresis
tempLed.value = temperature > threshold

To prevent the destruction of the cooling unit, the hysteresis should be at least large enough that it prevents signal noise from starting or stopping the unit. However, if the application is to keep people comfortable, then the hysteresis should not be so large that people feel too cold before the unit shuts off. The larger the hysteresis, the less frequent the unit will start and the longer it will last.

\$\endgroup\$
0
5
\$\begingroup\$

Here are a few comments:

  • To ensure resources are closed, use a with statement instead of a simple open and close:
with open("/sys/bus/w1/devices/28-3cffe076cfcf/w1_slave") as tempStore:
    data = tempStore.read()
  • If the device ID is subject to change, extract it to a variable so its easier to modify. Otherwise, you'd have to go through the code to see where its uses whenever you need to change it:
DEVICE_ID = "28-3cffe076cfcf"
TEMP_SENSOR_PATH = f"/sys/bus/w1/devices/{DEVICE_ID}/w1_slave"
...
open(TEMP_SENSOR_PATH)
  • A simple open statement is fine, but the usual way to go now is using pathlib:
from pathlib import Path

DEVICE_ID = "28-3cffe076cfcf"
TEMP_SENSOR_PATH = Path("/sys/bus/w1/devices/") / DEVICE_ID / "w1_slave"
...
with TEMP_SENSOR_PATH.open() as tempStore:
...
  • Separate concerns: Reading the temperature is unrelated to updating the sensor, so make it clearer by creating two different functions with signatures as such:
...
while True:
    temperature = readTemperature()
    updateLED(temperature, tempLed)
  • I am not familiar with digitalio, but it seems a bit weird to create and configure the variable tempSensor, but then read the temperature from a file instead requesting it to the sensor directly. This could possibly also fix what @AJNeufeld commented with regards of the obscure parsing of the file.

  • I am not sure what you mean by "use Temperature as a global variable so I can use it on other things like a screen/LCD". Are you referring to inter-process communications? Otherwise, if you plan on controlling the LCD display in the same process (which seems like a good option), your main loop would end up like:

while True:
    temperature = readTemperature()
    updateLED(temperature, tempLed)
    updateLCD(temperature, tempLcd)

You could say this already seems like a good fit for the observer pattern, but if you don't plan on adding more subscribers, it might be a bit of an overhead

  • To make the code cleaner, you could use OOP, and encapsulate the sensors in your own classes, so you would first initialise them:
tempSensor = TempSensor(pin=TEMP_SENSOR_PIN)
tempLed = TempLed(pin=TEMP_LED_PIN)
tempLcd = TempLcd(pin=TEMP_LCD_PIN)

And them use them in the main loop:

while True:
    temperature = tempSensor.readTemperature()
    tempLed.update(temperature)
    tempLcd.update(temperature)
  • Try to avoid using from xxx import *, as it makes it harder to know which package is being used when using global statements, as in D19.
\$\endgroup\$
3
\$\begingroup\$

imports

Already mentioned earlier, but it is not recommended to do that:

from board import *

because of namespace pollution and potential function clashes which could be hard to debug. Some background reading: A guide to Python Namespaces

Besides, you import the same module later:

import board

But you could import just the functions you need eg:

from board import somefunc, someotherfunc

However it seems that you not even using the module in this code. datetime also appears to be unused. Ditto for busio. I recommend that you get rid of unused imports or variables because they are unnecessary distractions. The program could be more concise. A good IDE can highlight unused resources in your code.

Comments

The actions you are doing on your pins (?) right after the imports deserve to be commented in plain English. It's not obvious what these lines achieve exactly and why they are critical. Even one line or two would be welcome. Yours comments are a bit cryptic but no doubt you know what they mean now, but if you revisit your code in 6 months you may need a refresher.

The rest of the code is self-explanatory.

Naming conventions

Mixing lowercase and uppercase in variables should be avoided but you can use the underscore _ character to separate keywords. Read up on PEP8.

Parsing data

The way the data is being parsed is a bit clumsy and may be not very robust. You could use a regular expression and capture the desired pattern. Or simplify your split operation like this:

data.split(" ")[-1]

If you know that the temperature value is the rightmost data field, then you can just split the string like you're doing and take the first element starting from the right, hence the negative index. Then you can for example use a further [2:] to extract the temperature from the string after t=.

logging

Instead of using print I recommend that you start the habit of using the logging module. To quote myself, an example. It would be beneficial for the following reasons:

  • in addition to console you can also output text to a file, and the text can be formatted differently than on console
  • the file can also be more verbose than the console - you can decide to send debug messages to file only for example
  • your data samplings will be timestamped automatically if you so wish
  • this is definitely desirable for unattended applications

Exception handling

There is no real exception handling presently, you are only handling the Ctrl-C event but the program could crash for any reason. Just add a catch-all except clause to handle other exceptions that are not explicitly addressed in your code, and log the full error details along with stacktrace. For this you use the logging module.

As mentioned by @Reinderien you can also add a finally clause in your try block and move this code to finally:

digitalio.cleanup()

Thus, it will be systematically executed when leaving the program, even if the program exits as a result of an unhandled exception.

Improvements for the future - data collection

As I pointed earlier you are not logging anything, not keeping any history. I think you may be interested in collecting data to analyze trends or answer questions like:

  • How long has the temperature been above threshold within a certain time period ?
  • How many times did the program turn on the air conditioning ?
  • How long did the air conditioning operate within a certain time period ?
  • What was the maximum temperature recorded ?

For example if you find out that the airco is running 25% of the time it could mean a number of things: possibly the room is not insulated enough, or the airco is not powerful enough. But it's not something you can easily tell without measuring and recording data samples.

If your program turns the airco on and off like it should, it's already good - it's doing its job. But it does not help improve the operating conditions in the long term.

What follows is outside the scope of the review and is merely a suggestion if you want to take your project to the next level, but you could have a machine running Prometheus that collects data from your sensors at regular intervals with the help of a custom exporter and all that data could be visualized on nice dashboards using Grafana. Free and open source of course.

Here is a long tutorial, you can skip a good chunk of it and start with the Prometheus section. FYI the default retention period in Prometheus is 15 days... but you can configure that.

Finally, you could have a graph like this in your dashboard to review observed temperatures with the ability to filter on a time range... (courtesy of the above-mentioned tutorial)

Grafana screenshot

This may look like a big infrastructure project but if you run other machines or even servers it definitely makes sense. You can monitor all sorts of machines and collects all types of metrics to gain insight into the health and activity of your systems.

\$\endgroup\$
1
  • \$\begingroup\$ I'd say the actions being done on the pins are pretty standard when working with electronics. They just define the led pin being output, and the sensor being a pull up input \$\endgroup\$ Aug 11 at 21:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.