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

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.