Externes Skript starten + nicht auf Beendigung warten + WorkDir ändern

Code-Stücke können hier veröffentlicht werden.
Antworten
Zeruat
User
Beiträge: 2
Registriert: Freitag 8. Dezember 2017, 20:27

Heyho allesamt,

erst einmal sorry für den nicht ganz so klaren Titel!

Es geht um folgendes:


Ich habe ein kleines Programm geschrieben, welches als "Pseudo-Daemon" 24/7 auf meinem Server laufen soll. Leider bekomme ich noch manchmal Fehler, woraufhin das Programm abstürzt.
Um dem vorzubeugen, habe ich eine PHP Seite eingerichtet, welche mir beim Aufruf eine start.txt erstellt, sodass ich kurz und schmerzlos das Skript neu starten kann. Das Ganze soll mit einem zweiten Skript realisiert werden.
Undzwar mit dieser Logik: Das zweite Skript verbindet sich alle 30 Sekunde per FTP mit dem Server und schaut, ob die erstellte Datei vorhanden ist. Falls ja, soll die vorherige Instanz des eigentlichen Programms geschlossen werden (taskkill) und daraufhin noch einmal neu gestartet werden.
Falls nein, soll in 30 Sekunden wieder nachgeschaut werden.

Mein Problem ist jetzt folgendes:
1. das Verzeichnis in dem das Skript ausgeführt werden soll, ist ein anderes als das wo es momentan liegt.
2. da das auszuführende Skript dann in Endlosschleife läuft, soll nicht auf die Beendigung des gestarteten Skripts gewartet werden

Ich habe schon den ganzen Mittag gegooglet, und es mit os.spawn*, subprocess.call/run, popen usw.. probiert, aber leider einfach keine Lösung zusammengebaut bekommen...

Code: Alles auswählen

from ftplib import FTP
import time
import os
import subprocess

global ftp

run = 'true'
startfile = 'start.txt'

def connectftp():

    global ftp

    ftp = FTP('SERVER')
    ftp.login('USERNAME,'PASSWORT')
    ftp.cwd('DOCUMENTROOT')

def getcomfile():

    global ftp

    file_list = ftp.nlst()
    if startfile in file_list:
        ftp.delete(startfile)
        os.system('taskkill /IM conhost.exe /f')
        os.system('taskkill /IM cmd.exe /f')
        ## STARTE 'python menu.py -autoload 120 new' in 'D:\Refactored'
        file_list = ['']
    else:
        pass


connectftp()
while run == 'true':
    getcomfile()
    time.sleep(30)
Ich wäre echt dankbar, wenn mir da jemand zu helfen wüsste !

Edit: Das Ganze läuft auf einem Windows System.

P.S.: Sorry wenns im falschen Topic gelandet ist, bin neu hier
P.P.S: Mist, ich glaube es ist im falschen Topic gelandet, wäre super wenn ein Moderator es verschieben würde.


LG,
Zeruat
__deets__
User
Beiträge: 14494
Registriert: Mittwoch 14. Oktober 2015, 14:29

Warum so kompliziert? Zum ersten kann Python viele Fehler selbst anfangen. Abef selbst wenn wir das Neustart Modell beibehalten, was durchaus gut ist, hat das doch nix mit php und ftp zu tun. Schreib eine Systemd Unit. Oder ein wrapper Skript, welches dein eigentliches Skript startet und bei bedarf neu startet.
Zeruat
User
Beiträge: 2
Registriert: Freitag 8. Dezember 2017, 20:27

Danke für die rasch Antwort!

Die Fehlerproblematik ist eine andere Baustelle...

Habe vergessen zu erwähnen dass das Skript auf einem anderen (nicht Web-) Server läuft.
Das eigentliche "Haupt"Skript lädt Daten aus dem Internet runter, connected sich auf den Webserver, auch via ftp, und lädt nach jedem durchgang eine html mit Statistik-Daten inkl Timestamp hoch. Auf diese "Website" greife ich dann im Alltag zu um das Ganze zu überwachen. Deshalb bin ich auf die Idee gekommen, im Falle von länger nicht aktualisieren Daten, per Aufruf eines Links, (der Einfachheit halber) das Skript neu starten zu lassen...

Klappt auch alles super bis auf den Aufruf des neu zu startenden Skripts

Nochmal zusammengefasst:
Skript A läuft auf Server und lädt Daten aus dem Netz herunter.
Nach jedem Durchgang wird zu Monitoring Zwecken eine HTML generiert und auf einen externen Webserver hochgeladen. Somit kann ich den aktuellen Stand im Alltag mit jedem Internetfähigen Gerät schnell mal überprüfen. Falls ich sehe dass seit zb 10 Minuten nichts mehr aktualisiert wurde, wird mit Aufruf einer php eine Datei auf dem Webserver erzeugt, auf welche Skript B reagieren soll.
Auf dem Server wo Skript A läuft, läuft auch Skript B, welches alle 30Sek überprüft ob auf dem besagten Webserver diese txt existiert. Wenn diese txt existiert sollen alle Konsolenfenster geschlossen werden, und das Skript A neu gestartet werden. Skript A sollte in einer Endlosschleife laufen, d. h. Skript B, welches weiterhin nach der txt ausschau halten soll, soll Skript A starten, und unabhängig von Skript A weiter in der Schleife laufen...
__deets__
User
Beiträge: 14494
Registriert: Mittwoch 14. Oktober 2015, 14:29

So richtig verstanden habe ich das noch nicht. Was haben denn Konsolenfenster damit zu tun? Die sind doch völlig unnötig für das laufen lassen eines Skripts.

Und so oder so ist doch jedes Kriterium, das du nach außen kommunizierst etwas, das du ja per Definition lokal bestimmt hast, auch geeignet Lokal alles wieder zu starten. Web, PHP, all das hat damit doch nix zu tun.
Sirius3
User
Beiträge: 17711
Registriert: Sonntag 21. Oktober 2012, 17:20

@Zeruat: auch wenn das Konstrukt sich reichlich kompliziert anhört, weil das Skript B auch einfach nach einer lokal abliegenden Datei schauen könnte und selbständig neustarten, hat Dein Programm auch sonst noch einige Probleme. Vergiß dass es soetwas wie `global` gibt, das löst keine Probleme sondern schafft nur viele andere; `global` auf Modulebene hat gar keine Wirkung. Es gibt Wahrheitswerte True und False, nutze die, statt eines Strings `'true'`. Da Du eine Endlosschleife willst, ist `run` auch überflüssig, weil es sich nie ändert. Funktionen haben Rückgabewerte, statt also ein globales `ftp` zu erzeugen, sollte `connectftp` einfach sein lokales `ftp` zurückgeben. Du ignorierst, dass die FTP-Verbindung zusammenbrechen könnte, oder sonst ein FTP-Fehler auftreten könnte; dann stürzt Dein Programm ab und Du hast kein Kontrollskript mehr laufen. Du importierst schon `subprocess`, benutzt es aber nicht. Damit kann man ganz einfach angeben, aus welchem Verzeichnis das Programm gestartet werden soll. Was ist der Sinn, `file_list` eine Liste mit einem leeren String als einziges Element zuzuweisen und es dann nicht zu benutzen? Wenn im else-Zweig nur pass steht, kann man ihn auch ganz weglassen. Wenn Du das Python-Programm beenden willst, warum beendest Du dann zwei völlig andere Programme?

Code: Alles auswählen

import time
import subprocess
from ftplib import FTP

RESTARTFILE = 'start.txt'

def connectftp():
    ftp = FTP('SERVER')
    ftp.login('USERNAME', 'PASSWORT')
    ftp.cwd('DOCUMENTROOT')
    return ftp


def check_restart():
    # TODO: Fehlerbehandlung
    ftp = connectftp()
    file_list = ftp.nlst()
    if RESTARTFILE in file_list:
        ftp.delete(RESTARTFILE)
        return True
    return False


def start_python():
    return subprocess.Popen(["python", "menu.py", "-autoload", "120", "new"],
        cwd="D:/Refactored")

def main():
    process = start_python()
    while True:
        time.sleep(30)
        if check_restart():
            process.kill()
            process = start_python()

if __name__ == '__main__':
    main()
Cyress
User
Beiträge: 4
Registriert: Montag 8. März 2021, 01:04

Code: Alles auswählen

def upload(self):
        #apt-get install ftp-upload
        if Stupload=="Upload an":
            command = "ftp-upload -h " + HOST + " -u " + USERNAME + " --password " + PASSWORD + " -d  / " + current_dir + "/" + image_dir + "/" + filename
            if Debug==1:
                print ("ftp upload gestartet")
                print ("FTP Upload Befehl:" + command)
            os.system(command)

Habe das selbe problem...
Hätte gerne das das Script nicht wartet bis das Bild hochgeladen ist.
Je nach dem wie groß das Bild ist und wie schnell die verbindung dauert das zu lange.

einer eine idee?



Mit Grüßen aus dem schönen Sauerland

Sven Peterseim
Sirius3
User
Beiträge: 17711
Registriert: Sonntag 21. Oktober 2012, 17:20

Variablennamen schreibt man komplett klein, Stupload, current_dir, image_dir, filename und Debug kommen aus dem nichts, müssen also entweder an self gebunden sein, oder Argumente der Funktion upload.
os.system benutzt man nicht, und mit subprocess.Popen hast Du auch kein Problem mit dem Warten.
Du hast einen if-Pfad, der etwas zurückliefert, der andere nichts. Das sollte nicht sein.
Pfade setzt man nicht mit + zusammen, sondern benutzt pathlib.Path.

Code: Alles auswählen

    def upload(self, stupload, current_dir, image_dir, filename, debug):
        #apt-get install ftp-upload
        if stupload=="Upload an":
            path = Path('/') / current_dir / image_dir / filename
            command = ["ftp-upload", "-h", HOST, "-u", USERNAME, "--password", PASSWORD, "-d", str(path)]
            if debug:
                print("ftp upload gestartet")
                print("FTP Upload Befehl:" + ' '.join(command))
            return subprocess.Popen(command)
        return None
Benutzeravatar
__blackjack__
User
Beiträge: 13004
Registriert: Samstag 2. Juni 2018, 10:21
Wohnort: 127.0.0.1
Kontaktdaten:

Nächste iteration: Da `self` nicht verwendet wird, könnte man einfach eine Funktion daraus machen.

Eine Methode/Funktion die auf grund eines Arguments entscheidet, ob sie überhaupt etwas macht und etwas zurückgibt, oder `None` zurück gibt, ist komisch. Denn das zieht ja eine weitere, redundante, Prüfung nach sich, ob man mit dem Rückgabewert nun was machen kann oder nicht, weil er `None` ist. Da sollte der Aufrufer *einmal* entscheiden ob er das überhaupt aufrufen will oder nicht.

Mindestens `current_dir` würde ich einfach als `Path`-Objekt bereits vom Aufrufer erwarten.

Das `debug`-Argument würde man besser über Logging lösen.

Code: Alles auswählen

def start_ftp_upload(current_path, image_dir, filename):
    command = [
        "ftp-upload",
        "--host",
        HOST,
        "--user",
        USERNAME,
        "--password",
        PASSWORD,
        "--dir",
        "/",
        str(current_path / image_dir / filename),
    ]
    LOG.debug("FTP Upload Befehl: %s", " ".join(command))
    return subprocess.Popen(command)
“Most people find the concept of programming obvious, but the doing impossible.” — Alan J. Perlis
Cyress
User
Beiträge: 4
Registriert: Montag 8. März 2021, 01:04

Vielen Dank!!
ich werde eure Anregungen dieses Wochenende im ganzen Script einzubauen.
Das hier ist mein erstes Python Projekt und ich programmiere auch nur in meiner Freizeit.


Den subprocess Befehl habe ich aber direkt ausprobiert.
return subprocess.Popen(command) hat nicht geklappt aber

subprocess.Popen(command,shell=True) macht genau was ich will.

Ich hoffe das Ihr euch das überarbeitet Script nächste Woche durchliest und mir ein Fit back gibt habe heute viel gelernt.

Mit Grüßen aus dem grade sehr kaltem Sauerland

Sven Peterseim
Sirius3
User
Beiträge: 17711
Registriert: Sonntag 21. Oktober 2012, 17:20

shell=True ist falsch. Wie hast Du meinen oder __blackjacks__Code übernommen? Zeige was du versucht hast.
Cyress
User
Beiträge: 4
Registriert: Montag 8. März 2021, 01:04

Code: Alles auswählen

import math
import time
from itertools import cycle
from tkinter import Label, Button, Tk, StringVar, X
from tkinter.font import Font as TkFont
import pygame
from pygame.locals import *
from time import sleep
import os
import RPi.GPIO as GPIO
import subprocess
import datetime
import subprocess
globals()["alarm1"] = 0;
globals()["alarm2"] = 0;
globals()["alarm3"] = 0;
#setup pins
Kamaragpio = 17
horngpio = 21
GPIO.setmode(GPIO.BCM)
GPIO.setup(Kamaragpio,GPIO.IN)
GPIO.setup(horngpio,GPIO.OUT)
#setup variables
host = "ftp.strato.de"
username = "xxxx"
password = "xxxx"
command = ""
globals()["debug"] =1;
current_dir = os.getcwd()
globals()["stupload"] = "Upload off";
filename = ""
Scount = 1 
image_dir = "Pictures"
globals()["now"] ="";
camera_pause  = "1000"
count= 1
print("Regata Zeitaufname")
print("Mark Peterseim")
class Relays:
   
    def __init__(self, pin):
        if debug:
            print("-init-")
        self.pin = pin
        

    def on(self):
        if debug:
            print("-init- on")
            print("ON")
        GPIO.output(horngpio,1)

    def off(self):
        if debug:
            print("- init-off")
            print("OFF")
        GPIO.output(horngpio,0)

    def takepic(self):
        if debug:
            print("taking photo...")
        command = "sudo raspistill -o " + filename + " -q 100 -rot 270 -t " + camera_pause
        if debug:
            print(command)
        os.system(command)
        
    def movepic(self):
        path = Path('/') / current_dir / image_dir / filename
        command = "mv " + path
        if debug:
            print("Move pic" + command)
        os.system(command)

    def upload(self):
        if stupload=="Upload an":
            command = "ftp-upload -h " + host + " -u " + username + " --password " + password + " -d  / " + path
            if debug:
                print ("ftp upload gestartet")
                print ("FTP Upload Befehl:" + command)
            subprocess.Popen(command,shell=True)
                    

class GUI(Tk):
    def __init__(self, start_value, relays, on_time=1):
        """
        :param start_value: für den Countdown
        :param relays: Zum Ein-/Ausschalten
        :param on_time: Zeit wie lange eingeschaltet bleibt in sekunden
        """
        if debug:
            print("gui init")
        start_value = 20 
        super().__init__()
        self.start = start_value
        self.relays = relays
        self.on_time = on_time * 1000
        self.rate = int(1 / 60 * 1000)
        self.title(f"{start_value} Countdown")
        self.value = StringVar()
        self.reset()
        self.states = iter(cycle((("Stop", True), ("Start", False))))
        self.statess = iter(cycle((("Upload on", True), ("Upload off", False))))
        self.active = False
        self.actives = False
        self.current_value = start_value
        self.wechsel_knopf_start_stop = None
        self.wechsel_knopf_upload = None
        self.setup()
        self.foto()
        self.last = time.monotonic()
        self.after(self.rate, self.countdown)
        


    def setup(self):
        if debug:
            print(" GUI setup")
        font = TkFont(family="Lucida Grande", size=20)
        Label(self, text="Regatter System C Peterseim", font=font).pack(fill=X)
        Label(self, textvariable=self.value, font=font).pack(fill=X)
        self.wechsel_knopf_start_stop = Button(self, text="Start", font=font, command=self.wechsel_start_stop)
        self.wechsel_knopf_start_stop.pack(fill=X)
        Button(self, text="Zurücksetzen", command=self.reset, font=font).pack(fill=X)
        Button(self, text="Horn", command=self.Horn, font=font).pack(fill=X)
        Button(self, text="FOTO", command=self.foto, font=font).pack(fill=X)
        self.wechsel_knopf_upload = Button(self, text="Upload off", font=font, command=self.wechsel_upload_download)
        self.wechsel_knopf_upload.pack(fill=X)
        Button(self, text="Verlassen", command=self.destroy, font=font).pack(fill=X)
             
    def wechsel_start_stop(self):
        if debug:
            print("toggle")
        state, self.active = next(self.states)
        self.wechsel_knopf_start_stop.config(text=state)

    def wechsel_upload_download(self):
        global stupload
        if self.actives == False:
            self.actives = True
            stupload="Upload an"
            if debug:
                print("debug if schleife upload an=" + str(stupload))
        else:
            self.actives = False
            stupload="Upload off"
            if debug:
                print("debug if schleife upload off" + str(stupload))
            
        states, self.actives = next(self.statess)
        self.wechsel_knopf_upload.config(text=states)

    def reset(self):
        if debug:
            print("reset")
        self.current_value = self.start
        self.value.set(f"{self.start:.2f}")
        globals()["alarm1"] = 0;
        globals()["alarm2"] = 0;
        globals()["alarm3"] = 0;

    
    def countdown(self):
        current_time = time.monotonic()
        delta_time = current_time - self.last
        self.last = current_time
        global nownow
        nownow=self.current_value
        if self.current_value > 9 and self.current_value < 10 and alarm1 == 0:
            globals()["alarm1"]  = 1
            if debug:
                print('ALARM1')
            self.relays.on()
            self.after(1000, lambda: self.relays.off())
        if self.current_value > 4 and self.current_value < 5 and alarm2 == 0:
            globals()["alarm2"]  = 1
            if debug:
                print('ALARM2')
            self.relays.on()
            self.after(1000, lambda: self.relays.off())
        if self.current_value < 0 and alarm3 == 0:
            globals()["alarm3"]  = 1
            if debug:
                print('ALARM3')
            self.relays.on()
            self.after(1000, lambda: self.relays.off())
        if self.active:
            self.current_value -= delta_time
            self.set_value()
        if (GPIO.input(Kamaragpio)==1):
            self.foto()
        self.after(self.rate, self.countdown)

    def set_value(self):
        self.value.set(f"{self.current_value:.2f}")

    def Horn(self):
        if debug:
            print('Probe Horn')
        self.relays.on()
        self.after(1000, lambda: self.relays.off())

    def foto(self):
        if debug:
            print('Foto')
            print('count')
        global count
        count = count + 1
        if(count>2):
            if debug:
                print("Lichtschranke ausgelöst")
            now = datetime.datetime.now()
            timeString = now.strftime("%Y-%m-%d_%H:%M:%S")
            global nownow
            x="{0:.2f}".format(nownow)
            print("request received: " + timeString + "Timer-" +  str(x))
            global filename
            filename = "Bild-" + timeString + "-"+ str(x) + ".jpg"
            self.relays.takepic()
            self.relays.movepic()
            self.relays.upload()




relays = Relays(22)
gui = GUI(10, relays)
gui.mainloop()

Ich denke nicht schön aber selten ;)
Cyress
User
Beiträge: 4
Registriert: Montag 8. März 2021, 01:04

Code: Alles auswählen

import math
import time
from itertools import cycle
from tkinter import Label, Button, Tk, StringVar, X
from tkinter.font import Font as TkFont
import pygame
from pygame.locals import *
from time import sleep
import os
import RPi.GPIO as GPIO
import subprocess
import datetime
import subprocess
globals()["alarm1"] = 0;
globals()["alarm2"] = 0;
globals()["alarm3"] = 0;
#setup pins
Kamaragpio = 17
horngpio = 21
GPIO.setmode(GPIO.BCM)
GPIO.setup(Kamaragpio,GPIO.IN)
GPIO.setup(horngpio,GPIO.OUT)
#setup variables
host = "ftp.strato.de"
username = "xxxx"
password = "xxxx"
command = ""
globals()["debug"] =1;
current_dir = os.getcwd()
globals()["stupload"] = "Upload off";
filename = ""
Scount = 1 
image_dir = "Pictures"
globals()["now"] ="";
camera_pause  = "1000"
count= 1
print("Regata Zeitaufname")
print("Mark Peterseim")
class Relays:
   
    def __init__(self, pin):
        if debug:
            print("-init-")
        self.pin = pin
        

    def on(self):
        if debug:
            print("-init- on")
            print("ON")
        GPIO.output(horngpio,1)

    def off(self):
        if debug:
            print("- init-off")
            print("OFF")
        GPIO.output(horngpio,0)

    def takepic(self):
        if debug:
            print("taking photo...")
        command = "sudo raspistill -o " + filename + " -q 100 -rot 270 -t " + camera_pause
        if debug:
            print(command)
        os.system(command)
        
    def movepic(self):
        path = Path('/') / current_dir / image_dir / filename
        command = "mv " + path
        if debug:
            print("Move pic" + command)
        os.system(command)

    def upload(self):
        if stupload=="Upload an":
            command = "ftp-upload -h " + host + " -u " + username + " --password " + password + " -d  / " + path
            if debug:
                print ("ftp upload gestartet")
                print ("FTP Upload Befehl:" + command)
            subprocess.Popen(command,shell=True)
                    

class GUI(Tk):
    def __init__(self, start_value, relays, on_time=1):
        """
        :param start_value: für den Countdown
        :param relays: Zum Ein-/Ausschalten
        :param on_time: Zeit wie lange eingeschaltet bleibt in sekunden
        """
        if debug:
            print("gui init")
        start_value = 20 
        super().__init__()
        self.start = start_value
        self.relays = relays
        self.on_time = on_time * 1000
        self.rate = int(1 / 60 * 1000)
        self.title(f"{start_value} Countdown")
        self.value = StringVar()
        self.reset()
        self.states = iter(cycle((("Stop", True), ("Start", False))))
        self.statess = iter(cycle((("Upload on", True), ("Upload off", False))))
        self.active = False
        self.actives = False
        self.current_value = start_value
        self.wechsel_knopf_start_stop = None
        self.wechsel_knopf_upload = None
        self.setup()
        self.foto()
        self.last = time.monotonic()
        self.after(self.rate, self.countdown)
        


    def setup(self):
        if debug:
            print(" GUI setup")
        font = TkFont(family="Lucida Grande", size=20)
        Label(self, text="Regatter System C Peterseim", font=font).pack(fill=X)
        Label(self, textvariable=self.value, font=font).pack(fill=X)
        self.wechsel_knopf_start_stop = Button(self, text="Start", font=font, command=self.wechsel_start_stop)
        self.wechsel_knopf_start_stop.pack(fill=X)
        Button(self, text="Zurücksetzen", command=self.reset, font=font).pack(fill=X)
        Button(self, text="Horn", command=self.Horn, font=font).pack(fill=X)
        Button(self, text="FOTO", command=self.foto, font=font).pack(fill=X)
        self.wechsel_knopf_upload = Button(self, text="Upload off", font=font, command=self.wechsel_upload_download)
        self.wechsel_knopf_upload.pack(fill=X)
        Button(self, text="Verlassen", command=self.destroy, font=font).pack(fill=X)
             
    def wechsel_start_stop(self):
        if debug:
            print("toggle")
        state, self.active = next(self.states)
        self.wechsel_knopf_start_stop.config(text=state)

    def wechsel_upload_download(self):
        global stupload
        if self.actives == False:
            self.actives = True
            stupload="Upload an"
            if debug:
                print("debug if schleife upload an=" + str(stupload))
        else:
            self.actives = False
            stupload="Upload off"
            if debug:
                print("debug if schleife upload off" + str(stupload))
            
        states, self.actives = next(self.statess)
        self.wechsel_knopf_upload.config(text=states)

    def reset(self):
        if debug:
            print("reset")
        self.current_value = self.start
        self.value.set(f"{self.start:.2f}")
        globals()["alarm1"] = 0;
        globals()["alarm2"] = 0;
        globals()["alarm3"] = 0;

    
    def countdown(self):
        current_time = time.monotonic()
        delta_time = current_time - self.last
        self.last = current_time
        global nownow
        nownow=self.current_value
        if self.current_value > 9 and self.current_value < 10 and alarm1 == 0:
            globals()["alarm1"]  = 1
            if debug:
                print('ALARM1')
            self.relays.on()
            self.after(1000, lambda: self.relays.off())
        if self.current_value > 4 and self.current_value < 5 and alarm2 == 0:
            globals()["alarm2"]  = 1
            if debug:
                print('ALARM2')
            self.relays.on()
            self.after(1000, lambda: self.relays.off())
        if self.current_value < 0 and alarm3 == 0:
            globals()["alarm3"]  = 1
            if debug:
                print('ALARM3')
            self.relays.on()
            self.after(1000, lambda: self.relays.off())
        if self.active:
            self.current_value -= delta_time
            self.set_value()
        if (GPIO.input(Kamaragpio)==1):
            self.foto()
        self.after(self.rate, self.countdown)

    def set_value(self):
        self.value.set(f"{self.current_value:.2f}")

    def Horn(self):
        if debug:
            print('Probe Horn')
        self.relays.on()
        self.after(1000, lambda: self.relays.off())

    def foto(self):
        if debug:
            print('Foto')
            print('count')
        global count
        count = count + 1
        if(count>2):
            if debug:
                print("Lichtschranke ausgelöst")
            now = datetime.datetime.now()
            timeString = now.strftime("%Y-%m-%d_%H:%M:%S")
            global nownow
            x="{0:.2f}".format(nownow)
            print("request received: " + timeString + "Timer-" +  str(x))
            global filename
            filename = "Bild-" + timeString + "-"+ str(x) + ".jpg"
            self.relays.takepic()
            self.relays.movepic()
            self.relays.upload()




relays = Relays(22)
gui = GUI(10, relays)
gui.mainloop()

Ich denke nicht schön aber selten ;)
bin mit dem überarbeiten aber noch nicht fertig!

lg
Sven
Sirius3
User
Beiträge: 17711
Registriert: Sonntag 21. Oktober 2012, 17:20

Benutze keine globalen Variablen. Du hast doch schon Klassen. Vor allem greift man auf globale Namen nicht über globals() zu.
Variablennamen schreibt man komplett klein, Konstanten dagegen komplett GROSS, also KAMARA_PIN.
Die Strichpunkte sind allesamt überflüssig.
Für Debug-Aufgaben benutzt man logging.
os.system benutzt man nicht, vor allem nicht für Triviale Aufgaben wie `mv`. Und wie man den FTP-Upload aufruft, hatten wir ja schon.
Benutzeravatar
__blackjack__
User
Beiträge: 13004
Registriert: Samstag 2. Juni 2018, 10:21
Wohnort: 127.0.0.1
Kontaktdaten:

@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.
“Most people find the concept of programming obvious, but the doing impossible.” — Alan J. Perlis
Antworten