HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonModerate

Automatically turn on Computer from Raspberry Pi (based on user presence in home)

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
turnuserautomaticallyraspberrycomputerbasedhomefrompresence

Problem

I know the code is full of useless data and stylistic and analysis errors and I'd really love and appreciate to have a pro's idea about it so I can learn to code (and think) better.

import os, time
from ConfigParser import SafeConfigParser
configfilename = "imhome.ini"
scriptpath = os.path.abspath(os.path.dirname(__file__))
configfile = os.path.join(scriptpath, configfilename)
parser = SafeConfigParser()
parser.read(configfile)
ip_smartphone = parser.get('imhome', 'ip_smartphone')
mac_address_pc = parser.get('imhome', 'mac_address_pc')
hometime_path = parser.get('imhome', 'hometime_path')
in_path = parser.get('imhome', 'in_path')
mp3_path = parser.get('imhome', 'mp3_path')
maximum_time_before_logout = parser.get('imhome', 'maximum_time_before_logout')
if os.path.exists(hometime_path) == False:
        os.system ("touch " + str(hometime_path))
if os.path.exists(in_path) == True:
        os.system ("rm " + str(in_path))
finished = 1
while finished == 1:
    check_presence = os.system ("ping -w 1 " + str (ip_smartphone) + " >/dev/null")
    file_modification_time = (os.path.getmtime(hometime_path))
    file_delta_time = time.time() - file_modification_time
    if check_presence == 0 :
            os.system ("rm " + str(hometime_path))
            change_arrived=("echo " + str(check_presence) + " > hometime")
            os.system (change_arrived)

            if os.path.exists(in_path) == False:
            # You can add any custom commands to execute when home below this row

                os.system ("etherwake " + str(mac_address_pc))            
                os.system ("mpg123 " + str(mp3_path) + " >/dev/null")
                os.system ("touch " + str(in_path))
            # stop adding commands

    elif check_presence == 256 and file_delta_time > int(maximum_time_before_logout):
        if os.path.exists(in_path) == True:
                    os.system ("rm " + str(in_path))

Solution


  1. Comments on your code



-
You make quite a lot of use here of os.system, which runs a command in a subshell. This can be a risky function to use, because the shell can do anything. For example, you write

os.system ("rm " + str(hometime_path))


but what if hometime_path were -rf *?

You're probably safe in this particular program because it's only for your own use and because the strings come from your own configuration file. But it's worth getting into the habit of doing this kind of thing properly, so that in situations which really are risky, you'll know what to do.

In the case of rm you can use os.unlink:

os.unlink(hometime_path)


But in the general case, use subprocess.call and use the -- argument (if necessary) to make sure that the arguments you pass cannot be misinterpreted as options. For example,

subprocess.call(['rm', '--', hometime_path])


In a case where you want to redirect standard output, like this:

os.system ("ping -w 1 " + str (ip_smartphone) + " >/dev/null")


you can use the stdout keyword argument to subprocess.call:

subprocess.call(['ping', '-w', '1', '--', ip_smartphone],
                stdout = open('/dev/null', 'w'))


-
Most of your calls to str are unnecessary: these configuration parameters ought to be strings already. (If they are not, I think it you'd prefer to get an exception so that you could fix the problem, rather than silently coercing the incorrect value to a string.)

-
You don't need to compare booleans explicitly to True and False. So instead of:

if os.path.exists(in_path) == True:


just write:

if os.path.exists(in_path):


and instead of:

if os.path.exists(hometime_path) == False:


just write:

if not os.path.exists(hometime_path):


-
In this code you remove a file only if it exists:

if os.path.exists(in_path) == True:
    os.system ("rm " + str(in_path))


But the rm command already has a option for this, so you could write instead:

subprocess.call(['rm', '-f', '--', in_path])


Or do it in pure Python:

try:
    os.unlink(in_path)
except OSError: # No such file or directory
    pass


(This is an example of how it is often better to ask for forgiveness than permission.)

-
Assuming that " > hometime" is a mistake for " > " + hometime_path (as suggested in another answer), then in this bit of code:

os.system ("rm " + str(hometime_path))
change_arrived=("echo " + str(check_presence) + " > hometime")
os.system (change_arrived)


you remove a file and then overwrite it with the value of check_presence. There's no need to remove the file first: the second command will overwrite it if it exists.

And in any case this is easy to do in pure Python:

with open(hometime_path, 'w') as f:
    f.write('{}\n'.format(check_presence))


-
You use the modification time of the file hometime_path to remember the last time that your ping command succeeded. Why not remember this time in a variable in Python and avoid having to create and delete and examine this file?

-
You use the existence of the file in_path to remember whether your smartphone is connected. Why not remember this fact in a variable in Python and avoid having to create and delete and examine this file?

  1. Rewrite



Making the above improvements results in something like this (but this is untested, so beware):

import os, subprocess, time
from ConfigParser import SafeConfigParser
from datetime import datetime, timedelta

configfilename = "imhome.ini"
scriptpath = os.path.abspath(os.path.dirname(__file__))
configfile = os.path.join(scriptpath, configfilename)
parser = SafeConfigParser()
parser.read(configfile)
ip_smartphone = parser.get('imhome', 'ip_smartphone')
mac_address_pc = parser.get('imhome', 'mac_address_pc')
mp3_path = parser.get('imhome', 'mp3_path')
timeout = timedelta(seconds = int(parser.get('imhome', 'maximum_time_before_logout')))

devnull = open('/dev/null', 'wb')
connected = False
disconnect_time = None

while True:
    ping = subprocess.call(['ping', '-w', '1', '--', ip_smartphone],
                           stdout = devnull)
    if ping == 0:
        disconnect_time = datetime.now() + timeout
        if not connected:
            connected = True
            subprocess.call(['etherwake', mac_address_pc])
            subprocess.call(['mpg123', mp3_path], stdout = devnull)
    elif ping == 256 and connected and datetime.now() > disconnect_time:
        connected = False
    time.sleep(1)

Code Snippets

os.system ("rm " + str(hometime_path))
os.unlink(hometime_path)
subprocess.call(['rm', '--', hometime_path])
os.system ("ping -w 1 " + str (ip_smartphone) + " >/dev/null")
subprocess.call(['ping', '-w', '1', '--', ip_smartphone],
                stdout = open('/dev/null', 'w'))

Context

StackExchange Code Review Q#20646, answer score: 14

Revisions (0)

No revisions yet.