Car computer in python / GPS tracking

Question

I have an old car that I use for long distance driving and have coded an onboard computer based on a Raspberry Pi 3 and a few other modules. It's my first project in Python and while I've already gotten some help, I now have a version of my project which works quite well.

I use a FONA 808 from Adafruit as the cellular modem (via serial), the Sparkfun NEO-M9N as a GPS sensor (i2c), an OLED display (i2c) and a small temperature sensor via 1-wire. Here is a link to a picture of the computer in action, just so you have a better picture of it: https://www.instagram.com/p/CJO9HnNneg2/

I'd appreciate getting some optimization tips to make it run smoothly. I'm especially unsure on how I've used threading and if I'm being really efficient with the GPS and the logging etc... Thanks!

(also, if the question is missing some information, don't downvote, comment and I'll correct it)

import os
from threading import Thread
import glob
import serial
import subprocess
import urllib
import urllib.request
import urllib.parse
import array
import requests
from time import sleep
from luma.core.interface.serial import i2c
from luma.core.render import canvas
from luma.oled.device import sh1106
from PIL import ImageFont, Image, ImageDraw
import time
import board
import busio
import subprocess
import pymysql
import RPi.GPIO as GPIO
from haversine import haversine, Unit
import csv
import pandas
import datetime
import pathlib
import json
import smbus
import logging
import pynmea2
################## config display ##################
device = sh1106(i2c(port=1, address=0x3c), rotate=0)
device.clear()
global pending_redraw
pending_redraw = False
### setup different fonts
FA_solid = ImageFont.truetype('/home/pi/Desktop/fonts/fa-solid-900.ttf', 12)
FA_solid_largest = ImageFont.truetype('/home/pi/Desktop/fonts/fa-solid-900.ttf', 40)
text_largest = ImageFont.truetype('/home/pi/Desktop/fonts/digital-7.ttf', 58)
text_medium = ImageFont.truetype('/home/pi/Desktop/fonts/digital-7.ttf', 24)
text_small = ImageFont.truetype('/home/pi/Desktop/fonts/digital-7.ttf', 18)
 
### Initialize drawing zone (aka entire screen)
output = Image.new("1", (128,64))
add_to_image = ImageDraw.Draw(output)
### coordinates always: padding-left, padding-top. the first pair of zone is always = start
# temp_ext
temp_zone = [(14,44), (36,64)]
temp_start = (14,44)
temp_icon_zone = [(0,48), (15,64)]
temp_icon_start = (3,48)
# alti
alti_zone = [(14,22), (69,40)]
alti_start = (14,22)
alti_icon_zone = [(0,24), (15,40)]
alti_icon_start = (0,26)
# distance
dist_zone = [(14,0), (69,21)]
dist_start = (14,0)
dist_icon_zone = [(0,4), (15,21)]
dist_icon_start = (0,4)
# speed
speed_zone = [(66,0), (128,45)]
speed_start = (66,0)
# GPRS status
gprs_zone = [(114,46), (128,64)]
gprs_start = (114,50)
# GPS status, incl. GPS startup icon
status_icon_zone = [(70,50), (88,64)]
status_icon_start = (70,50)
status_zone = [(86,46), (113,64)]
status_start_text = (86,46)
status_start = (86,50)
# usage
#add_to_image.rectangle(speed_zone, fill="black", outline = "black")
#add_to_image.text(speed_start, "\uf00c", font=FA_solid, fill="white")
#device.display(output)
################## upload data from GPS folder via FONA to MySQL ##################
def fix_nulls(s):
 for line in s:
 yield line.replace('0円','')
def upload_data():
 while True:
 sleep(20)
 current_dir = "/home/pi/Desktop/data/gps"
 archive_dir = "/home/pi/Desktop/data/gps/archive"
 path, dirs, files = next(os.walk(current_dir))
 file_count = len(files)
 if file_count < 2:
 print("Not enough GPS.csv files found so it's probably in use now or doesn't exist")
 return
 list_of_files = glob.glob(current_dir+"/*.csv")
 oldest_file = min(list_of_files, key=os.path.getctime)
 oldest_file_name = os.path.basename(oldest_file)
 
 try:
 add_to_image.rectangle(gprs_zone, fill="black", outline = "black")
 add_to_image.text(gprs_start, "\uf0c2", font=FA_solid, fill="white")
 global pending_redraw
 pending_redraw = True
 print("Opening remote db")
 
 openPPPD()
 print("Opening remote db: done")
 
 db = pymysql.connect("XXX","XXX","XXX","XXX" )
 cursor = db.cursor()
 csv_data = csv.reader(fix_nulls(open(oldest_file)))
 next(csv_data)
 for row in csv_data:
 if row:
 cursor.execute('INSERT INTO gps_data_2 (gps_time, gps_lat, gps_long, gps_speed) VALUES (%s, %s, %s, %s)',row)
 print("Commiting to db")
 db.commit()
 cursor.close()
 
 closePPPD()
 
 print("Successfully commited to db")
 add_to_image.rectangle(gprs_zone, fill="black", outline = "black")
 add_to_image.text(gprs_start, "\uf058", font=FA_solid, fill="white")
 pending_redraw = True
 
 os.rename(current_dir+"/"+oldest_file_name, archive_dir+"/archive_"+oldest_file_name) 
 sleep(60)
 add_to_image.rectangle(gprs_zone, fill="black", outline = "black")
 
 except Exception as e:
 print("Database error:", e) 
 sleep(60)
 add_to_image.rectangle(gprs_zone, fill="black", outline = "black")
 return
 
 sleep(300)
################## config and start GPS ##################
BUS = None
address = 0x42
gpsReadInterval = 1
reading_nr = 1
reading_nr_upload = 1
reading_nr_upload_nbrowsinlog = 0
total_km = 0
prev_lat = 0
prev_long = 0
def connectBus():
 global BUS
 BUS = smbus.SMBus(1)
def parseResponse(gpsLine):
 gpsChars = ''.join(chr(c) for c in gpsLine)
 local_pending_redraw = False
 
 if "$GNGGA" in gpsChars:
 if ",1," not in gpsChars:
 print("Looking for fix... (GGA)")
 add_to_image.rectangle(status_icon_zone, fill="black", outline = "black")
 add_to_image.rectangle(status_zone, fill="black", outline = "black")
 add_to_image.text(status_icon_start, "\uf124", font=FA_solid, fill="white")
 add_to_image.text(status_start, "\uf128", font=FA_solid, fill="white")
 local_pending_redraw = True
 return False
 try:
 nmea = pynmea2.parse(gpsChars, check=True)
 #print("GGA:", '%.6f'%(nmea.latitude), ",",'%.6f'%(nmea.longitude), ", sats:", nmea.num_sats, ", alt:", nmea.altitude) # GGA
 if "0.0" in str(nmea.latitude):
 return False
 if "0.0" in str(nmea.longitude):
 return False
 
 ## show fix + nb satellites
 #num_sats = str(nmea.num_sats)
 #num_sats = num_sats.lstrip("0")
 #add_to_image.rectangle(status_icon_zone, fill="black", outline = "black")
 #add_to_image.rectangle(status_zone, fill="black", outline = "black")
 #add_to_image.text(status_icon_start, "\uf124", font=FA_solid, fill="white")
 #add_to_image.text(status_start_text, num_sats, font=text_medium, fill="white")
 
 ## update altitude
 add_to_image.text(alti_icon_start, "\uf077", font=FA_solid, fill="white")
 add_to_image.rectangle(alti_zone, fill="black", outline = "black")
 add_to_image.text(alti_start, str('%.0f'%(nmea.altitude)), font=text_medium, fill="white")
 
 ## update total distance
 global reading_nr
 global total_km
 global prev_lat
 global prev_long
 dist = 0
 if reading_nr != 1:
 dist = haversine(((float(prev_lat)), (float(prev_long))), ((float(nmea.latitude)), (float(nmea.longitude))))
 total_km = total_km+dist
 add_to_image.text(dist_icon_start, "\uf1b9", font=FA_solid, fill="white")
 add_to_image.rectangle(dist_zone, fill="black", outline = "black")
 add_to_image.text(dist_start, "%0.1f" % total_km, font=text_medium, fill="white")
 prev_lat = nmea.latitude
 prev_long = nmea.longitude
 
 local_pending_redraw = True
 reading_nr +=1
 
 except Exception as e:
 print("GGA parse error:", e)
 add_to_image.rectangle(status_zone, fill="black", outline = "black")
 local_pending_redraw = True
 pass
 
 if "$GNRMC" in gpsChars:
 if ",A," not in gpsChars: # 1 for GGA, A for RMC
 print("Looking for fix... (RMC)")
 add_to_image.rectangle(status_icon_zone, fill="black", outline = "black")
 add_to_image.rectangle(status_zone, fill="black", outline = "black")
 add_to_image.text(status_icon_start, "\uf124", font=FA_solid, fill="white")
 add_to_image.text(status_start, "\uf128", font=FA_solid, fill="white")
 local_pending_redraw = True
 return False
 try:
 nmea = pynmea2.parse(gpsChars, check=True)
 if "0.0" in str(nmea.latitude):
 return False
 if "0.0" in str(nmea.longitude):
 return False
 add_to_image.rectangle(status_zone, fill="black", outline = "black")
 
 ## update speed
 add_to_image.rectangle(speed_zone, fill="black", outline = "black")
 add_to_image.text(speed_start, str('%.0f'%(nmea.spd_over_grnd*1.852)), font=text_largest, fill="white")
 local_pending_redraw = True
 
 ## log every 5th GPS coordinate in CSV file
 global reading_nr_upload
 global reading_nr_upload_nbrowsinlog
 #print(reading_nr_upload)
 if reading_nr_upload % 5 == 0:
 t = datetime.datetime.combine(nmea.datestamp, nmea.timestamp).strftime("%s")
 d = datetime.datetime.combine(nmea.datestamp, nmea.timestamp).strftime("%Y%m%d%H")
 filename = '/home/pi/Desktop/data/gps/gps_' + d + '.csv'
 with open(filename, 'a', newline='') as csvfile:
 gps_writer = csv.writer(csvfile, delimiter=',', quotechar='|', quoting=csv.QUOTE_MINIMAL)
 gps_writer.writerow([t, nmea.latitude, nmea.longitude, nmea.spd_over_grnd*1.852])
 reading_nr_upload_nbrowsinlog +=1
 print("Added to log. Total in Log from this session is", reading_nr_upload_nbrowsinlog)
 add_to_image.rectangle(status_icon_zone, fill="black", outline = "black")
 add_to_image.rectangle(status_zone, fill="black", outline = "black")
 add_to_image.text(status_icon_start, "\uf124", font=FA_solid, fill="white")
 add_to_image.text(status_start, "\uf56f", font=FA_solid, fill="white")
 reading_nr_upload +=1
 
 #print("RMC: speed is", nmea.spd_over_grnd*1.852) # RMC
 #print("RMC nmea.longitude:", nmea.longitude)
 
 except Exception as e:
 print("RMC parse error:", e)
 add_to_image.rectangle(status_zone, fill="black", outline = "black")
 local_pending_redraw = True
 pass
 
 if local_pending_redraw == True:
 global pending_redraw
 pending_redraw = True
def readGPS(gpsReadInterval=1):
 c = None
 response = []
 try:
 while True: # Newline, or bad char.
 global BUS
 c = BUS.read_byte(address)
 if c == 255:
 return False
 elif c == 10:
 break
 else:
 response.append(c)
 parseResponse(response)
 except IOError:
 time.sleep(0.5)
 connectBus()
connectBus()
def updateGPS(gpsReadInterval=1):
 while True:
 readGPS()
 #sleep(gpsReadInterval)
################## config external thermometer ##################
def update_temp_ext(temp_signature='t=', update_interval=60):
 sleep(10)
 add_to_image.text(temp_icon_start, "\uf2c9", font=FA_solid, fill="white")
 while True:
 f = open('/sys/bus/w1/devices/28-012032ffbd96/w1_slave', 'r')
 lines = f.readlines()
 f.close()
 equals_pos = lines[1].find(temp_signature)
 if equals_pos != -1:
 temp_string = lines[1][equals_pos+2:]
 temp_c = round(float(temp_string) / 1000.0)
 add_to_image.rectangle(temp_zone, fill="black", outline = "black")
 add_to_image.text(temp_start, str(temp_c), font=text_medium, fill="white")
 global pending_redraw
 pending_redraw = True
 
 #filename = 'data/temp_ext/tempext_' + datetime.datetime.now().strftime("%Y%m%d") + '.csv'
 #with open(filename, 'a', newline='') as csvfile:
 # temp_ext_writer = csv.writer(csvfile, delimiter=' ', quotechar='|', quoting=csv.QUOTE_MINIMAL)
 # temp_ext_writer.writerow([str(temp_c)])
 
 time.sleep(update_interval)
 
 
 
 
################## update display ##################
def update_display():
 while True:
 # there is a potential race condition here, not critical
 global pending_redraw
 if pending_redraw:
 pending_redraw = False
 device.display(output)
 time.sleep(0.1)
 
 
################## start cellular connection ################## 
def openPPPD():
 print("Opening PPPD")
 
 subprocess.call("sudo pon fona", shell=True)
 print("FONA on")
 
 add_to_image.rectangle(gprs_zone, fill="black", outline = "black")
 add_to_image.text(gprs_start, "\uf0c2", font=FA_solid, fill="white")
 global pending_redraw
 pending_redraw = True
 
 sleep(20)
 try:
 add_to_image.rectangle(gprs_zone, fill="black", outline = "black")
 add_to_image.text(gprs_start, "\uf0c2", font=FA_solid, fill="white")
 pending_redraw = True
 urllib.request.urlopen('http://google.com')
 print("Connection is on")
 add_to_image.rectangle(gprs_zone, fill="black", outline = "black")
 add_to_image.text(gprs_start, "\uf382", font=FA_solid, fill="white")
 pending_redraw = True
 return True
 except:
 print("No connection")
 add_to_image.rectangle(gprs_zone, fill="black", outline = "black")
 add_to_image.text(gprs_start, "\uf127", font=FA_solid, fill="white")
 pending_redraw = True
 sleep(5)
 return False
# Stop PPPD
def closePPPD():
 print("turning off PPPD")
 subprocess.call("sudo poff fona", shell=True)
 print("turned off")
 return True
 
 
 
 
 
 
 
 
################## threading and program execution ##################
if __name__ == '__main__':
 temp_ext_thread = Thread(target = update_temp_ext)
 display_thread = Thread(target=update_display)
 gps_thread = Thread(target = updateGPS)
 data_thread = Thread(target = upload_data)
 
 display_thread.start() 
 gps_thread.start()
 data_thread.start()
 temp_ext_thread.start()
 
 display_thread.join()

• pacmaninbw: Welcome to the Code Review Community where we review working code and provide suggestions for how to improve that code. The question is fairly good, but it is border line on working code. What makes it questionable is but is not super super reliable.. This is why I haven't voted it up.
• Damien Bourdonneau: @pacmaninbw thx for your feedback! I meant to say something like "it works, but I wouldn't trust it with my life". It's working at the moment, I believe the only hiccups are when there's a loose contact on my breadboard - I have ordered a PCB to remedy that
• pacmaninbw: According to some people on this site, Duct tape or Duck tape fixes everything including breadboards.
• user9123: While this isn't really about the code, I'm a little concerned about you directly connecting to a remote MySQL server, since that tends to be pretty insecure (your credentials are probably broadcasted to the nearest cell tower in plaintext unless you've set up TLS). I would definitely recommend using a HTTPS POST request instead, along with a PHP script on the server to commit the information to the DB upon receiving said request, possibly also with some authentication, even if HTTP basic auth.
• user9123: @DamienBourdonneau I see. With the prepare/commit issue, it should be fine since if it's formatted as JSON, the server needs the whole message to start working on it, so there won't be any malformed data, but you could also include a checksum to be sure. As for POST body size, it should be configurable on the server, assuming your host lets you do that. You may want to contact their support and ask, but it should be pretty big by default anyway. Good luck though if you do decide to try it! :)

Answer 1

There's A LOT going on in your code but I'll try to give you some hints and some suggestions regarding the overall structure/workflow of the code.

• you have really abused the use of globals in your code. Some of them don't even make sense. I'd suggest you try and understand what the use of it is and stop using it unless really needed. For example, the use of global keyword outside a function has no effect (e.g. pending_redraw).
• for setting your fonts, you could easily create a function and a dataclass to stick to the DRY principle:

FONTS_BASEDIR = '/home/pi/Desktop/fonts/'
def setup_font(font_filename, value):
 return ImageFont.truetype(
 os.path.join(FONTS_BASEDIR, font_filename),
 value
 )
@dataclass
class Fonts:
 fa_solid = setup_font('fa-solid-900.ttf', 12)
 fa_solid_largest = setup_font('fa-solid-900.ttf', 40)
 text_largest = setup_font('digital-7.ttf', 58)
 text_medium = setup_font('fa-solid-900.ttf', 24)
 text_small = setup_font('fa-solid-900.ttf', 18)

• remove the unused imports and try not to duplicate them.
• try to be as clear/accurate as possible in your comments. If I were to believe this: coordinates always: padding-left, padding-top. the first pair of zone is always = start which doesn't seem to be the case for temp_icon_zone and temp_icon_start ((0, 48) != (3, 48)).
• you have a lot of constants which look more like configuration variables. I'd suggest you create a config file to have easier access to all of these. Config variables should also be named using the UPPER_CASE notation. (e.g. temp_zone would be TEMP_ZONE = [(14, 44), (36, 64)] and temp_start would become: TEMP_START = TEMP_ZONE[0])
• try using os.path.join() instead of current_dir + "/" + oldest_file_name, archive_dir + "/archive_" + oldest_file_name when joining together pieces of a path. (see my example from setup_font function)
• you misspelled committed and committing
• here: db = pymysql.connect("XXX", "XXX", "XXX", "XXX") it looks like you're using clear-text credentials. That's bad practice and I'd suggest you stop doing that! There are multiple other ways of importing your credentials, one of which would be setting the password as an ENV Variable.
• this:

if "0.0" in str(nmea.latitude):
 return False
if "0.0" in str(nmea.longitude):
 return False

can be rewritten as this:

if "0.0" in str(nmea.latitude) or "0.0" in str(nmea.longitude):
 return False

or even better:

return "0.0" not in str(nmea.latitude) or "0.0" not in str(nmea.longitude)

• here:

f = open('/sys/bus/w1/devices/28-012032ffbd96/w1_slave', 'r')
lines = f.readlines()
f.close()
equals_pos = lines[1].find(temp_signature)

you only seem to use whatever it's on the second line of that file, so why keep the whole file into memory? Try to break after you reached that very line:

with open('/sys/bus/w1/devices/28-012032ffbd96/w1_slave') as f:
 for i, line in enumerate(f):
 if i == 1:
 equals_pos = line.find(...)
 break

• Damien Bourdonneau: that's great, thanks for the feedback! I'll look into your comments (which all make sense from a first read), implement them and edit/comment when confused or successful! Also, I'm not not sure about threading at the end and on which thread I join - does that look right to you?
• Grajdeanu Alex: Please don’t edit your code but rather ask a follow-up question instead with the new code to be reviewed. Also, it’s worth allowing 1-2 days until accepting an answer since there might be valuable input still coming from other reviewers
• Setris: return "0.0" not in str(nmea.latitude) or "0.0" not in str(nmea.longitude) does not have the same behavior as the first rewrite because it changes a conditional return to an unconditional return
• Damien Bourdonneau: @GrajdeanuAlex I'm implementing everything atm but I have a few questions: following your advice, I now have an .ini file with different config variables etc. Is that a safe(r) place to store the mysql info? Also, regarding TEMP_ZONE = [(14, 44), (36, 64)]: I have added that to my ini file too but it seems that when I try to use it, it is not seen as a tuple but just some random string and thus the coordinates are not read correctly. Got any tips on that? Have been unsuccessful so far. The rest I've implemented as far as possible!
• Grajdeanu Alex: "Is that a safe(r) place to store the mysql info?" -> not quite / it depends. It's usually a common practice to have a .env file where you can store these kind of things. You also have to make sure you don't add this .env file to git. The parsed configuration (from this env file) is used as a basis for adding environment variables. If you want to keep the data structures and use them in your code, you can use, at any point, any other data type format like a yaml file / json file or even a config.py.

Answer 2

Other minor points:

Generators

Your generator -

for line in s:
 yield line.replace('0円','')

can be simplified to

return (line.replace('0円', '') for line in s)

To see what the difference is, we borrow from some techniques in a different answer:

>>> def f():
... for c in 'a b':
... yield c.replace(' ', '')
>>> def g():
... return (c.replace(' ', '') for c in 'a b')
>>> from inspect import isgeneratorfunction
>>> from dis import dis
>>> isgeneratorfunction(f)
True
>>> isgeneratorfunction(g)
False
>>> dis(f)
 2 0 LOAD_CONST 1 ('a b')
 2 GET_ITER
 >> 4 FOR_ITER 18 (to 24)
 6 STORE_FAST 0 (c)
 3 8 LOAD_FAST 0 (c)
 10 LOAD_METHOD 0 (replace)
 12 LOAD_CONST 2 (' ')
 14 LOAD_CONST 3 ('')
 16 CALL_METHOD 2
 18 YIELD_VALUE
 20 POP_TOP
 22 JUMP_ABSOLUTE 4
 >> 24 LOAD_CONST 0 (None)
 26 RETURN_VALUE
>>> dis(g)
 2 0 LOAD_CONST 1 (<code object <genexpr> at 0x00000236289E7F50, file "<stdin>", line 2>)
 2 LOAD_CONST 2 ('g.<locals>.<genexpr>')
 4 MAKE_FUNCTION 0
 6 LOAD_CONST 3 ('a b')
 8 GET_ITER
 10 CALL_FUNCTION 1
 12 RETURN_VALUE
Disassembly of <code object <genexpr> at 0x00000236289E7F50, file "<stdin>", line 2>:
 2 0 LOAD_FAST 0 (.0)
 >> 2 FOR_ITER 18 (to 22)
 4 STORE_FAST 1 (c)
 6 LOAD_FAST 1 (c)
 8 LOAD_METHOD 0 (replace)
 10 LOAD_CONST 0 (' ')
 12 LOAD_CONST 1 ('')
 14 CALL_METHOD 2
 16 YIELD_VALUE
 18 POP_TOP
 20 JUMP_ABSOLUTE 2
 >> 22 LOAD_CONST 2 (None)
 24 RETURN_VALUE

This demonstrates that writing a generator function does not produce equivalent bytecode to writing a function that returns a generator. The generator function produces bytecode that is shorter, but the generator-returning function has code that's more terse; so in the end it's somewhat of a wash.

Paths

It's not a good idea to bake /home/pi into your data directory paths. This should be configurable, and/or auto-discovered based on the location of the source. Beyond that, storing your application in a subdirectory of a login user's desktop directory is not a good idea. Do some research on the standard Unix directory layout, which would see your fonts in /usr/share/my_app, your application in /usr/bin, etc.

• Peilonrayz: How is converting from a generator to a generator expression any simpler? I don't see how moving the for loop from the line above to behind the expression could be called "simplified" when all you've done is rearrange the code.
• Reinderien: @Peilonrayz I don't understand your comment. Knowing how the interpreter differentiates between these two styles is educational.
• Reinderien: My original comment was actually just wrong, due to a typo in the example function. The yield style produces simpler bytecode, but the return style requires fewer lines of code. In the end it's a judgement call based on context.
• Gavin S. Yancey: @Reinderien is yield from best of both worlds?
• Reinderien: Autodiscovery isn't strictly necessary if you choose sane paths for your data. You can omit getcwd and hard-code a base path for your fonts like /usr/share/carcomp/fonts.