NTP Clock on a Raspberry Pi

Question

I've been tinkering around with making a fancy clock on a Raspberry Pi B. This little application will sync the Pi's time with an NTP server, then display it using toilet so it's nice n' big & centered on a monitor. This was made for a GUI-less Pi, and I'm looking to hopefully make it more efficient. Right now the seconds aren't a uniform duration (the time.sleep(0.3) is there to even it out), and I'm sure it's because the Pi is struggling to keep up with my newbie code.

#!/usr/bin/env python
import subprocess
import sys
import os
import time
from datetime import datetime
import ntplib

NTP_MISS = 0

def display_clock():
    #Clock Positioning
    rows, columns = os.popen('stty size', 'r').read().split()
    h_pos = int(int(columns) / 6)
    v_pos = "\033[" + str(int(int(rows) / 6)) + ";0H"

    #Initialization
    check_ntp_time = time.time()
    os.system('setterm --cursor off -powersave off')
    subprocess.call("clear")

    #Meat & Potatoes
    try:
        while check_ntp_time + 86400 >= time.time():
            current_date = subprocess.check_output(["date", "+%r"]).strip().decode("utf-8")
            current_date = " " * h_pos + current_date
            sys.stdout.write(v_pos + subprocess.check_output(["toilet", "-w 200", current_date]).decode("utf-8"))
            sys.stdout.flush()
            time.sleep(0.3)
    except KeyboardInterrupt:
        print("Closing NTP Clock")
        os.system('setterm --cursor on -powersave on')
        subprocess.call("clear")
        print("Closed NTP Clock")
        exit()

    #Update time using NTP pool.
    get_ntp_time()



def get_ntp_time():
    call = ntplib.NTPClient()
    while True:
        try:
            #Try to connect to the NTP pool...
            response = call.request('pool.ntp.org')
            print("Time updated!")
            break
        except:
            NTP_MISS += 1
            if NTP_MISS >= 5:
                print("NTP server unavailable for too long! Terminating...")
                os.system('setterm --cursor on -powersave on')
                subprocess.call("clear")
                print("NTP Clock failed. Server unresponsive for too long.")
                exit()
            #Not a huge deal if we miss it, just continue displaying the clock.
            print("Could not connect to pool.")
            display_clock()
    #If we get a response, update the system time.
    t = datetime.fromtimestamp(response.tx_time)
    t = t.strftime('%Y-%m-%d %I:%M:%S %Z')
    set_string = "--set=" + t
    subprocess.call(["date", set_string])
    display_clock()



if __name__ == "__main__":
    display_clock()

I'd prefer to use the subprocess instead of os.system, but for some reason using subprocess with setterm returned a "command not found" error. So that's why I'm using os.system instead.

What all can I do to optimize & clean up this code?

Answer 1

In general your script looks good when considering the style and structure, but there are a few issues:

• Strange name for check_ntp_time – This holds the time since the last time you did time.time(), and in reality is a temporary variable for when you need to check it the last time. It doesn't convey the real purpose of the variable.

  I would rather do time_to_check_ntp = time.time() + 86400, and let the condition be time.time() < time_to_check_ntp or similar, which reads easier as to whether we've come to that time already, or not.

• Consider replacing toilet with python stuff – One option could be to use pyfiglet, and another could be to use fonts of your own based on script suggestions from "Is there a python library that allows to easily print ascii-art text?" and "ASCII Art With Letters".

• New function for setterm & clear – Introduce a new function where you can set it to on or off, which would remove some repetition of code.
• Replace call to date to get date with datetime module – In general I would like for python script to do most of it using python stuff. Some of it like clear and setterm, or setting the date needs to be done in the shell on the terminal, so some can't be avoided, but stuff like getting the date I think should be done using internal tools, if possible.
• Avoid the recursivity – Both display_clock() and get_ntp_time() calls each other causing a bad recursivity. If you change display_clock to have a while True: loop you could have a if time.time() > time_to_check_ntp: which then calls get_ntp_time() which possibly should be named correct_time_using_ntp(), and sets a new time_to_check_ntp.
• Remove output before clearing the terminal – No point in outputting text straight ahead of a terminal clear. Nobody would be able to catch these message, unless something fails. I would remove it, or add a logger to keep track of the history of the script.

I'm not going to propose new code, as I don't have the ability to test it, but I hope my suggestions are meaningful and you see how to implement it. If not, please do comment.

Answer 2

To even out the updates, you should just sleep for the remainder of the second.

Replace

time.sleep(0.3)

with

time.sleep(1 - time.time() % 1)

Answer 3

You're using a while loop but just using it to count up to 5. Instead, use a for loop where you count up to 5. In Python for loops can have an else block that gets called if the for loop never reaches a break statement. This means you can rearrange your code to look like this:

for _ in range(5):
    try:
        #Try to connect to the NTP pool...
        response = call.request('pool.ntp.org')
        print("Time updated!")
        break
    except:
        #Not a huge deal if we miss it, just continue displaying the clock.
        print("Could not connect to pool.")
        display_clock()
else:
    print("NTP server unavailable for too long! Terminating...")
    os.system('setterm --cursor on -powersave on')
    subprocess.call("clear")
    print("NTP Clock failed. Server unresponsive for too long.")
    exit()

This makes it easier to read, as the code to handle no break is put at the end, after the whole loop. Instead of splitting up the loop with the final case handled in the middle.

---

On further inspection, I see that you're intending to not loop 5 times on each function call, but only run this attempt 5 times in total. Your current code wont actually do that, it would raise an error. Running this code will demonstrate it:

var = 0

def function():
    var += 1

function()

Traceback (most recent call last):
  File "python", line 6, in <module>
  File "python", line 4, in function
UnboundLocalError: local variable 'var' referenced before assignment

var exists inside the global scope. This does mean that it could be read by function, eg. with print(var). But if you try to modify this value, that violates the scope. This is also an issue with your NTP_MISS value.

To get around this, you'd either need to explicitly refer to it in the global scope with global NTP_MISS inside your function, or you'd have to pass it as a parameter to the function. Given that it would have to pass through another function, I would advocate for global in this case.

def get_ntp_time():
    global NTP_MISS

Now you could update your value as you have above without raising errors. If you want to keep a running total of the exceptions caught, then I recommend sticking with while and only using for if you decide to only break when there have been 5 exceptions in a row.