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

NTP Clock on a Raspberry Pi

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

Problem

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

Solution

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

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

Context

StackExchange Code Review Q#113527, answer score: 6

Revisions (0)

No revisions yet.