Hey,
ich hab mir hier ein kleines Script zusammen geschrieben (teilweise auch kopiert).
Es ist mein erstes Script und ich wollte wissen ob man noch Verbesserungen einbauen kann.
Das Script soll einfach nur CPU, RAM und IP Infos auf dem Display ausgeben und den Pi herunterfahren und neu starten lassen können.
Einziges Problem welches ich habe:
Ich kann im Script die Hintergrundbeleuchtung vom LCD nicht ausschalten (Ziele 199). Nur wenn ich das Script beende funktioniert der Befehl (Zeile 210).
Hättet Ihr eventuell eine Lösung dafür?
Link zum Skript:
https://pastebin.com/VXnxc2vB
16x2 LCD / Menü
Moin,
ein paar allgemeine Anmerkungen und Verbesserungsvorschläge zu deinem Code:
Zu guter Letzt: Nicht entmutigen lassen.
Es braucht Zeit, bis man lernt, gut zu programmieren. Wenn man seinen Code veröffentlicht und dann erstmal eine lange Liste mit Verbesserungsvorschlägen bekommt, kann das entmutigend wirken, aber es hilft einem, zu erkennen, was man besser machen kann.
ein paar allgemeine Anmerkungen und Verbesserungsvorschläge zu deinem Code:
- Beachte PEP8. Das macht das Lesen und Verstehen deines Codes für dich und andere (und du wirst in ein paar Wochen auch „andere“ sein) einfacher. Insbesondere: Einrückung mit 4 Leerzeichen pro Ebene, `klein_mit_unterstrichen` als Name für Variablen und Funktionen.
- `GROẞ_MIT_UNTERSTRICHEN` ist Konstanten vorbehalten.
- Auf Modulebene sollten nur Funktionen, Klassen und Konstanten definiert werden. Globale Variablen sind keine Lösung für irgendein Problem, sie sind das Problem.
- Zusammengehörige Daten sollten in einer Datenstruktur zusammengefasst werden. In diesem Fall sind das etwa die Adresse des LCDs und der Status der Hintergrundbeleuchtung (und auch das `bus`-Objekt). In einfachen Fällen bietet sich ein Wörterbuch oder ein `namedtuple` an, aber wenn es auch zu den Daten gehörende Funktionen gibt (hier die `lcd_*`-Funktionen), dann bietet sich eine Klasse an. Die Daten, die für jedes Display unterschiedlich sind, also etwa die Adresse oder der Status der Hintergrundbeleuchtung, werden dann zu Attributen der Instanzen der Klasse, Konstanten wie `LCD_LINE_*` werden zu Konstanten, die auf Klassenebene definiert werden, die Funktionen werden zu Methoden.
- Zeile 95: `for i in range(len(sequence)):` ist in Python ein Anti-Pattern, da man direkt über die Element einer Sequenz iterieren kann. Außerdem muss `ord` keine Zahl zurückgeben, die sich in einem Byte darstellen lässt: `ord("ẞ") == 7838`. Es würde sich vielleicht anbieten, den übergebenen String in einem Encoding zu encoden, das das LCD versteht (wahrscheinlich ASCII) und dann über die Bytes des entstandenen `bytes`-Objektes zu iterieren (ungetestet):
Code: Alles auswählen
def lcd_string(message, line): lcd_byte(line, LCD_CMD) for byte in message.ljust(LCD_WIDTH).encode("ascii"): lcd_byte(byte, LDC_CHR) - Du importierst `psutil`, aber in `get_ram_info` parst du die Ausgabe von `free`. Das lässt sich einfacher durch `psutil` erledigen.
- Zeile 99: Dateien sollten mit dem `with`-Statement geöffnet werden. Damit spart man sich das manuelle Aufrufen der `close`-Methode und es ist garantiert, dass diese aufgerufen wird, auch wenn eine Exception auftritt. Ungetestet:
Code: Alles auswählen
def get_cpu_temperature(): with open("/sys/class/thermal/thermal_zone0/temp") as temperature: return int(temperature.read()) / 1000 - Zeile 140, 147, ...: Um die Bedingung bei `if` und `elif` müssen (und sollten) keine Klammern geschrieben werden, da sie komplett unnötig sind.
- Zeile 158f.: Du möchtest hier Zeichenkettenformatierung benutzen, anstatt zu runden:
Problematisch am Runden ist, dass es Genauigkeit verliert und somit, sobald man mit dem gerundeten Wert weiter rechnet, zu falschen Ergebnissen führt.
Code: Alles auswählen
lcd_string("RAM Used {:.1f} MB".format(ram_used), LCD_LINE_1) lcd_string("RAM Free {:.1f} MB ".format(ram_free), LCD_LINE_2) # oder wenn du Python 3.6 benutzt lcd_string(f"RAM Used {ram_used:.1f} MB", LCD_LINE_1) lcd_string(f"RAM Free {ram_free:.1f} MB", LCD_LINE_2) - Zeile 176 u. 189: `os.system` und `os.popen` sollte nicht benutzt werden, die Funktionen aus dem `subprocess`-Modul ersetzen diese.
- Zeile 211: Ein `pass` hat keinen Effekt. Nie. Es macht also keinen Sinn, `pass` in einem nichtleeren Block zu schreiben.
- Die Funktionen `red`, `green`, `blue` und `black` könnte man auch abstrahieren. Ich nehme an, dass an den drei GPIOs eine RGB-LED angeschlossen ist? Da könnte man eine Klasse `RgbLed` schreiben, der man die drei Pins übergibt, an denen die LED angeschlossen ist und die eine Methode `set_color` (o. Ä.) hat, der man einen RGB-Farbcode übergibt und die dann an die drei Pins ein passendes PWM-Signal anlegt. Die Klasse würde sich dann auch um die `GPIO.setup`- und `GPIO.output`-Aufrufe für die drei Pins kümmern.
Zu guter Letzt: Nicht entmutigen lassen.
Es braucht Zeit, bis man lernt, gut zu programmieren. Wenn man seinen Code veröffentlicht und dann erstmal eine lange Liste mit Verbesserungsvorschlägen bekommt, kann das entmutigend wirken, aber es hilft einem, zu erkennen, was man besser machen kann.