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

Question

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))

Answer 1

1. Comments on your code

1. 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'))
   

2. 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.)

3. 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):
   

4. 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.)

5. 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))
   

6. 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?

7. 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?

2. 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)

• Pitto: This is seriously great fuel for my brain! I am VERY thankful also for the time you took to explain and incredibly rewrite the code! I'll hide it in my drawer and try to write it on my own to check how I do compared to you. If you happen to be in central Italy you have a dinner paid! :D
• Pitto: I'm finally back from my holidays and I am trying to understand better your work... I can't get how you handle time... If I print the variables to understand what is going on I still can't get it. The main problem is timeout. in timeout you get the number of seconds defined in the config file, right? And then you use this to calculate disconnect_time adding this very moment to maximum time before logout? Why? Maybe it's just a mistake (you told me it wasn't checked) but I am a newbie so... :)
• Gareth Rees: The idea is that disconnect_time is the clock time at which the program will consider the smartphone to be disconnected if it hasn't received any more ping responses by then. Each time a ping response is received, we update diconnect_time to a be a bit further in the future. But if we don't receive any ping responses, we leave it unchanged, until eventually datetime.now() > disconnect_time. (This is a standard idiom for timeout processing.)
• Pitto: Mmmh ok... But why to you sum it with timeout (which is the value included in my config file)?
• Gareth Rees: Suppose that your config file has timeout = 15 seconds. Then each time we receive a ping response, we set disconnect_time = datetime.now() + timeout. That is, disconnect_time now represents a time 15 seconds in the future. If we haven't received any more ping responses by then, we'll consider the smartphone to be disconnected.

Answer 2

Neat idea with that script. It looks good; not bad for the first program. Just a few comments:

1. I would improve some variable names: file_modification_time -> phone_last_seen_time, file_delta_time -> phone_unseen_duration. When I was reading the program it took me some time to figure out their purpose.
2. Use while True: instead of while finished == 1: so it is immediately clear that it is an infinite loop.
3. In the line change_arrived=("echo " + str(check_presence) + " > hometime") you probably want str(hometime_path) instead of "hometime".
4. It would be also good to describe purpose of some variables in comments. Mainly hometime_path and in_path.

Answer 3

I would add a time.sleep(1) call at the end of the loop. Without that I had to kill the program from the shell to stop it, as it was not responding to Ctrl-C.