@mayr_2112: Weitere Anmerkungen zum Quelltext im ersten Beitrag:
`time` zweimal importiert. In direkt aufeinanderfolgenden Zeilen.
``as`` beim Import ist zum Umbennenen da, `SPI` aus `Adafruit_GPIO` wird aber gar nicht umbenannt. Wobei das Modul im Code auch überhaupt nicht verwendet wird‽
Konstanten werden komplett in Grossbuchstaben benannt.
Auf Modulebene sollte nur Code stehen der Konstanten, Funktionen, und Klassen definiert. Das Hauptprogramm steht üblicherweise in einer Funktion die `main()` heisst. Daraus folgt dann auch, dass Funktionen und Methoden alles was sie ausser Konstanten benötigen als Argument(e) übergeben bekommen. `LCD_Temperatur()` bekommt dann allerdings 11 Argumente, was ein Zeichen dafür ist das in dieser Funktion zu viel unterschiedliches gemacht wird, oder das man anfangen sollte Werte sinnvoll zu Objekten zusammenzufassen.
Warnungen sollte man nicht ignorieren, sondern deren Ursache beseitigen. Beim `GPIO`-Modul ist es wichtig sicherzustellen das am Ende des Programmablaufs, egal warum das beendet wird, die `cleanup()`-Funktion aufgerufen wird.
Warum steht bei dem aufsetzen der Taster-Pins noch mal der Variablenname aus dem Code als Kommentar in der Zeile?
Statt eines nichstsagenden `Relay` an das unsinnigerweise auch noch eine ebenfalls nichtssagende 1 angehängt wurde, und einem Kommentar, dass das der Pin für die Heizspule ist, sollte man da einen Namen wählen der das aussagt und den Kommentar weglassen.
Der Kommentar ``Sensor einlesen`` mach inhaltlich überhaupt keinen Sinn.
`exit()` gibt es eigentlich gar nicht. Das ist undokumentiert. Wenn man die Funktion braucht, dann sollte man sie explizit aus dem `sys`-Modul nehmen. Und man sollte sie nur verwenden wenn man zumindest potentiell auch einen anderen Rückgabecode als 0 übermitteln möchte. Möchte man in diesem Fall sogar, denn 0 bedeutet, dass das Programm ohne Probleme durchlief, was ja nicht der Fall ist wenn `pi` nicht mit dem Daemon verbunden ist. Das sollte man testen *bevor* man `spi_open()` aufruft. Danach macht es wenig Sinn.
Den `sensor` könnte man auch `temperature_sensor` nennen, damit der Leser weiss was das für ein Sensor ist. Und auch hier sollte man wieder sicherstellen, dass am Programmende diese Ressourcen ordentlich aufgeräumt werden. Sowohl das `pi`-Objekt als auch die SPI-Verbindung.
Wenn man `display` meint, sollte man nicht nur `disp` schreiben.
Faustregel für Kommentare: Die beschreiben nicht *was* der Code macht, denn das steht da ja bereits als Code, sondern warum der das so macht. Sofern das nicht offensichtlich ist. Da sind so einige Kommentare überflüssiger Ballast.
Das gefüllte Rechteck zeichnen im Hauptprogramm kann man sich sparen, weil `Image.new()` bereits ein leeres Bild liefert.
`bottom` wird definiert, aber nirgends verwendet. `x` ist immer 0, der Kommentar darüber suggiert verwirrenderweise etwas anderes. Wenn man das `x` rauswirft, hat `LCD_Temperatur()` schon mal ein Argument weniger. Da die Funktion auch das Bildobjekt übergeben bekommt, braucht man ihr auch `width` und `height` gar nicht übergeben, denn diese Informationen kann man vom Bildobjekt abfragen. Wieder zwei Argumente eingespart.
Die beiden Temperaturgrenzen werden in der Schleife immer neu berechnet obwohl sich die Werte davon nie ändern.
`getSensorTemp()` ist fehlerhaft. Wenn *nicht* zwei Bytes gelesen werden konnten, ist `Istwert` undefiniert und führt beim Versuch das zurückzugeben zu einem `NameError`.
`c`, `d`, und `t` sind keine guten Namen. `t` ist auch überflüssig. 987°C ist ein extrem komischer und willkürlicher Wert für eine Temperatur die gar nicht gelesen werden konnte. Wenn man schon keine Ausnahme auslösen möchte und das unbedingt über einen Wert regeln möchte, dann sollte der auch ”sinnvoll” sein. Also beispielweise NaN wenn es vom Typ `float` sein soll oder `None`.
Eine Funktion die einen Sensor abfragt sollte keine Ausgaben mit `print()` machen. Für so etwas eignet sich Logging weil man das an- und abschalten kann, je nach dem ob man diese Infos haben möchte oder nicht. Und man kann sie auch in eine Datei oder woanders hin schicken wenn man sie dauerhaft sichern und auswerten möchte.
`LCD_Temperatur()` ist nicht nur von der Schreibweise her falsch für eine Funktion, sondern auch inhaltlich. Funktionen werden üblicherweise nach ihrer Tätigkeit benannt. Die Funktion macht auch zu viel verschiedenes. Die Ermittelt eine IP mit einem externen Programm, formatiert einzelne Werte in Zeichenketten, und aktualisiert das LCD. Die sollte einfach nur alles notwendige zum Anzeigen von mehreren Zeichenketten auf dem Display übergeben bekommen und nichts davon wissen müssen was das für Werte sind und wo die her kommen. Dann werden das auch gleich wieder weniger Argumente, denn die Zeichenketten die angezeigt werden sollen übergibt man sinnvollerweise als Liste. Dann lässt sich das Anzeigen auch mit einer Schleife mit weniger Code-Wiederholungen schreiben. Und es sind wieder 3 Argumente weniger.
Die ersten vier Argumente beschreiben zusammen den Zustand der Anzeige und verändern sich nicht. Die könnte man also sinnvollerweise in einem Objekt zusammenfassen. Dann bleiben von den 11 Argumenten am Ende nur noch zwei übrig.
Die zweite Zuweisung an `cmd` ist überflüssig, weil der Wert nirgends verwendet wird. In der ersten Zuweisung sind die \ in der Zeichenkette überflüssig. Der Wert kommt auch überhaupt nicht von der Webseite die im Kommentar davor genau das behauptet.
Zum aufrufen von externen Programmen: ``shell=True`` sollte man nur verwenden wenn man tatsächlich eine Shell braucht. Die wird bei der IP nur wegen der Pipe in das ``cut`` benutzt, aber das was ``cut`` da macht, kann man problemlos in Python lösen. Und man möchte an der Stelle auch Text und keine Bytes zurückbekommen.
Zwischenstand (ungetestet):
Code: Alles auswählen
#!/usr/bin/env python3
import subprocess
import sys
import time
from collections import namedtuple
import Adafruit_SSD1306
import pigpio
from loguru import logger
from PIL import Image, ImageDraw, ImageFont
from RPi import GPIO as gpio
TEN_UP_BUTTON_PIN = 23
TEN_DOWN_BUTTON_PIN = 24
ONE_UP_BUTTON_PIN = 25
ONE_DOWN_BUTTON_PIN = 26
HEATING_COIL_PIN = 5
DISPLAY_RST = None
Display = namedtuple("Display", "display image draw font x_offset y_offset")
def create_display(rst, x_offset=0, y_offset=0):
display = Adafruit_SSD1306.SSD1306_128_64(rst)
display.begin()
display.clear()
display.display()
image = Image.new("1", (display.width, display.height))
return Display(
display,
image,
ImageDraw.Draw(image),
ImageFont.load_default(),
x_offset,
y_offset,
)
def update_display(display, lines):
display.draw.rectangle(
(0, 0, display.image.width, display.image.height), outline=0, fill=0
)
for i, line in enumerate(lines):
display.draw.text(
(display.x_offset, display.y_offset + i * 16),
line,
font=display.font,
fill=255,
)
display.display.image(display.image)
display.display.display()
def get_temperature(pi, sensor_handle):
temperature = float("nan")
byte_count, data = pi.spi_read(sensor_handle, 2)
if byte_count == 2:
word = (data[0] << 8) | data[1]
#
# Bits 15, 2, und 1 müssen 0 sein, laut Datenblatt.
#
if (word & 0b1000_0000_0000_0110) == 0:
temperature = (word >> 3) / 4
logger.debug("{:.2f}°C", temperature)
else:
logger.error("bad reading {:b}", word)
else:
logger.error("wrong byte count: {}", byte_count)
return temperature
#
# TODO Die Funktion ist zu lang. Sinnvoll in Funktionen aufteilen.
#
@logger.catch
def main():
try:
gpio.setmode(gpio.BCM)
gpio.setup(
[
TEN_UP_BUTTON_PIN,
TEN_DOWN_BUTTON_PIN,
ONE_UP_BUTTON_PIN,
ONE_DOWN_BUTTON_PIN,
],
gpio.IN,
)
gpio.setup(HEATING_COIL_PIN, gpio.OUT)
pi = pigpio.pi()
try:
if not pi.connected:
sys.exit(1)
temperature_sensor_handle = pi.spi_open(0, 1_000_000)
try:
display = create_display(DISPLAY_RST, 0, -2)
target_temperature = float(input("Sollwert: "))
hysteresis = float(input("Hysterese: "))
min_temperature = target_temperature - hysteresis
max_temperature = target_temperature + hysteresis
#
# TODO Hier sollte eigentlich eine Abfrage eines Starttasters
# stattfinden.
#
while True:
#
# TODO `psutil` statt eines externen Programms verwenden.
# Und dann möchte man auch die Netzwerkschnittstelle angeben
# von der man die IP haben möchte und nicht einfach
# irgendeine der Möglichkeiten zufällig auswählen.
#
ip_addresses = subprocess.check_output(
"hostname --all-ip-addresses", universal_newlines=True
).split()
temperature = get_temperature(
pi, temperature_sensor_handle
)
update_display(
display,
[
"IP: {}".format(ip_addresses[0]),
"Isttemp. = {:.2f} °C".format(temperature),
"Solltemp. = {:.2f} °C".format(target_temperature),
"Hysterese = +-{:2f} °C".format(hysteresis),
],
)
if temperature < min_temperature:
logger.debug("relay an")
gpio.output(HEATING_COIL_PIN, gpio.HIGH)
elif temperature > max_temperature:
logger.debug("relay aus")
gpio.output(HEATING_COIL_PIN, gpio.LOW)
time.sleep(0.5)
finally:
pi.spi_close(temperature_sensor_handle)
finally:
pi.stop()
finally:
gpio.cleanup()
if __name__ == "__main__":
main()
