@Cyress: Da sind einige unnötige Semikolons an Zeilenenden.
`math`, `pygame`, alles aus `pygame.locals` und `sleep` aus `time` werden importiert aber nirgends verwendet. `subprocess` wird zweimal importiert. ``as`` bei Importen ist zum umbenennen, `GPIO` wird aber gar nicht umbenannt. Der Code verwendet `Path` ohne die Klasse importiert zu haben.
Das was Du da mit ``globals()`` machst, bitte gleich wieder vergessen, ebenso das ``global``-Schlüsselwort. Man verwendet keine globalen Variablen und schon gar nicht *so* kryptisch. Auf Modulebene macht das zudem überhaupt gar keinen Sinn das so durch die Brust ins Auge zu machen.
Auf Modulebene sollte nur Code stehen der Konstanten, Funktionen, und Klassen definiert. Das Hauptprogramm steht üblicherweise in einer Funktion die `main()` heisst.
Die globalen Namen `Scount`, `command` und `now` werden definiert aber nirgends verwendet.
Namen werden in Python klein_mit_unterstrichen geschrieben. Ausnahmen sind Konstanten (KOMPLETT_GROSS) und Klassen (PascalCase). Namen sollten keine kryptischen Abkürzungen enthalten, oder gar nur daraus bestehen.
Namen sollten nicht durchnummeriert werden. Da sollte man sich entweder passendere Namen überlegen, oder man will gar keine Einzelnamen, sondern eine Datenstruktur verwenden. Oft eine Liste.
Im Falle von `alarm1` bis `alarm3` braucht man aber nicht wirklich drei Variablen, denn die starten mit 0 und werden dann nacheinander zu 1 — diese Information könnte man auch einfach in *einen* Alarmzähler stecken der von 0 bis 3 hochzählt und damit angibt wie viele Alarme es bereits gab.
Python hat einen Datentyp für Wahrheitswerte, da sollte man nicht 0 und 1 für missbrauchen.
Das aktuelle Arbeitsverzeichnis an `current_dir` zu binden macht keinen Sinn. An der Stelle wo Du das dann benutzt geht der Code zudem davon aus, dass das Ergebnis von `os.getpwd()` ein absoluter Pfad ist. Ich denke das ist nicht garantiert.
`camera_pause` ist eigentlich eine Zahl, die sollte man nicht als Zeichenkette definieren. Wenn man die später als Zeichenkettendarstellung braucht, kann man sie *dann* umwandeln.
`stupload` ist etwas ziemlich schräges. Das ist eine globale Variable die GUI-Text enthält, der vom Zustand vom Attribut/Flag `actives` in der `GUI`-Klasse abhängt. Also erst einmal ist es komisch in der `Relays.upload()` den Text zu vergleichen, statt den dazugehörigen Wahrheitswert zu verwenden. Und dann macht es wenig Sinn diese Methode überhaupt aufzurufen wenn man schon vor dem Aufruf weiss, das man gerade gar nichts hochladen möchte. Dann ruft man diese Methode einfach nicht auf. Das setzen des Wertes von `stupload` ist auch redundant programmiert weil da erst ein ``if``/``else`` steht das diesen Wert und das `actives`-Flag festlegt und gleich danach Code folgt der `self.actives` (hoffentlich!) mit dem gleichen Wert überschreibt. Wobei das bisher sogar egal wäre, weil `self.actives` bisher gar nicht wirklich verwendet wird.
`filename` ist auch sowas von unnötig als globale Variable. Der Wert wird in einer Methode eines Objekts global gesetzt um dann in von dort aufgerufenen Methoden in einem anderen Objekt benutzt zu werden. Das ist so etwas von eindeutig ein Fall für ein Argument statt einer globalen Variable.
Ebenfalls unnötig ist das eigenartig benannte globale `nownow`. Das ist eine globale Variable über die ein Wert *innerhalb* eines Objekts kommuniziert wird, der da bereits als Attribut existiert.
Beim `GPIO`-Modul sollte man dafür sorgen, dass am Programmende die `GPIO.cleanup()`-Funktion aufgerufen wird.
Die `Relays`-Klasse ist gar keine Klasse sondern einfach nur ein paar Funktionen in eine Klasse gesteckt. Zudem auch noch Funktionen die gar nicht alle zusammengehören, wie das schalten der Relays (oder ist es nur eines?) und das aufnehmen, verschieben, und hochladen eines Fotos. Der Pin-Wert der beim erstellen übergeben wird (22) wird nirgends verwendet. Das sollte man tun und dann alles ausser `on()` und `off()` aus dieser Klasse rauswerfen.
Und das Attribut in der GUI würde ich dann von `relays` nach `horn` umbenennen, damit der Leser weiss *was* da geschaltet wird, denn ein `self.horn.on()`/`self.horn.off()` ist wesentlich aussagekräftiger als ein genrisches `self.relays.on()`/`self.relays.off()`
Das `start_value`-Argument in `GUI.__init__()` wird sofort durch einen anderen, festen Wert überschrieben womit man sich das als Argument hätte sparen können.
Das `on_time` Argument/Attribut wird nirgends verwendet.
`states` und `statess` sind keine guten Namen. Erstens viel zu generisch und zweitens viel zu ähnlich und drittens ist das zusätzliche `s` beim zweiten Namen total kryptisch/nichtssagend. Das gleiche gilt für das ”parallele” Attributpaar `active` und `actives`.
Das mit den `cycle()`\s ist für das toggeln eines Wahrheitswerts IMHO Kanonen auf Spatzen. Da würde man statt Iterator und Flag einfach eine Liste mit den beiden Texten und das Flag verwenden.
Beim Namen `wechsel_upload_download()` macht der `download`-Teil keinen Sinn weil es nur einen Upload gibt.
Funktionen und Methodennamen beschreiben üblicherweise die Tätigkeit die sie durchführen. Dann weiss der Leser was die tun und kann sie auch von eher passiven Werten unterscheiden. `foto` wäre ein guter Name für ein Foto, aber nicht für eine Methode die ein Foto aufnimmt. Analog gilt für das für `horn` und hier gibt es ja sogar eine Namenskollision — wenn man das `Relay`-Objekt `horn` nennt, kann die Methode nicht genau so heissen.
Bei einem `set_value()`-Namen erwartet der Leser, das er dort einen Wert übergeben kann, der dann irgendwie gesetzt wird. Die Methode setzt aber keinen Wert sondern aktualisiert die Countdown-Variable. Der Code kommt auch noch in der `reset()`-Methode vor, statt diese Methode aufzurufen. Und in `foto()` wird der Anzeigewert auch noch mal erzeugt, statt den vorhandenen Wert aus dem `StringVar`-Objekt zu verwenden.
Die ``lambda``-Ausdrücke sind unnötig, da kann man einfach die Methode übergeben die aufgerufen werden soll. Ausserdem gibt es die Code für Horn an/nach einer Sekunde einmal als Methode und dann noch mehrfach im Code statt einfach die Methode aufzurufen.
`x` ist kein guter name für eine Zeichenkette die den aktuellen, formatierten Countdownwert enthält. Und das ist bereits eine Zeichenkette. Es macht keinen Sinn `str()` damit aufzurufen.
In `countdown()` schliessen sich die Bedingungen der ersten drei ``if``\s gegenseitig aus, das ist also ein Fall für ``elif``.
Wenn man den gleichen Wert gegen zwei Grenzwerte testet und diese beiden Tests mit ``and`` verknüpft, ist es kürzer und lesbarer wenn man stattdessen verkettete Vergleiche verwendet. Also statt ``x > a and x < b`` besser ``a < x < b``.
Wenn man sich die drei ``if``-Bedingungen anschaut und was da jeweils gemacht wird, ist das alles sehr regelmässig strukturiert. Bedingung und Code der jeweils abhängig ausgeführt wird, hängt im Grunde nur von den Grenzwerten beim Test und der Anzahl der bisherigen Alarme ab. Das kann man also kürzer schreiben in dem man eine Schleife über die variablen Teile schreibt und den Code nur *einmal* statt dreimal.
Wenn der Countdown nicht läuft, braucht man auch die Alarme nicht behandeln, oder?
`takepic()` verwendet noch `os.system()`.
`movepic()` startet unnötigerweise ein externes Programm statt das `shutil`-Modul zum Verschieben der Datei zu verwenden. In der Funktion fehlt auch irgendwie der zweite Pfad — man braucht zum verschieben ja Quelle und Ziel.
In `upload()` wird eine Zeichenkette zusammengebastelt und ``shell=True`` verwendet. Ausserdem ist `path` undefiniert.
Man sollte `subprocess.Popen` so nicht verwenden. Da wird nie das Ergebnis abgefragt, womit bei jedem Upload ein Zombieprozess entsteht der beim Betriebssystem in der Prozesstabelle vor sich hin vegetiert bis das Programm beendet wird.
Zwischenstand (ungetestet):
Code: Alles auswählen
#!/usr/bin/env python3
import datetime
import shutil
import subprocess
import time
from pathlib import Path
from threading import Thread
from tkinter import Button, Label, StringVar, Tk, X
from tkinter.font import Font
from RPi import GPIO
KAMERA_PIN = 17
HORN_PIN = 21
FTP_HOST = "ftp.strato.de"
FTP_USERNAME = "xxxx"
FTP_PASSWORD = "xxxx"
DEBUG = True
IMAGE_PATH = Path("Pictures")
CAMERA_PAUSE = 1000
def take_picture(filename):
if DEBUG:
print("taking photo...")
command = [
"sudo",
"raspistill",
"--output",
filename,
"--quality",
"100",
"--rotation",
"270",
"--timeout",
str(CAMERA_PAUSE),
]
if DEBUG:
print(command)
subprocess.run(command, check=True)
def move_picture(filename):
if DEBUG:
print("Move pic")
shutil.move(filename, IMAGE_PATH)
def upload_picture(filename):
command = [
"ftp-upload",
"--FTP_HOST",
FTP_HOST,
"--user",
FTP_USERNAME,
"--password",
FTP_PASSWORD,
"--dir",
"/",
str(IMAGE_PATH / filename),
]
if DEBUG:
print("FTP Upload Befehl:", command)
subprocess.run(command, check=True)
class Relay:
def __init__(self, pin):
if DEBUG:
print("-init-")
self.pin = pin
def on(self):
if DEBUG:
print("ON")
GPIO.output(self.pin, 1)
def off(self):
if DEBUG:
print("OFF")
GPIO.output(self.pin, 0)
class GUI(Tk):
COUNTDOWN_STATE_TEXTS = ["Start", "Stop"]
ON_OFF_TEXTS = ["on", "off"]
COUNTDOWN_UPDATE_RATE = int(1 / 60 * 1000)
def __init__(self, countdown_start_value, horn):
super().__init__()
self.title(f"{countdown_start_value} Countdown")
self.countdown_start_value = countdown_start_value
self.horn = horn
self.countdown_value_var = StringVar()
self.current_value = None
self.alarm_count = None
self.reset()
self.is_countdown_running = False
self.should_upload = False
font = Font(family="Lucida Grande", size=20)
Label(self, text="Regatter System C Peterseim", font=font).pack(fill=X)
Label(self, textvariable=self.countdown_value_var, font=font).pack(
fill=X
)
self.start_stop_knopf = Button(
self,
text=self.COUNTDOWN_STATE_TEXTS[self.is_countdown_running],
font=font,
command=self.wechsel_start_stop,
)
self.start_stop_knopf.pack(fill=X)
Button(self, text="Zurücksetzen", command=self.reset, font=font).pack(
fill=X
)
Button(self, text="Horn", command=self.sound_horn, font=font).pack(
fill=X
)
Button(self, text="FOTO", command=self.take_photo, font=font).pack(
fill=X
)
self.upload_toggle_knopf = Button(
self,
text=f"Upload {self.ON_OFF_TEXTS[self.should_upload]}",
font=font,
command=self.wechsel_upload_download,
)
self.upload_toggle_knopf.pack(fill=X)
Button(self, text="Verlassen", command=self.quit, font=font).pack(
fill=X
)
self.photo_count = 1
self.take_photo()
self.last_timestamp = time.monotonic()
self.after(self.COUNTDOWN_UPDATE_RATE, self.countdown)
def update_countdown_display(self):
self.countdown_value_var.set(f"{self.current_value:.2f}")
def reset(self):
if DEBUG:
print("reset")
self.current_value = self.countdown_start_value
self.alarm_count = 0
self.update_countdown_display()
def wechsel_start_stop(self):
self.is_countdown_running = not self.is_countdown_running
self.start_stop_knopf["text"] = self.COUNTDOWN_STATE_TEXTS[
self.is_countdown_running
]
def wechsel_upload_download(self):
self.should_upload = not self.should_upload
self.upload_toggle_knopf["text"] = self.ON_OFF_TEXTS[
self.should_upload
]
def sound_horn(self):
self.horn.on()
self.after(1000, self.horn.off)
def take_photo(self):
if DEBUG:
print("Foto")
self.photo_count += 1
if self.photo_count > 2:
if DEBUG:
print("Lichtschranke ausgelöst")
time_text = f"{datetime.datetime.now():%Y-%m-%d_%H:%M:%S}"
countdown_text = self.countdown_value_var.get()
print("request received:", time_text, "Timer:", countdown_text)
filename = f"Bild-{time_text}-{countdown_text}.jpg"
take_picture(filename)
move_picture(filename)
if self.should_upload:
Thread(target=upload_picture, args=[filename]).start()
def countdown(self):
#
# FIXME This is unnecessarily imprecise. Do not measure times between
# updates to update/calculate the current countdown value but remember
# the end time when the countdown is started and calculate the current
# countdown value from the current time and that end time.
#
current_time = time.monotonic()
delta_time = current_time - self.last_timestamp
self.last_timestamp = current_time
if self.is_countdown_running:
for alarm_count, lower_limit in enumerate([9, 4, -1]):
if (
lower_limit < self.current_value < lower_limit + 1
and self.alarm_count == alarm_count
):
self.alarm_count += 1
if DEBUG:
print(f"ALARM{self.alarm_count}")
self.sound_horn()
break
self.current_value -= delta_time
self.update_countdown_display()
if GPIO.input(KAMERA_PIN):
self.take_photo()
self.after(self.COUNTDOWN_UPDATE_RATE, self.countdown)
def main():
print("Regata Zeitaufname")
print("Mark Peterseim")
try:
GPIO.setmode(GPIO.BCM)
GPIO.setup(KAMERA_PIN, GPIO.IN)
GPIO.setup(HORN_PIN, GPIO.OUT)
gui = GUI(20, Relay(HORN_PIN))
gui.mainloop()
finally:
GPIO.cleanup()
if __name__ == "__main__":
main()
Das mit dem Countdown ist schlecht gelöst weil da nicht immer vom gleichen Startpunkt gerechnet wird, sondern bei jedem mal die vergangene Zeit vom letzten Aufruf manuell ausgerechnet und abgezogen wird. Damit handelt man sich völlig ohne Not Ungenauigkeiten ein. Beim Start des Countdowns sollte man *einmal* das Ende berechnen und sich merken, und ab da dann den aktuellen Wert immer aus der aktuellen Zeit und diesem Referenzwert berechnen.
Einen Button um einen Zustand zu wechseln mit entsprechend wechselnden Texten könnte man als eigene Klasse implementieren um Codewiederholungen zu vermeiden.