3
\$\begingroup\$

Python Noob building an automation irrigation system in Python for Raspberry Pi.

Python has two functions in the most basic use case:

  1. on/off the LED (which will in future be a relay->pump) and
  2. Notifies "This is out of water" (which will in future be a HTTP post)
  • I have a GPIO switch that working successfully on: water_level_sensor.value
  • I obviously don't want the While loop to just spam that its looping, it should only print on each change of state

The below code appears to work successfully on both use cases, however, I am unsatisfied/unconfident that this is a very good/clean method to achieve the "print once" per change of state loop.

It works by having:

  • X = "Wet" outside the outer while loop
  • X = "Dry" at the bottom of the nest while
  • X = "NULL" at the bottom of the inner loop

It just seems messy... How should someone solve this kind of problem neatly/efficiently? I'm also concerned that I will struggle to pull out of this solution any state data.

def water_level():
    while True:
        x = "Wet"          
        while water_level_sensor.value == False:
            water_level_led.value = False
            if x == "Wet":
                print("They System has Water")
            x = "Dry"
        
        while water_level_sensor.value == True:
            water_level_led.value = True
            if x == "Dry":
                print("The System is Dry and needs Water")
            x = ""
New contributor
GlennB is a new contributor to this site. Take care in asking for clarification, commenting, and answering. Check out our Code of Conduct.
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Welcome to Code Review! Please edit your question so that the title describes the purpose of the code, rather than its mechanism. We really need to understand the motivational context to give good reviews. Thanks! \$\endgroup\$ – Toby Speight Jul 21 at 14:51
8
\$\begingroup\$

Just curious, but have you checked CPU usage ? Programs that run in a while loop can be terribly inefficient and taxing. Here you are just probing a sensor, this does not look like a computationally-intensive task but it's still overkill.

Seems to me that a timer would be preferable. Sampling values from your sensors at regular intervals, even every few minutes, would be sufficient. You have a simple example of Python timer here: How to repeat a function every N minutes? and links to more advanced discussions.

There is no real-time imperative here. Most of the time your application will be idle and should not consume a lot of resources.

I see one possible improvement: given that the accuracy of the sensors may be approximate, and that threshold values could cause ups and downs in the sampled reading, I would wait for multiple confirmations of low water levels before triggering the "need water" event. This is not ICU, the plant is not going to die if it has to wait a bit for its dose of water :)

Don't use variable names like x, instead use more meaningful names like you did in the rest of your code.

\$\endgroup\$
1
  • \$\begingroup\$ Not checked CPU... I'll need to do that. For your other point, You're right - not super time-sensitive, a minute or so is not going to kill the plants. I'm just unsure how to represent this with an N function - will take your recommended reading - thank you :) \$\endgroup\$ – GlennB 2 days ago
6
\$\begingroup\$

If I understand what you are trying to do, I think you can (1) remember the prior state, (2) print only on changes, and (3) select the message to print based on the new state.

def water_level():
    messages = {
        False: "The system has water",
        True: "The system is dry and needs water",
    }
    previous = None
    while True:
        val = water_level_sensor.value
        if val != previous:
            water_level_led.value = val
            print(messages[val])
            previous = val
\$\endgroup\$
1
  • \$\begingroup\$ Shouldn't there be some sleep call somewhere? \$\endgroup\$ – Eric Duminil Jul 22 at 12:00
4
\$\begingroup\$

I agree with @Anonymous that you should not loop continuously like that, or you'll waste energy and CPU time continuously checking the values. To solve this you can just sleep, wake up every minute, check the state and go back to sleep.

Another more code-style related observation is that you don't need to nest while loops like that, but only keep track of the last value.

The variable naming is also not ideal, use descriptive naming instead of x. Actually in this case, you do not need the variable at all.

For instance, you could do:

import time

def water_level():
    previous_water_low = None
    while True:
        water_low = water_level_sensor.value

        if water_low != previous_water_low:
            on_water_level_change(water_low)

        previous_water_low = water_low
        time.sleep(60)

 def on_water_level_change(water_low: bool):
    water_level_led.value = water_low

    if water_low:
        print("The System is Dry and needs Water")
    else:
        print("They System has Water")
New contributor
Francesco Pasa is a new contributor to this site. Take care in asking for clarification, commenting, and answering. Check out our Code of Conduct.
\$\endgroup\$
2
  • \$\begingroup\$ Thank you for the detail and example code - very helpful. Its slightly confusing to me as my coding is not sufficient to understand what is fully going on here. particularly around "on_water_level_change" and "def on_water_level_change(water_low: bool):" Its very efficient looking solution though. Thank you! \$\endgroup\$ – GlennB 2 days ago
  • \$\begingroup\$ @GlennB This is similar to what @FMc proposed in the other answer. You just keep track of what the last water_level_sensor value and if that changes it triggers a on_water_level_change, where you can take actions based on the change. I also added an extra sleep to reduce energy use. I just like to factor the real actions into an own function, so that you have a loop doing the triggering and another function defining what happens on water level change. You could also change the trigger condition (multiple sensors? wait for multiple dry minutes?), without changing the action function. \$\endgroup\$ – Francesco Pasa 2 days ago

Your Answer

GlennB is a new contributor. Be nice, and check out our Code of Conduct.

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.