Nur ein Bit ändern

[kiaralle]

Ok, hatte ich übersehen.
Danke euch allen.
Das wird.

[kiaralle]

Hallo,

hier meine erster Versuch der Umstellung.
Die Ergebnisse sind alle ok und entsprechen den hinterlegten Daten im Geber.
Der Aufbau des data-Strings werdet ihr sicher bemängeln. Macht ihr sicher eleganter.

Das mit dem number_of_datafields=int.from_bytes(response[3:4],'big',signed=False) ist ok?
Hatte ich aus dem Netz übernommen.
Ich stelle mittlerweile alles in Frage

Code: Alles auswählen

   def listing(self):
        
        BIT_AKTIV = 1 << 7
        BIT_WRITABLE = 1 << 6
        BIT_PASSWORT = 1 << 4
        BIT_PASSAktiv = 1 << 3
        BIT_SPEICHER = 1 << 0


        data = bytearray.fromhex("FF 4E")
        data.append(ChecksumXor8.calc(data))
        
        try:
            serial_interface.reset_input_buffer()
            serial_interface.reset_output_buffer()#flush output buffer, aborting current output 
            time.sleep(0.01)  #give the serial port sometime to receive the data
            serial_interface.write(data)
            time.sleep(0.01)
            response = serial_interface.read(7)
            number_of_datafields=int.from_bytes(response[3:4],'big',signed=False)

        except Exception as e1:
            print ("error communicating...: " + str(e1))

        self.datenfeld_info.config(state='normal')
        self.datenfeld_info.delete('2.0','end')


        i=0
        for i in range(number_of_datafields):
            data = bytearray([0xff, 0x4c, i])
            data.append(ChecksumXor8.calc(data))
            try:
                serial_interface.reset_input_buffer()
                serial_interface.reset_output_buffer()
                time.sleep(0.010) 
                serial_interface.write(data)
                time.sleep(0.01)
                response = serial_interface.read(7)
            
                if response[0] == 0x50 or ChecksumXor8.calc(response) != 0:
                    print(sick_error.srm_errors(hex(response[1])))
                else:
                    datenfeld = response[3]
                    nummer = '\n' + str(i) + '\t'
                    enabled = "ja\t" if datenfeld & BIT_AKTIV else "nein\t"

                    writeable = "ja\t" if datenfeld & BIT_WRITABLE else "nein\t"

                    password = str(datenfeld & BIT_PASSWORT)+'\t'
        
                    password_active = "ja\t" if datenfeld & BIT_PASSAktiv else "nein\t"
        
                    speichergroese = str((1 + (datenfeld & BIT_SPEICHER)) * 16) + '\t'
                    
                    data = nummer + enabled + writeable +  password + password_active + speichergroese
                    self.datenfeld_info.insert('end',data)
                    
                    
            
            except Exception as e1:
                print ("error communicating...: " + str(e1))

[__blackjack__]

@kiaralle: Konstanten definiert man üblicherweise nicht _in_ Funktionen, sondern auf Modulebene.

`BIT_PASSAktiv` ist von der Gross-/Kleinschreibung her inkonsistent.

`BIT_SPEICHER` ist so nur korrekt, falls das wirklich nur ein Bit ist, also es nur die Speichergrössen 16 und 32 geben kann. Dann könnte man sich aber auch die Rechnung sparen und das so schreiben wie man beispielsweise bei den Flags "ja" und "nein" an Namen gebunden hat. Sollte die Speichergrösse durch die drei untersten Bits entschieden werden, dann ist die Maske dafür, wie schon mal gesagt, 0b00000111, 0x07, 0o007, 7, ``(1 << 3) - 1``, oder ``2**3 - 1``, oder wie auch immer man den Wert schreiben möchte. Wenn es um Lesbarkeit geht, würde ich persönlich die Binärschreibweise verwenden, weil man da direkt, ohne überlegen oder rechnen sieht, wie die Bitmaske aussieht. Aus Gewohnheit wäre hexadezimal oder dezimal meine zweite Wahl, weil man das in vielen Sprachen, welche keine Binärschreibweise erlaubt, so machen muss, und man bei einem Wert wie 7 auch nicht lange überlegen muss wie das Bitmuster dazu aussieht, wenn man so etwas eine Weile gemacht hat. Das gleiche gilt auch für oktal, nur dass ich da erst seit kurzem Erfahrungen mit sammle. Ist etwas einfacher als bei hexadezimal, weil man nur eine Untermenge an Bitmustern auswendig lernen muss oder als Tabelle zu Hand haben muss. 0 bis 7 statt 0 bis 15.

Die Ausnahmebehandlung ist falsch. Du hast da beim Abfragen der Anzahl der Datenfelder ein ``try``/``except`` aber danach wird einfach weitergemacht als wäre alles okay, auch wenn die Abfrage gar nicht funktioniert hat und in den ``except``-Zweig gelaufen ist. Was unweigerlich in einen vermeidbaren Folgefehler laufen wird, weil dann `number_of_fields` überhaupt gar nicht definiert ist. Da müsste man dann entweder die Methode vorzeitig beenden, oder den Teil der nur laufen soll falls die Abfrage geklappt hat, in ein ``else`` zu dem ``try``/``except`` packen.

`serial_interface` kommt magisch aus dem nichts, ist also eine globale Variable/globaler Zustand. Alles was eine Funktion oder Methode ausser Konstanten benötigt, wird als Argument(e) übergeben. Also müsste das entweder an das Objekt gebunden sein, wo die Methode drauf definiert ist, oder als zusätzliches Argument beim Aufruf übergeben werden.

Es gibt wiederholten Code für das Senden eines Kommandos und Empfangen der Antwort, der dort nahezu identisch zwei mal steht. Das sollte man in eine Funktion oder Methode heraus ziehen. Kann bei der Abfrage der Anzahl der Felder die Prüfsumme nicht auch falsch sein? Oder ein Fehlercode wie 0x50 kommen?

Es sieht nach einer starken Vermischung von GUI und Programmlogik aus. Das sollte man auch besser trennen. Und vielleicht auch die _Details_ der Kommunikation besser ”verstecken”. Also nicht eine Funktion oder Methode in der man wissen muss welche Bytewerte die Abfrage der Feldanzahl kodieren, sondern eine Funktion oder Methode `get_data_field_count()` die dass dann weiss. Das macht den Code verständlicher.

[__blackjack__]

Bei der Fehlermeldung im zweiten Fall denke ich das ist falsch bei falscher Prüfsumme das gleiche zu machen wie bei einem 0x50 als erstem Bytewert. Bei einer falschen Prüfsumme weiss man ja letztlich gar nicht was das Problem ist, also welches Byte oder welche Bytes falsch sind. Und das muss man auch zuerst prüfen, denn bei einer falschen Prüfsumme kann man sich auch gar nicht sicher sein, dass 0x50 als erstes Byte auch tatsächlich der Wert ist, der gesendet wurde.

Wenn es im `tkinter`-Modul eine Konstante für Argumente gibt, die keine beliebigen Zeichenketten sind, sollte die auch verwenden. Also beispielsweise `tkinter.NORMAL` statt einer Zeichenkette mit dem Wort.

Die Zuweisung ``i = 0`` wird nirgends verwendet, kann also weg fallen.

Zeilenendezeichen gehören ans Ende. Das heisst man sollte Zeilen damit abschliessen, und nicht neue damit Anfangen und dann nicht beenden.

Beliebige Ausnahmen durch eine nichtssagende feste Zeile zu ersetzen hilft bei der Fehlersuche nicht nur nicht, sondern macht die auch noch schwerer. Statt `print()` würde man da Logging verwenden, was dann in diesem Fall auch gleich den kompletten Traceback mitprotokolliert.

Das ginge dann so ungefähr in diese Richtung:

Code: Alles auswählen

BIT_AKTIV = 1 << 7
BIT_WRITABLE = 1 << 6
BIT_PASSWORT = 1 << 4
BIT_PASSWORD_ACTIVE = 1 << 3
MEMORY_SIZE_MASK = 0b0000_0111


def communicate(serial_interface, request):
    request_data = bytearray(request)
    request_data.append(ChecksumXor8.calc(request_data))
    serial_interface.reset_input_buffer()
    serial_interface.reset_output_buffer()
    time.sleep(0.01)
    serial_interface.write(request_data)
    time.sleep(0.01)
    response = serial_interface.read(7)
    if ChecksumXor8.calc(response) != 0:
        raise ChecksumXor8.ChecksumError()

    if response[0] == 0x50:
        raise sick_error.SRMError(response[1])

    return response

...

    def listing(self, serial_interface):
        datafield_count = int.from_bytes(
            communicate(serial_interface, [0xFF, 0x4E])[3:4], "big"
        )
        self.datenfeld_info.config(state=tk.NORMAL)
        self.datenfeld_info.delete("2.0", tk.END)
        for index in range(datafield_count):
            try:
                value = communicate(serial_interface, [0xFF, 0x4C, index])[3]
            except:
                logging.exception("error communicating...")
            else:
                row = [
                    str(index),
                    "ja" if value & BIT_AKTIV else "nein",
                    "ja" if value & BIT_WRITABLE else "nein",
                    str(value & BIT_PASSWORT),
                    ("ja" if value & BIT_PASSWORD_ACTIVE else "nein"),
                    str((1 + (value & MEMORY_SIZE_MASK)) * 16),
                ]
                self.datenfeld_info.insert(tk.END, "\t".join(row) + "\n")