TkInter GUI interface for STM32 MCU

Question

I'm very new to Python and this is my first ever Python GUI app. I am writing a little app to interact with my STM32 Black Pill board. This is very minimal as I intend to use this as the basis for future project with specific intentions. How did I do? What can I do better?

import tkinter as tk
import serial.tools.list_ports
from threading import *
class Main(tk.Tk):
 def __init__(self, *args, **kwargs):
 tk.Tk.__init__(self, *args, **kwargs)
 self.title("Breadboard MCU Interface")
 self.resizable(False, False)
 ### Menu Configuration ###############################################################################
 self.menubar = tk.Menu(self)
 self.configmenu = tk.Menu(self.menubar, tearoff=0)
 self.configmenu.add_command(label="Communications Port", command=lambda: PortConfig())
 self.menubar.add_cascade(label="Configure", menu=self.configmenu)
 self.menubar.add_command(label="Connect", command=connect)
 self.config(menu=self.menubar)
 ### End Menu Configuration ###########################################################################
 ### GPIO Canvas ######################################################################################
 self.gpio_canvas = tk.Canvas(self, width=325, height=100)
 self.gpio_canvas.create_text(170, 30, text="GPIO", font=('Helvetica 15 bold'))
 self.gpio_canvas.create_text(25, 55, text="Port A")
 self.gpio_canvas.create_text(25, 70, text="Port B")
 self.ports = { "A": [], "B": [] }
 for pin in range(16, 0, -1):
 self.ports['A'].append(self.gpio_canvas.create_oval(60 + (pin * 15), 50, 70 + (pin * 15), 60, fill = "green"))
 self.ports['B'].append(self.gpio_canvas.create_oval(60 + (pin * 15), 65, 70 + (pin * 15), 75, fill = "green"))
 self.gpio_canvas.pack(fill="both")
 ### End GPIO Canvas ###################################################################################
 def updatePinDisplay(self, portid, pinvalues):
 for i in range(0,16):
 pinvalue = ((pinvalues >> i) & 1)
 if pinvalue:
 self.gpio_canvas.itemconfig(self.ports[portid][i], fill="green2")
 else:
 self.gpio_canvas.itemconfig(self.ports[portid][i], fill="green")
 #self.update()
class PortConfig(tk.Toplevel):
 def __init__(self, *args, **kwargs):
 def getSelection(choice):
 self.selected = str(choice).split(' ')[0]
 def setComPort():
 global comport
 comport = self.selected
 connect()
 self.destroy()
 tk.Toplevel.__init__(self, *args, **kwargs)
 self.title("Connection Configuration")
 self.resizable(False, False)
 self.config(padx=5, pady=5)
 ports = serial.tools.list_ports.comports()
 default = tk.StringVar(self, "Please select a communications port")
 self.selected = None
 self.portslist = tk.OptionMenu(self, default, *ports, command=getSelection).pack(side="left")
 self.okbutton = tk.Button(self, text="OK", command=setComPort).pack(side="left", padx=10)
 self.focus()
 self.grab_set()
class WorkThread(Thread):
 global connection
 def run(self):
 global porta_pins
 print("Worker thread running...")
 try:
 while connection.is_open:
 while connection.in_waiting:
 indata = connection.readline().decode('Ascii')
 portid, pinvalues = indata.split('#')
 app.updatePinDisplay(portid, int(pinvalues))
 except Exception as e:
 print(str(e))
 pass
 
 print("Worker thread closing down.")
def connect():
 global connection, connected, workthread
 try:
 connection = serial.Serial(comport, baudrate=1152000, bytesize=serial.EIGHTBITS, parity=serial.PARITY_NONE, stopbits=serial.STOPBITS_ONE, timeout=2)
 except:
 pass
 if connection is not None:
 connected = True
 connection.write("Connection established.\r\n".encode('Ascii'))
 app.menubar.entryconfigure(2, label="Disconnect", command=disconnect)
 workthread = WorkThread()
 workthread.start()
 else:
 connected = False
 print("Connection failed.")
def disconnect():
 global connection, connected, workthread
 connection.close()
 connected = False
 workthread.join()
 workthread = None
 app.menubar.entryconfigure(2, label="Connect", command=connect)
if __name__ == '__main__':
 comport = None
 connection = None
 connected = False
 workthread = None
 app = Main()
 app.mainloop()

• Heslacher: Welcome to Code Review. Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers . I have rolled back your edits.

Answer 1

This is some pretty nice code; it's clear what is happening here.

---

use informative names

from threading import *
class Main(tk.Tk):

That's a bit generic. Consider using something more descriptive, perhaps:

class BlackPillGui(tk.Tk):

And that * star is just annoying. Please prefer from threading import Thread.

---

globals?!?

 global connection, connected, workthread
 connection.close()

Two concerns here.

1. Prefer to put these in an object or datastructure, instead of using the global namespace.
2. No need to declare global connection since you don't assign to it. The subsequent undeclared app reference offers an illustration of such use.

---

don't represent the same concept two ways

I am sad to see the connected boolean being introduced.

It appears it is always identical to if connection is not None:, or just if connection:. Recommend you elide it.

Kudos on using an if __name__ == '__main__': guard. Though frankly, since everything relies on Tk, it's not like a unit test could import this module and usefully test it.

---

restraint in comments

 ### Menu Configuration ###############################################################################

Uggh, ASCII art!

If there's some coding standard which requires this, that you're trying to adhere to, then cite the reference.

Otherwise, pep-8 asks that you show some restraint. In particular, when you lint your code you are apparently inhibiting E266 messages in your .flake8 config file: E266 too many leading '#' for block comment

---

layout, magic numbers

The GPIO canvas has a bunch of magic numbers for layout, with relationships among some of them.

 self.gpio_canvas.create_text(25, 55, text="Port A")
 self.gpio_canvas.create_text(25, 70, text="Port B")

IDK, maybe that's OK? Consider defining things like LEFT_MARGIN = 25.

---

comments suggest helpers

The fact that you felt it necessary to point out the begin / end of Menu Configuration, and of GPIO Canvas, suggests that the __init__ ctor should have a pair of helpers:

 def menu_config(self):
 ...
 def gpio_canvas(self):
 ...

---

use conventional names

 def updatePinDisplay(self, portid, pinvalues):

Pep8 asks that you spell it update_pin_display.

Pleaseconsiderclarifyingcertainnames such as com_port, ok_button, port_id, and pin_values.

---

optional type hints to help the reader

The plural pin values name was very helpful, thank you. And the "right shift i places" soon clarifies things. Still, when I read the signature I had in mind there was a container of values, rather than a bit vector encoded as an int.

Declaring

 ...(self, port_id: str, pin_values: int):

would be a kindness to the Gentle Reader. (Plus, mypy could help with linting it if you like.)

---

mapping for business logic

 if pinvalue:
 self.gpio_canvas.itemconfig(self.ports[portid][i], fill="green2")
 else:
 self.gpio_canvas.itemconfig(self.ports[portid][i], fill="green")

You can DRY up the pin coloring in this way:

 pin_colors = ["green", "green2"]
 self.gpio_canvas.itemconfig(self.ports[port_id][i], fill=pin_colors[pin_value])

This avoids pasting a pair of similar calls to itemconfig.

---

simplify nested loops

 while connection.is_open:
 while connection.in_waiting:

Recommend a simple conjunct:

 while (connection.is_open:
 and connection.in_waiting):

I worry a little about failure modes, and the UX. I imagine there may be off-nominal situations where we loop forever, not obtaining any text lines. Maybe it would be useful to print() or to update a Tk text box, letting the user know we're awaiting input? And then immediately blank it out, once the input is received.

---

no bare excepts

def connect():
 ...
 except:
 pass

Disabling CTRL/C KeyboardInterrupt is not what you want. Prefer except Exception:

Also, you could simplify print(str(e)) to the identical print(e).

---

This code is clear, and achieves its design objectives.

I would be willing to delegate or accept maintenance tasks on this codebase.

• RadioMercy: Thank you very much for the input, I appreciate your response and will work on following these suggestions moving forward.