Class mit serial Modul erstellt für BMW IBus(Datenbus)

Wenn du dir nicht sicher bist, in welchem der anderen Foren du die Frage stellen sollst, dann bist du hier im Forum für allgemeine Fragen sicher richtig.
harryberlin
User
Beiträge: 227
Registriert: Donnerstag 17. Dezember 2015, 12:17

Hallo Zusammen

Einige erinnern sich vllt. noch, ich hatte um die Weihnachtszeit mal ein völlig schlechten Python Code von meinem kodi addon gezeigt.
Nun bin ich mal wieder dabei, den code umzuschreiben.

Aktuell geht es um eine Klasse die Serial Daten lies und entsprechend Telegramme ausgibt.
Funktion is folgt:
Es wird die Serial-Device geöffnet und zeitgleich in einem Thread geprüft, ob der Datenbus frei ist.
Dann kann man den Thread zum lesen starten.
Man kann nun ununterbrochen sich die telegramme mit der get funktion holen und entsprechend weiterverarbeiten.
Senden kann man mit der write funktion, die von unterschiedlichsten Threads aufgerufen wird.
Entsprechend muss ich zwischen high und low prio nachrichten unterscheiden, und verhindern dass keine Kollisionen entstehen.

Könnt ihr mal drüber schauen, was ich verbessern kann? Sofern es möglich ist bitte nicht gleich den ganzen Code umschreiben. Oder zumindest eine Erklärung dazu, damit ich es als Anfänger verstehe.

Code: Alles auswählen

#!/usr/bin/env python
# -*- coding: utf-8 -*-

import serial, time
from threading import Thread
from copy import deepcopy


def log(message, log_level=1):
    print '%s' % message


def set_kodi_prop(key, value):
    log('%s - %s' % (key, value))


class ibusFace(object):
    def __init__(self, devPath):
        try:
            self.SDEV = serial.Serial(devPath,
                                      baudrate=9600,
                                      bytesize=serial.EIGHTBITS,
                                      parity=serial.PARITY_EVEN,
                                      stopbits=serial.STOPBITS_ONE)
            self.init_success = True
        except:
            self.init_success = False
            return

        self.read_buffer = []

        self.cancel_all = False  # set True to cancel to close the serial device

        self.read_access = False  # set False to trim packet_buffer

        self.dont_send = False  # is True to access high prio answer messages
        self.writing = False  # is True while writing
        self.wait_highprio_writing = False  # let wait a low prio message

        self.ctsCnt = 0  # free bus counter
        self.msgerrcnt = 0  # counter for read errors
        set_kodi_prop('msgerr_cnt', '0')

        time.sleep(0.2)
        self.SDEV.setRTS(False)

        self.SDEV.flushInput()

        # log('SERIAL INPUT BUFFER: ' + str(self.SDEV.inWaiting())) # just for information
        log('IBUS: Initialized')

        # Reading Thread
        self.cancel_read_thread = False
        self.read_thread = Thread(target=self._reading)
        self.read_thread.setDaemon(True)

        # Free Bus Thread
        self.cancel_cts_thread = False
        self.cts_thread = Thread(target=self._check_free_bus)
        self.cts_thread.setDaemon(True)
        self.cts_thread.start()

    def _reading(self):
        self.read_access = True
        while not self.cancel_read_thread:
            self._read_char()
            time.sleep(0.001)

    def _check_free_bus(self):
        self.ctsCnt = 0
        while not self.cancel_cts_thread:
            try:
                if self.SDEV.getCTS():
                    self.ctsCnt += 1
                else:
                    self.ctsCnt = 0
                time.sleep(0.001)
            except:
                time.sleep(0.001)

    def wait_free_bus(self, waiting=17, timeout=1000.0):
        while self.ctsCnt < waiting:
            if self.cancel_cts_thread:
                return False
            if not timeout > 0.0:
                return False
            timeout -= 0.5
            time.sleep(0.0005)
        return True

    def start_reading(self):
        self.read_thread.start()

    def flush_input(self):
        self.SDEV.flushInput()

    def set_rts(self, rts_state):
        self.SDEV.setRTS(rts_state)
        log('IBUS: SET RTS: ' + str(rts_state))

    def _read_char(self):
        try:
            while self.SDEV.inWaiting() > 0:
                if self.cancel_read_thread:
                    return
                if self.read_access:
                    self.read_buffer.append('%02X' % ord(self.SDEV.read()))
        except:
            log('IBUS: Hit a serialException', 3)
            return

    def _cut_packet(self, range=1):
        self.read_access = False
        time.sleep(0.003)
        self.read_buffer = self.read_buffer[range:]
        self.read_access = True

    def get_bus_packet(self):
        packet = {'src': None,
                  'len': None,
                  'dst': None,
                  'dat': [],
                  'xor': None}
        try:
            dataLen = int(self.read_buffer[1], 16)
        except:
            return

        if dataLen < 3 or dataLen > 37:
            log('IBUS: Length of +37 found (%s), %s' % (dataLen, self.read_buffer), 2)
            self._cut_packet()
            self.msgerrcnt += 1
            set_kodi_prop('msgerr_cnt', '%s' % self.msgerrcnt)
            return
        else:
            buffer_len = len(self.read_buffer)
            if buffer_len < 5 or buffer_len < dataLen + 2:
                return
            message = self.read_buffer[:dataLen + 2]
            if self._is_packet_ok(message):
                packet['src'] = message[0]
                packet['len'] = message[1]
                packet['dst'] = message[2]
                packet['dat'] = message[3:dataLen + 1]
                packet['xor'] = message[-1]
                self._cut_packet(dataLen + 2)
                log('IBUS-READ: %s' % ' '.join(message), 5)
                return packet
            log('IBUS: Corrupt packet found (%s), %s' % (dataLen, self.read_buffer), 2)
            self._cut_packet()
            return

    def write_bus_packet(self, src, dst, data, highprio=False):
        while self.dont_send:
            time.sleep(0.001)

        self.dont_send = highprio
        if not highprio:
            while self.wait_highprio_writing:
                time.sleep(0.001)

            self.wait_highprio_writing = True
        length = '%02X' % (2 + len(data))
        packet = [src, length, dst]
        full_packet_as_string = ' '.join(deepcopy([src, length, dst, ' '.join(data)]))

        for p in data:
            packet.append(p)

        for i in range(len(packet)):
            packet[i] = int('0x%s' % packet[i], 16)

        chk = self._get_checksum(packet)
        lastInd = len(packet) - 1
        packet[lastInd] = chk

        if highprio:
            while True:
                if self._write_high_packet(packet):
                    break
                # log('IBUS-SENT: TRY AGAIN', 2)
                time.sleep(0.001)

            log('IBUS-SENT: %s CK SUCCESS' % full_packet_as_string, 2)
        else:
            while True:
                if self._write_low_packet(packet):
                    break
                # log('IBUS-SENT: TRY AGAIN', 2)
                time.sleep(0.001)

            log('IBUS-SENT: %s CK SUCCESS' % full_packet_as_string, 2)

        if highprio:
            self.dont_send = False
        self.wait_highprio_writing = False

    def _write_low_packet(self, packet):
        if self.writing:
            return False
        data = ''.join((chr(p) for p in packet))
        self.writing = True
        if not self.cancel_all:
            if self.wait_free_bus():
                if not self.dont_send:
                    self.ctsCnt = 0
                    self.SDEV.write(data)
                    self.ctsCnt = 0
                    self.SDEV.flush()
                    self.ctsCnt = 0
                    self.writing = False
                    return True
                else:
                    self.writing = False
                    return False
            else:
                self.writing = False
                return False
        
        

    def _write_high_packet(self, packet):
        if self.writing:
            return False
        data = ''.join((chr(p) for p in packet))
        self.writing = True
        if not self.cancel_all:
            if self.wait_free_bus():
                self.ctsCnt = 0
                self.SDEV.write(data)
                self.ctsCnt = 0
                self.SDEV.flush()
                self.ctsCnt = 0
                self.writing = False
                return True
            else:
                self.writing = False
                return False
        

    def _get_checksum(self, packet):
        chk = 0
        packet.append(0)
        for p in packet:
            chk ^= p

        return chk

    def _is_packet_ok(self, message):
        message_tmp = deepcopy(message)
        for i in range(len(message_tmp)):
            message_tmp[i] = int('0x%s' % message_tmp[i], 16)

        chk = self._get_checksum(message_tmp[:-1])
        if chk == message_tmp[-1]:
            return True
        else:
            return False

    def write_hex_string(self, hexstring):
        hexstring_tmp = hexstring.split(' ')
        src = hexstring_tmp[0]
        dst = hexstring_tmp[2]
        datend = len(hexstring_tmp) - 1
        data = hexstring_tmp[3:datend]
        self.write_bus_packet(src, dst, data)

    def close(self):
        self.cancel_read_thread = True
        self.cancel_cts_thread = True
        self.cancel_all = True
        time.sleep(600)
        self.SDEV.close()
        log('IBUS: Closed')
empty Sig
BlackJack

Für logging mit Logleveln gibt es bereits das `logging`-Modul in der Standardbibliothek.

``print '%s' % message`` ist sinnfrei. ``print`` wandelt die Werte die ausgegeben werden sollen bereits in Zeichenketten um.

Klassennamen fangen mit einem Grossbuchstaben an.

Namen sollte man nicht abkürzen. `devPath` kann man problemlos als `development_path` ausschreiben. Huch, das bedeutet `dev` hier ja gar nicht. :-)

Die Ausnahmebehandlung ”behandelt” durch die Bank weg *alle* Ausnahmen, was in der Regel nicht sinnvoll ist. Wenn Du nicht weisst welche Ausnahmen Du bekommen kannst, kannst Du sie normalerweise nicht sinnvoll behandeln. Dann behandle sie einfach nicht. Das ist sinnvoller als beispielsweise eine Ausnahme in ein Attribut mit einem Wahrheitswert zu wandeln. Das ist noch schlechter als sie in einen solchen Rückgabewert zu wandeln, denn genau um diese Art von fehleranfälligen Rückgabewerten loszuwerden wurden Ausnahmen überhaupt erst erfunden. So dass Ausnahmesituationen eben nicht aus versehen oder absichtlich ignoriert werden können, ohne dass man das explizit in Code ausdrückt.

Namen komplett in Grossbuchstaben sind für Konstanten. Ein `Serial`-Objekt ist keine Konstante.

`read_access` und den Kommentar verstehe ich so auf Anhieb nicht. Dort und auch bei anderen Flags habe ich das Gefühl das dass unsicher ist was Threads angeht, weil das teilweise verdammt nach Attributen mit einfachen Wahrheitswerten aussieht um kritische Abschnitte zu schützen. Das geht so nicht. Ausserdem wird mit den Flags an einigen Stelle „busy waiting“ betrieben. Gar nicht gut.

`range` ist der Name einer eingebauten Funktion, diese Namen sollte man nicht für andere Dinge nehmen.

Wenn ein Code-Abschnitt in das setzen eines Flags und das rücksetzen am Ende ”eingerahmt” ist, würde ich ``finally`` verwenden um am Ende den Zustand auch unter allen Umständen zurück zu setzen. Hier greift wieder mein Verdacht das einige Flags wohl eher Sperrobjekte sein sollten. Die kann man mit der ``with``-Anweisung verwenden.

`get_bus_packet()` gibt entweder das `packet` oder `None` zurück. Statt `None` wäre eine Ausnahme besser. Aber zumindest sollte man *explizit* `None` zurückgeben und nicht einfach nur ``return`` schreiben und am Ende sogar auf das implizite ``return None`` am Ende von Funktionen und Methoden setzen. Ausserdem kann man sich zwei ``return``-Anweisungen sparen, weil die am Ende von ``if``/``else``-Zweigen stehen, wo nach dem Konstrukt sowieso schon ``return None`` steht, beziehungsweise stehen sollte.

`packet` mit Dummywerten zu installieren ist unnötige Arbeit wenn man die sowieso alle ersetzt. Und dann sollte man das auch nicht auf Vorrat erstellen, sondern dann wenn es tatsächlich gebraucht wird.

Beide Verwendungen von `deepcopy()` sind unsinnig. Im ersten Fall komplett, im zweiten ist eine Kopie nur deswegen nötig weil Du die Datenstruktur „in place“ veränderst statt eine neue zu erstellen. Was Du durch die Kopie letztendlich ja sowieso machst.

Listen haben eine `extend()`-Methode wenn man alle Elemente eines anderen iterierbaren Objektes anhängen möchte.

Die Umwandelei zwischen Listen mit Zahlen, Zeichenketten mit je zwei Hexadezimalziffern, und Zeichenketten mit Binärdaten erscheint mir sehr suspekt. Wozu soll das gut sein? Das macht alles nur komplexer und verwirrender als es sein müsste. Lass die Hexdarstellung da raus.

Wenn man eine Zeichenkette zur Basis 16 in eine Zahl wandeln will, muss man da kein '0x' davor pappen. Das geht auch ohne.

Das hier:

Code: Alles auswählen

        chk = self._get_checksum(packet)
        lastInd = len(packet) - 1
        packet[lastInd] = chk
Schreibt man in einer Zeile so:

Code: Alles auswählen

        packet[-1] = self._get_checksum(packet)
Gegen Ende der Methode ist dann fast der gleiche Quelltext der sich nur durch einen Methodennamen unterscheidet. Daten- und Codewiederholungen vermeidet man so gut es geht, weil das Fehlerquellen sind, wenn man das Programm warten oder erweitern muss. Und die beiden Methoden selbst sind identisch! (Oder es ist bereits zu spät für mich Code anzuschauen :-))

Autsch: `get_checksum()` verändet das Argument. Damit rechnet niemand, so etwas ist eine ganz schlechte Idee! Ausserdem ist die Methode keine Methode sondern eine Funktion. Und diese Art der Prüfsumme hat die Eigenschaft, dass man sie prüfen kann in dem man die gesamte Nachricht verwendet und dann schaut ob 0 dabei heraus kommt. Und da eine 0 keine Auswirkung auf das Ergebnis hat, braucht man an die Daten auch keine 0 anhängen.

Zwischenergebnis (ungetestet):

Code: Alles auswählen

#!/usr/bin/env python
# -*- coding: utf-8 -*-
import logging
import time
from itertools import chain
from threading import Thread

import serial
 
logging.basicConfig(log_level=logging.INFO)
LOGGER = logging.getLogger(__name__)


def set_kodi_prop(key, value):
    LOGGER.info('%s - %s', key, value)
 
 
def calculate_checksum(packet):
    result = 0
    for value in packet:
        result ^= value
    return result


class IbusFace(object):

    def __init__(self, device):
        self.serial = serial.Serial(device, parity=serial.PARITY_EVEN)
 
        self.read_buffer = list()
 
        self.cancel_all = False  # set True to cancel to close the serial device
 
        self.read_access = False  # set False to trim packet_buffer
 
        self.dont_send = False  # is True to access high prio answer messages
        self.writing = False  # is True while writing
        self.wait_highprio_writing = False  # let wait a low prio message
 
        self.free_bus_counter = 0
        self.read_error_count = 0
        set_kodi_prop('msgerr_cnt', '0')
 
        time.sleep(0.2)
        self.serial.setRTS(False)
        self.flush_input()
 
        LOGGER.info('IBUS: Initialized')
 
        self.cancel_read_thread = False
        self.read_thread = Thread(target=self._reading)
        self.read_thread.daemon = True
 
        # Free Bus Thread
        self.cancel_cts_thread = False
        self.cts_thread = Thread(target=self._check_free_bus)
        self.cts_thread.daemon = True
        self.cts_thread.start()
 
    def _reading(self):
        self.read_access = True
        while not self.cancel_read_thread:
            self._read_char()
            time.sleep(0.001)
 
    def _check_free_bus(self):
        self.free_bus_counter = 0
        while not self.cancel_cts_thread:
            try:
                if self.serial.getCTS():
                    self.free_bus_counter += 1
                else:
                    self.free_bus_counter = 0
            except:
                pass  # TODO Why is it okay to ignore *every* exception here?
            finally:
                time.sleep(0.001)

    def wait_free_bus(self, waiting=17, timeout=1000.0):
        while self.free_bus_counter < waiting:
            if self.cancel_cts_thread or timeout <= 0:
                return False
            timeout -= 0.5
            time.sleep(0.0005)
        return True
 
    def start_reading(self):
        self.read_thread.start()
 
    def flush_input(self):
        self.serial.flushInput()
 
    def set_rts(self, rts_state):
        self.serial.setRTS(rts_state)
        LOGGER.info('IBUS: SET RTS: %s', rts_state)
 
    def _read_char(self):
        try:
            while self.serial.inWaiting() > 0:
                if self.cancel_read_thread:
                    return
                if self.read_access:
                    self.read_buffer.append('%02X' % ord(self.serial.read()))
        except:
            LOGGER.exception('IBUS: Hit a serialException')
            # 
            # TODO Why is it okay to handle every exception this way?
            # 
            return
 
    def _cut_read_buffer(self, offset=1):
        self.read_access = False
        try:
            time.sleep(0.003)
            self.read_buffer = self.read_buffer[offset:]
        finally:
            self.read_access = True
     
    def get_bus_packet(self):
        try:
            data_length = int(self.read_buffer[1], 16)
        except (IndexError, ValueError):
            return None
 
        if data_length < 3 or data_length > 37:
            LOGGER.error(
                'IBUS: Length of +37 found (%s), %s',
                data_length,
                self.read_buffer,
            )
            self._cut_read_buffer()
            self.read_error_count += 1
            set_kodi_prop('msgerr_cnt', str(self.read_error_count))
        else:
            buffer_len = len(self.read_buffer)
            if buffer_len < 5 or buffer_len < data_length + 2:
                return None
            message = self.read_buffer[:data_length + 2]
            if calculate_checksum(message) == 0:
                self._cut_read_buffer(data_length + 2)
                LOGGER.debug('IBUS-READ: %s', ' '.join(message))
                # 
                # TODO `collections.namedtupel` instead of `dict` and
                #   better names for attributes/keys.
                # 
                return {
                    'src': message[0],
                    'len': data_length,
                    'dst': message[2],
                    'dat': message[3:data_length],
                    'xor': message[-1],
                }
            else:
                LOGGER.error(
                    'IBUS: Corrupt packet found (%s), %s',
                    data_length,
                    self.read_buffer
                )
            self._cut_read_buffer()

        return None
 
    def write_bus_packet(self, source, destination, data, high_prio=False):
        while self.dont_send:
            time.sleep(0.001)
 
        self.dont_send = high_prio
        if not high_prio:
            while self.wait_highprio_writing:
                time.sleep(0.001)
            self.wait_highprio_writing = True

        length = '%02X' % (2 + len(data))
        packet = [
            int(v, 16) for v in chain([source, length, destination], data)
        ]
        packet.append(calculate_checksum(packet))
        while True:
            if self._write_packet(packet):
                break
            # log('IBUS-SENT: TRY AGAIN', 2)
            time.sleep(0.001)

        LOGGER.debug(
            'IBUS-SENT: %s CK SUCCESS',
            ' '.join([source, length, destination, ' '.join(data)])
        )
 
        if high_prio:
            self.dont_send = False
        self.wait_highprio_writing = False
 
    def _write_packet(self, packet):
        if self.writing:
            return False
        data = ''.join((chr(p) for p in packet))
        self.writing = True
        if not self.cancel_all:
            if self.wait_free_bus():
                if not self.dont_send:
                    self.free_bus_counter = 0
                    self.serial.write(data)
                    self.free_bus_counter = 0
                    self.serial.flush()
                    self.free_bus_counter = 0
                    self.writing = False
                    return True
                else:
                    self.writing = False
                    return False
            else:
                self.writing = False
                return False
 
    def write_hex_string(self, hex_string):
        hex_values = hex_string.split(' ')
        self.write_bus_packet(
            hex_values[0], hex_values[2], hex_values[3:len(hex_values) - 1]
        )
 
    def close(self):
        self.cancel_read_thread = True
        self.cancel_cts_thread = True
        self.cancel_all = True
        time.sleep(600)
        self.serial.close()
        LOGGER.info('IBUS: Closed')
Ich habe den Eindruck die Klasse macht zu viel. Kann es sein, dass man lesen und schreiben in zwei nahezu unabhängige Attributsätze aufteilen kann, die sich nur das `Serial`-Objekt teilen?
harryberlin
User
Beiträge: 227
Registriert: Donnerstag 17. Dezember 2015, 12:17

Guten Morgen

Das ist ja wahnsinn was du in kurzer Zeit da gemacht hast :)

Ich muss mich entschuldigen, ich hätte mehr dazu erklären müssen.

1.
log bitte so lassen, da ich hier nur eine ersatzfunktion reingeballert habe. normal logge ich ins kodi logfile, und das mit level 1 - 5.
mit xbmc importen und meinen loglevels dazu wollte ich hier nicht aufhalten.

2.
device_path ist damit gemeint. ich würde mal behaupten, solche aktuell kleinen fehler kann man später auch ganz gut noch nachziehen. war mir jedenfalls nicht entfallen, dass die formulierung schlecht ist.

3.
bei deiner erklärung zu ausnahmen komme ich nicht mit, da fehlt mir irgendwie der bezug zum code.

4.
serial object ok, das gleiche wie 2.

5.
read_access ist dazu gedacht, wenn ich das packet trimme, dass das einlesen und anhängen neuer zeichen übersprungen wird.
damit keine zeichen verloren gehen.

6.
ok, dass es range woanders aus python gibt, wusste ich nicht. offset ist auch nicht unbedingt die passende bezeichnung. für mich ist offset immer nur ein zeichen. aber es kann ja auch ein gewisser range zum kürzen notwendig sein.

7.
ja ich muss bei high prio nachrichten die anderen sperren, und auch halten. einfach überspringen darf nicht sein. und, wenn eine neue high prio nachricht kommt, dann muss diese vorgezogen werden können, sofern im moment des eintreffens der high prio eine high prio nachricht geschrieben wird.

8.
ja, man könnte auch return None einsetzen.

9.
ok, das mit den packet dummys schau ich mal ob das so geht.

10.
deepcopy war nötig, da ich die checksum mit dem gelesenen zeichen(die gesendete checksum) vergleichen möchte. da fand ich nur die lösung mit deepcopy, da mir sonst die komplette nachricht umgesschrieben wurde.

11.
den einwurf listen extend verstehe ich nicht.

12.
hier komme ich auch nicht ganz mit. ich brauche das packet als hex-string für weitere auswertungen.

13.
calc checksum bitte innerhalb der klasse lassen. so bleibt es sozusagen ein geschnürtes komplettpaket.
zudem scheinst du zu verwechseln, dass ich einmal die checksum berechne(fürs schreiben) und einmal vergleiche(beim lesen).

14.
ich sah keine andere lösung, die low und high prio messages anders abzufahren. muss ich mal testen, ob das so läuft

15.
ok, bei get_checksum() das muss ich mir mal genau anschauen.

16. wieso sollten die funktionen auf einmal attribute sein? bitte das ganze nicht teilen, es soll ein komplettpacket bleiben. oder wie war das gemeint?

18.
die funktion zum hex string senden ist noch under construction, also derweil ignorierren.

17. zur Funktion noch mal etwas detailierter
es sollen zeichen der ibus nachrichten als hexstring in den buffer gelesen werden.
per funktion get_bus_packet() soll ein packet zurück gegeben werden.
wenn keins vorhanden ist, dann soll None erfolgen.

beim schreiben muss man zwischen low und high prio nachrichten unterscheiden können.
es darf zu keiner kollision der beiden hign und low prio nachrichten kommen, zu dem muss die busaktivität beachtet werden. deswegen wait_free_bus()
zudem sollen beim einlesen die korrupten nachrichten ebenso herausgefiltert werden, aber nur schrittweise. also cut 1

der rest der ganze kram, checksum usw. das ganze management soll innerhalb der klasse passieren. deswegen auch der unterstrich vor einigen funktionen.

mit close() sollen dann die ganzen threads beendet und der port geschlossen werden. hier liegt auch teilweise das problem, da kodi immer ein timeout bringt und das addon dann killt.

ich hoffe es ist besser verständlich.
bei fragen, bitte vorher nachhaken, bevor in die falsche richtung interpretiert wird.
Zuletzt geändert von harryberlin am Mittwoch 13. Juli 2016, 09:26, insgesamt 1-mal geändert.
empty Sig
Sirius3
User
Beiträge: 17754
Registriert: Sonntag 21. Oktober 2012, 17:20

Wenn ich so etwas sehe, sträuben sich mir alle Haare:

Code: Alles auswählen

        while True:
            if self._write_packet(packet):
                break
            time.sleep(0.001)
 
    def _write_packet(self, packet):
        if self.writing:
            return False
        self.writing = True
        do_something()
        self.writing = False
        return True
nicht dass es auch noch einen Fall gibt, bei dem writing nicht mehr auf False gesetzt wird und damit die obere Schleife zu einer Endlos-Schleife wird.
So etwas ist quasi das Paradebeispiel eines Locks:

Code: Alles auswählen

        [...]
        self.writing = threading.Lock()
        [...]
        self._write_packet(packet)

    def _write_packet(self, packet):
        if self.cancel_all:
            return False
        with self.writing:
            data = ''.join((chr(p) for p in packet))
            self.wait_free_bus(timeout=float('inf'))
            self.SDEV.write(data)
            self.SDEV.flush()
            self.ctsCnt = 0
            return True
Wobei hier, wenn man eine Priorisierung hat, man eine PriorityQueue benutzen muß, sonst purzeln die Pakete nach dem Zufallsprinzip durch die Leitung. Macht die ganze Sache auch nochmal deutlich einfacher, weil es dann nur einen Thread gibt, der schreiben darf. Wenn Du Dein Programm sauber refaktoriert hast, solltest Du so gut wie keine Flags mehr haben und zwei Threads, einen für's Lesen und einen für's Schreiben, die über eine Queue sauber an die Außenwelt angebunden sind.
harryberlin
User
Beiträge: 227
Registriert: Donnerstag 17. Dezember 2015, 12:17

@sirius
gibt es irgendwo eine seite für idioten, wo lock und queue erklärt wird. ich check das einfach nicht. :cry:
was meinst du mit zufallsprinzip. wenn ich mehrere high und low prio nachrichten anstehen habe, dann kann ich diese nicht nach der reihe abarbeiten.

ein weiterer thread zur überwachung der bus-aktivität muss dennoch vorhanden sein. da ich keine andere lösung sehe als den cts zu überwachen. und so einfach wie unter windows mit mscomm32.ocx scheint es bei python nicht zu gehen. da braucht man wirklich nur cts abfragen und dann einfach schreiben. unter linux python gibt es dann immer kollisionen.
deshalb die 17ms. ist jetzt ein erfahrungswert, mit dem es ganz gut läuft.
empty Sig
Sirius3
User
Beiträge: 17754
Registriert: Sonntag 21. Oktober 2012, 17:20

@harryberlin: Locks und Queues sind Konzepte, die man immer braucht, wenn man Nebenläufigkeit hat. Es kann nicht schaden generell bescheid darüber zu wissen. Soweit ich sehe, brauchst Du diesen cts nur beim Schreiben, also reicht der Check auch innerhalb des Schreib-Threads.
harryberlin
User
Beiträge: 227
Registriert: Donnerstag 17. Dezember 2015, 12:17

hast du quellen für locks und queues? wie gesagt ich tu mir da sehr schwer es zu verstehen.
selbst wenn es grundlegend ist, erklärt es mir die sache nicht leichter.

nein im hauptprogramm brauch ich es auch. um den einstieg zu finden.
zu dem muss es cts ununterbrochen ausgewertet werden, da ich nicht erst, wenn ich es wissen will, bei 0 anfangen kann.

im hauptprogramm sieht es ungefähr so aus.

Code: Alles auswählen

ibus = iBusface('/dev/usbtty0')
ibus.wait_free_bus(120)    # wait big free bus to get start of new message.
ibus.flush_input()
ibus.start_reading()
empty Sig
BlackJack

Ad 3.: Was genau verstehst Du denn nicht? Oder anders: Wenn Du's nicht verstehst, dann lass die Behandlung von Ausnahmen ganz bleiben. Ein ``except:`` ohne konkrete Ausnahme(n) anzugeben die da behandelt werden sollen, ist ganz ganz ganz furchtbar, solange man nicht mindestens die Ausnahme nicht samt Traceback irgendwo protokolliert, denn sonst schluckt so eine nicht-behandlung Fehler die zu Folgefehlern führen können, deren Ursache man nur sehr schwer bis gar nicht finden kann. Und sofern es nicht offensichtlich ist, gehört da immer ein Kommentar hin, der erklärt warum es in Ordnung ist an der Stelle *alle* Ausnahmen so zu behandeln, wie sie behandelt werden.

Ad 6.: `offset` kann mehr als ein Zeichen oder irgendwas anderes sein.

Ad 7.: Mir ist aus dem Quelltext so auf Anhieb nicht klar welche Semantik das ganze haben soll, aber kann es sein, dass eine Prioritätswarteschlange für die Nachrichten eventuell einfacher und verständlicher wäre als diese ganzen Flags, die ja eigentlich echte Sperrmechanismen sein sollten‽

Ad 10.: `deepcopy()` ist nicht nötig wenn man das nicht so verquer macht und eigentlich schon sehr nahe an einem Programmfehler. Niemand erwartet von einer Funktion die eine Prüfsumme berechnet und `get_irgendwas()` heisst, dass die die übergebenen Daten *verändert*. Und wie ich gezeigt habe, ist das bei dem Prüfsummenalgorithmus ja auch überhaupt gar nicht nötig die Daten zu verändern. Ob da nun eine Null ans Ende angefügt wird oder nicht, ändert nichts am Ergebnis. Also kann man es auch sein lassen.

Ad 11.: Du hast eine Schleife in der Elemente aus einer anderen Liste per `append()` angehängt werden. Dafür gibt es die `extend()`-Methode.

Ad 12.: Was Du mit dem Packet irgendwo anders machst, sollte beim Senden und Empfangen egal sein. Da hat die Hexdarstellung nichts zu suchen und macht das alles nur unnötig kompliziert mit dem ganzen unsinnigen hin- und her umwandeln. Die ”natürliche” Form von Binärdaten sind in Python 2 der `str`-Datentyp. In der Form sollten die Daten auch verarbeitet werden und nur in etwas anderes umgewandelt werden, wenn das wirklich nötig ist.

Ad 13.: Wenn die Funktion in der Klasse bleiben soll, dann sollte man wenigstens mit `staticmethod()` deutlich machen, dass man weiss, dass das keine normale Methode ist.

Nö, ich verwechsele da nichts. Warum denkst Du das? Einmal wird die Checksumme berechnet und an das Packet angehängt über das sie berechnet wurde, und einmal wird die Checksumme über ein Packet mit Checksumme berechnet — das muss immer 0 ergeben, sonst stimmt etwas nicht.

Ad 16: Ich vermute Du meinst das Aufteilen von lesen und schreiben‽ Wenn Du Dir die (Daten-)Attribute der Klasse anschaust und die in zwei Gruppen aufteilst, solche die mit Packets lesen zu tun haben und solche die mit Packets schreiben zu tun haben, gibt es dann ausser dem `Serial`-Objekt irgendwelche Attribute die in beiden Gruppen sind? Das ist ein Zeichen, dass die Klasse eigentlich zwei verschiedene Aufgaben erfüllt. Uns so komplex und lang wie die Klasse ist, wäre es vielleicht von Vorteil diese Aufgaben jeweils in ihre eigenen Klassen zu verschieben.

Ad 17.: Wie gesagt, Hex-Strings haben an der Stelle nichts zu suchen.

Sperren bei nebenläufiger Programmierung für Idioten könnte schwierig werden, denn das ist kein einfaches Thema. So eine Sperre lässt sich ja noch leicht erklären, das Problem sind aber die Situationen die man damit in den Griff bekommen möchte, also all die lustigen Probleme die man bei nebenläufiger Programmierung bekommen kann und die nicht-deterministisch sind.

Mit Zufallsprinzip ist gemeint, das wenn merere Threads für Nachrichten gleicher Priorität in der ``while``-Schleife warten (ich unterstelle hier mal dass das überhaupt fehlerfrei funktioniert), dann entscheidet der Zufall welcher davon dann weitermachen darf und welche warten müssen. Wenn über einen längeren Zeitraum immer mehr als ein Thread senden möchte, kann es sogar sein, das über den ganzen Zeitraum ein ”Pechvogel” dabei ist, der beim kloppen um das ”Lock” immer den kürzeren zieht.

Weil Du mehrere Prioritäten hast, eben auch eine Prioritätswarteschlange und keine FIFO-Warteschlange.
harryberlin
User
Beiträge: 227
Registriert: Donnerstag 17. Dezember 2015, 12:17

ich schlage vor, wir lösen leichte probleme zuerst. alles gleichzeitig verkompliziert es nur und ist nicht meine stärke.

zu 3.
"__init__" der klasse: da ist mir egal was zum fehler führt, ergebnis ist, ich kann die serial device nicht nutzen.

in der funktion "_check_free_bus": kann eigentlich nur sein, dass die serial device ein problem hat.

das gleiche dann eigentlich auch bei der funktion "_read_char". nur kommt hier noch hinzu, da das reading schon frühzeitig gestartet wird, kommt unter umständen der fehler, aber der loop soll trotzdem laufen.

in der funktion "get_bus_packet" bei dataLen: kann eigentlich nur auftreten, wenn der buffer noch nicht soweit gefüllt ist.

zu 6. ok dann eben "offset"

zu 11. ok, du meinst in der funktion "write_bus_packet" wo data dem packet hinzugefügt wird.

zu 12. meinst du hier den abschnitt in der funktion "write_bus_packet"?

Code: Alles auswählen

for i in range(len(packet)):
            packet[i] = int('0x%s' % packet[i], 16)
zu 13. check ich nicht. wie mach ich die static? und wo liegt der unterschied zwischen methode und funktion?

einmal wird die checksum dem packet zum senden angehängt.
andermal wird beim lesen, die gelesene checksum mit der eigentlichen checksum verglichen. wenn du sagst es muss 0 raus kommen, ok dann glaube ich es dir. so sehr hab ich da noch nicht weiter drüber nachgedacht.

zu 16. zwei klassen verkompliziern es dann nur für evtl. andere projekte einzusetzen. ich hätte gern eine komplettlösung.

zu 17. an welcher stelle genau haben hexstrings nichts zu suchen? code ausschnitt?

offen bleiben vorerst: 7, 10 und lock, queue
empty Sig
harryberlin
User
Beiträge: 227
Registriert: Donnerstag 17. Dezember 2015, 12:17

in der funktion "_write_low_packet" ist ein kleiner unterschied
soll sozusagen auch noch im letzten moment das senden einer low prio nachricht verhindern.
was nicht passieren darf, dass das schreiben angehelten wird, wenn einmal begonnen wurde.

Code: Alles auswählen

if not self.dont_send:
weiterhin habe ich in der funktion "write_bus_packet" gesehen:
das "self.wait_highprio_writing = True" ist fehl am platz, bzw. kann wohl komplett raus. könnt noch von älteren code ein überbleibsel sein. oder ich muss noch mal tiefer drüber nachdenken.

Code: Alles auswählen

self.dont_send = highprio
if not highprio:
    while self.wait_highprio_writing:
        time.sleep(0.001)
    self.wait_highprio_writing = True
empty Sig
Sirius3
User
Beiträge: 17754
Registriert: Sonntag 21. Oktober 2012, 17:20

@harryberlin: Du gehst das schreiben von Nachrichten komplett falsch an. Du trennst zwar high von low priority Nachrichten voneinander, aber innerhalb der beiden Gruppen werden die Nachrichten in einer beliebigen Reihenfolge (also manche unter Umständen nie) geschrieben.

Dass man nur 2 Threads braucht, soll das hier demonstrieren:

Code: Alles auswählen

#!/usr/bin/env python
# -*- coding: utf-8 -*-
 
import serial, time
from threading import Thread
from copy import deepcopy
from Queue import Queue, PriorityQueue, Empty

def log(message, log_level=1):
    print '%s' % message

def set_kodi_prop(key, value):
    log('%s - %s' % (key, value))

def is_packet_ok(packet):
    chk = packet['src'] ^ packet['len'] ^ packet['dst'] ^ packet['xor']
    for c in packet['dat']:
        chk ^= ord(c)
    return chk == 0

class IBusFace(object):
    def __init__(self, device_path):
        self.serial = serial.Serial(device_path,
            baudrate=9600,
            bytesize=serial.EIGHTBITS,
            parity=serial.PARITY_EVEN,
            stopbits=serial.STOPBITS_ONE,
            timeout=0.5)
 
        self.read_buffer = Queue()
        self.write_buffer = PriorityQueue()
 
        self.cancel = False  # set True to cancel to close the serial device
 
        self.cts_count = 0  # free bus counter
        self.errors = 0  # counter for read errors
        set_kodi_prop('msgerr_cnt', '0')
 
        time.sleep(0.2)
        self.serial.setRTS(False)
        self.flush_input()
 
        log('IBUS: Initialized')
 
        # Reading Thread
        self.read_thread = Thread(target=self._reading)
        self.read_thread.setDaemon(True)
 
        # Free Bus Thread
        self.write_thread = Thread(target=self._writing)
        self.write_thread.setDaemon(True)
        self.write_thread.start()
 
    def _reading(self):
        while not self.cancel:
            c = self.serial.read()
            if c:
                self.read_buffer.put(c)

    def read_byte(self):
        return ord(self.read_buffer.get())

    def read_bytes(self, length):
        return ''.join(self.read_buffer.get() for _ in range(length))

    def start_reading(self):
        self.read_thread.start()
 
    def flush_input(self):
        self.serial.flushInput()
 
    def set_rts(self, rts_state):
        self.serial.setRTS(rts_state)
        log('IBUS: SET RTS: ' + str(rts_state))
 
    def get_bus_packet(self):
        src = self.read_byte()
        length = self.read_byte()
        if 3 <= length <= 37:
            packet = {
                'src': src,
                'len': length,
                'dst': self.read_byte(),
                'dat': self.read_bytes(length - 2),
                'xor': self.read_byte(),
            }
            if not is_packet_ok(packet):
                log('IBUS: Corrupt packet found (%s)' % packet, 2)
                return None
            log('IBUS-READ: %s' % packet, 5)
            return packet
        else:
            log('IBUS: Length of +37 found (%s), %s' % (length, self.read_buffer), 2)
            self.errors += 1
            set_kodi_prop('msgerr_cnt', '%s' % self.msgerrcnt)
            return None

    def _writing(self):
        self.cts_count = 0
        while not self.cancel:
            if self.serial.getCTS():
                self.cts_count += 1
            else:
                self.cts_count = 0
            if self.cts_count>17:
                try:
                    prio, data = self.write_buffer.get(timeout=0.001):
                    self.serial.write(data)
                    self.serial.flush()
                    self.cts_count = 0
                except Empty:
                    pass
            else:
                time.sleep(0.001)
 
    def wait_free_bus(self, waiting=17, timeout=1000.0):
        while self.cts_count < waiting:
            if self.cancel or timeout <= 0.0:
                return False
            timeout -= 0.5
            time.sleep(0.0005)
        return True

    def write_bus_packet(self, src, dst, data, highprio=False):
        checksum = src ^ (len(data) + 2) ^ dst
        for c in data:
            checksum ^= ord(c)
        data = '%c%c%c%s%c' % (src, len(data) + 2, dst, data, checksum)
        self.write_buffer.put((0 if highprio else 1, data))
 
    def close(self):
        self.cancel = True
        time.sleep(600)
        self.serial.close()
        log('IBUS: Closed')
BlackJack

@harryberlin: Ad 3.: Das ist Dir nicht mehr egal wenn der Fehler ein Programmierfehler von Dir ist, den Du aber nicht findest, weil Du nur noch weisst *das* etwas nicht funktioniert hat, aber nicht herausfinden kannst *was* das denn nun war. Und Du verhinderst nicht, dass das Objekt trotz Fehler weiterverwendet wird, was zu Folgefehlern führen kann, die man dann erst mal mit dem eigentlichen Fehler, den Du ”versteckst”, in Verbindung bringen muss. Wenn Du diese unsinnige ”Fehlerbehandlung” einfach gar nicht machen würdest, hast Du weniger Code, niemand kann den Fehler einfach so ignorieren, absichtlich oder aus versehen, und man kann den Fehler auf höherer Ebene entweder *sinnvoll* behandeln, oder bekommt einen Programmabbruch, der einem erlaubt Fehlerursache und Stelle im Quelltext zu analysieren.

Es kann viel mehr Ausnahmen auslösen als Du Dir vorstellen kannst. Genau das ist ja gerade das Problem: Die Ausnahmen mit denen Du nicht rechnest, können halt auch auftreten, und es ist dabei dann selten sinnvoll die alle gleich zu behandeln. Wenn Du nicht an jeder Stelle mit `NameError`, `AttributeError`, `MemoryError`, `RuntimeError`, … rechnest, dann gehst Du anscheinend davon aus, dass Du ein unfehlbarer Programmierer bist. Ich halte mich für halbwegs gut, trotzdem habe ich diese Ausnahmen doch ab und zu in Code in dem ich nicht damit gerechnet habe. Und ich würde mich dumm und dämlich suchen wenn ich die dann überall einfach unterdrücken würde, weil sowas passiert ja nicht… Denkste!

Ad 12.: Ich meine überall wo etwas mit der Hexdarstellung von Bytewerten gemacht wird. Das ist ja nicht nur da.

Ad 13.: Das macht man in dem man die `staticmethod()`-Funktion als Dekorator verwendet.

Der Unterschied zwischen einer Funktion und einer Methode ist, dass eine Methode zu einem Objekt gehört mit dessen Zustand sie irgend etwas sinnvolles macht. Wenn eine Methode das nicht macht, dann ist es keine Methode, denn dann kann man sie auch als Funktion aus der Klasse heraus ziehen.

Die Prüfsumme wird bei Deinem Code über den Paketinhalt und einer 0 als letztes Byte gebildet. Da eine XOR-Verknüpfung mit 0 nichts ändert, eigentlich nur über den Paketinhalt. Das heisst der Paketinhalt ergibt einen Wert x. Der wird dann als letztes Byte des Pakets gespeichert. Wenn man jetzt die Prüfsumme über das Paket und die Prüfsumme x berechnet, dann ergibt der Algorithmus für die Paketdaten ohne Prüfsumme x und das wird dann mit x XOR verknüpft also ``x^x`` in Python-Syntax. Und jeder Wert mit sich selbst XOR verknüpft gibt 0. Ein Umstand der zum Beispiel in x86-Assembler gerne genutzt wird um ein Register auf 0 zu setzen, weil das weniger Bytes und Laufzeit in Anspruch nimmt, als einen 0-Wert aus dem Arbeitsspeicher in das Prozessorregister zu laden.

Ad 16.: Erstens ist hier von drei Klassen die Rede, die `IbusFace`-Klasse bleibt ja bestehen, und zweitens ist das Unsinn. Warum sollte es etwas verkomplizieren wenn man bei gleicher öffentlicher Schnittstelle den Code lesbarer und verständlicher aufteilt?

Ad 17.: *Überall* in dem Code haben Hexstrings nichts zu suchen. Da braucht man Binärdaten, also `str`, maximal eine Liste mit `str`-Elementen, und zum Berechnen der Prüfsumme mal kurz die einzelnen Bytewerte als Zahl.
harryberlin
User
Beiträge: 227
Registriert: Donnerstag 17. Dezember 2015, 12:17

@sirius und black
sorry, ihr zeigt mir lösungen die in unterschiedliche richtungen gehen. das verwirrt mich nur noch mehr.
eine gemeinsame lösung wäre von vorteil.

@sirius
irgendwie fehlt mir da der umgang mit fehlerhaften nachrichten.

@black
zu 3. ok, dann sag ich es mal so, ich weiß nicht wie man richtig mit except arbeitet.

zu 12. mach bitte explicit angaben. ich kann dir nicht folgen.

zu 13. und wieder ein fremdwort "dekorator", dem ich nicht mächtig bin. aber ich glaub ich habs richtig gemacht.

zu 16. verkomplizieren tut es das, weil man dann nicht nur eine klasse instanzieren muss, sondern die zweite und dritte. und dann müssen die noch zusammen arbeiten, was im hauptprogramm wieder geregelt werden muss. zumindest nach meinem verständnis.

zu 17. im code müssen hexstrings drin sein, weil ich diese 1. beim lesen zurück gegeben haben möchte und 2. zum schreiben übergeben möchte. alles andere macht meine restliches programm schwer nachvollziehbar, wenn ich "nummer" statt "hex-nummern" arbeiten muss.
für log bzw. debug ausgaben benötige ich ebenfalls die hex telegramme, da ich sie nur so sinnvoll deuten kann.

mein aktueller Code schaut jetzt so aus:

Code: Alles auswählen

#!/usr/bin/env python
# -*- coding: utf-8 -*-

import serial, time
from threading import Thread


def log(message, log_level=1):
    print '%s' % message


def set_kodi_prop(key, value):
    log('%s - %s' % (key, value))


class IBusFace(object):
    def __init__(self, device_path):
        try:
            self.serial_port = serial.Serial(device_path,
                                      baudrate=9600,
                                      bytesize=serial.EIGHTBITS,
                                      parity=serial.PARITY_EVEN,
                                      stopbits=serial.STOPBITS_ONE)
            self.connected = True
        except:
            self.connected = False
            return

        self.read_buffer = []

        self.cancel_all = False  # set True to cancel to close the serial device

        self.read_access = False  # set False to trim packet_buffer

        self.dont_send = False  # is True to access high prio answer messages
        self.writing = False  # is True while writing
        self.wait_highprio_writing = False  # let wait a low prio message

        self.ctsCnt = 0  # free bus counter
        self.msgerrcnt = 0  # counter for read errors
        set_kodi_prop('msgerr_cnt', '0')

        time.sleep(0.2)
        self.serial_port.setRTS(False)

        self.serial_port.flushInput()

        # log('SERIAL INPUT BUFFER: ' + str(self.serial_port.inWaiting())) # just for information
        log('IBUS: Initialized')

        # Reading Thread
        self.cancel_read_thread = False
        self.read_thread = Thread(target=self._reading)
        self.read_thread.setDaemon(True)

        # Free Bus Thread
        self.cancel_cts_thread = False
        self.cts_thread = Thread(target=self._check_free_bus)
        self.cts_thread.setDaemon(True)
        self.cts_thread.start()

    @staticmethod
    def _calculate_checksum(packet):
        result = 0
        for value in packet:
            result ^= value
        return result

    def _reading(self):
        self.read_access = True
        while not self.cancel_read_thread:
            self._read_char()
            time.sleep(0.001)

    def _check_free_bus(self):
        self.ctsCnt = 0
        while not self.cancel_cts_thread:
            try:
                if self.serial_port.getCTS():
                    self.ctsCnt += 1
                else:
                    self.ctsCnt = 0
                time.sleep(0.001)
            except:
                time.sleep(0.001)

    def wait_free_bus(self, waiting=17, timeout=1000.0):
        while self.ctsCnt < waiting:
            if self.cancel_cts_thread:
                return False
            if not timeout > 0.0:
                return False
            timeout -= 0.5
            time.sleep(0.0005)
        return True

    def start_reading(self):
        self.read_thread.start()

    def flush_input(self):
        self.serial_port.flushInput()

    def set_rts(self, rts_state):
        self.serial_port.setRTS(rts_state)
        log('IBUS: SET RTS: ' + str(rts_state))

    def _read_char(self):
        try:
            while self.serial_port.inWaiting() > 0:
                if self.cancel_read_thread:
                    return
                if self.read_access:
                    self.read_buffer.append('%02X' % ord(self.serial_port.read()))
        except:
            log('IBUS: Hit a serialException', 3)
            return

    def _cut_read_buffer(self, offset=1):
        self.read_access = False
        time.sleep(0.003)
        self.read_buffer = self.read_buffer[offset:]
        self.read_access = True

    def get_bus_packet(self):
        try:
            data_length = int(self.read_buffer[1], 16)
        except:
            return None

        if data_length < 3 or data_length > 37:
            log('IBUS: Length of +37 found (%s), %s' % (data_length, self.read_buffer), 2)
            self._cut_read_buffer()
            self.msgerrcnt += 1
            set_kodi_prop('msgerr_cnt', '%s' % self.msgerrcnt)
            return None
        else:
            buffer_len = len(self.read_buffer)
            if buffer_len < 5 or buffer_len < data_length + 2:
                return None
            message = self.read_buffer[:data_length + 2]
            
            if self._calculate_checksum(message) == 0:
                log('IBUS-READ: %s' % ' '.join(message), 5)
                return {
                    'src': message[0],
                    'len': message[1],
                    'dst': message[2],
                    'dat': message[3:data_length],
                    'xor': message[-1],
                }
            else:
                log('IBUS: Corrupt packet found (%s), %s' % (data_length, self.read_buffer), 2)
                self._cut_read_buffer()
                return None

    def write_hex_message(self, hexstring):
        hexstring_tmp = hexstring.split(' ')
        src = hexstring_tmp[0]
        dst = hexstring_tmp[2]
        data = hexstring_tmp[3:-1]
        self.write_bus_packet(src, dst, data)

    def write_bus_packet(self, src, dst, data, highprio=False):
        while self.dont_send:
            time.sleep(0.001)

        self.dont_send = highprio
        if not highprio:
            while self.wait_highprio_writing:
                time.sleep(0.001)
            self.wait_highprio_writing = True

        length = '%02X' % (2 + len(data))
        packet = [src, length, dst]

        packet.extend(data)

        for i in range(len(packet)):
            packet[i] = int('0x%s' % packet[i], 16)

        packet.append(self._calculate_checksum(packet))

        if highprio:
            while True:
                if self._write_high_packet(packet):
                    break
                # log('IBUS-SENT: TRY AGAIN', 2)
                time.sleep(0.001)
            
            log('IBUS-SENT: %s CK SUCCESS' % ' '.join([src, length, dst, ' '.join(data)]))

        else:
            while True:
                if self._write_low_packet(packet):
                    break
                # log('IBUS-SENT: TRY AGAIN', 2)
                time.sleep(0.001)

            log('IBUS-SENT: %s CK SUCCESS' % ' '.join([src, length, dst, ' '.join(data)]))

        if highprio:
            self.dont_send = False
        self.wait_highprio_writing = False

    def _write_low_packet(self, packet):
        if self.writing:
            return False
        data = ''.join((chr(p) for p in packet))
        self.writing = True
        if not self.cancel_all:
            if self.wait_free_bus():
                if not self.dont_send:
                    self.ctsCnt = 0
                    self.serial_port.write(data)
                    self.ctsCnt = 0
                    self.serial_port.flush()
                    self.ctsCnt = 0
                    self.writing = False
                    return True
                else:
                    self.writing = False
                    return False
            else:
                self.writing = False
                return False

    def _write_high_packet(self, packet):
        if self.writing:
            return False
        data = ''.join((chr(p) for p in packet))
        self.writing = True
        if not self.cancel_all:
            if self.wait_free_bus():
                self.ctsCnt = 0
                self.serial_port.write(data)
                self.ctsCnt = 0
                self.serial_port.flush()
                self.ctsCnt = 0
                self.writing = False
                return True
            else:
                self.writing = False
                return False

    def close(self):
        self.cancel_read_thread = True
        self.cancel_cts_thread = True
        self.cancel_all = True
        time.sleep(600)
        self.serial_port.close()
        log('IBUS: Closed')
empty Sig
BlackJack

Ad 12.: Überall wo Du Bytewerte als zwei Zeichen hast die Hexadezimalziffern entsprechen. Das wirst Du doch selbst finden können.

Ad 16.: Verkomplizieren tut es das nicht, weil es zwar mehr Einzelteile sind, die Einzelteile dafür aber einfacher zu verstehen sind. Und auch getrennt testbar sind, womit man dann wieder einfacher aussagen über Fehler beziehungsweise die Korrektheit der einzelnen Teile treffen kann.

Von welchem Hauptprogramm reden wir? Ich rede von einer Änderung die Code der die alte Klasse verwendet, überhaupt nicht mitbekommt, weil sich aus Sicht des Benutzers der bisherigen Schnittstelle nichts ändern muss. Es geht darum Komplexität aufzuteilen in in einzelnen Klassen zu isolieren, so dass man am Ende zwar die gleiche Gesamtkomplexität hat, aber einfachere Einzelteile, die man getrennt betrachten und verstehen kann.

Ad 17.: Das magst Du so sehen, ich glaube es aber nicht. Hexdarstellung ist etwas für die Anzeige und nicht etwas mit dem man Daten verarbeitet. Das hat bei binärer Datenübertragung nichts zu suchen und es macht den Code hier durch die Umwandlungen komplexer an einer Stelle wo das keiner erwartet. Schreib Dir da einen Wrapper oder meinetwegen auch extra Methoden, aber im Kern einer solchen Klasse, die zur Kommunikationsschicht gehört, hat so eine zusätzliche Darstellung der Daten nichts verloren.

Und es ist ja nicht Zahl vs. Hexdarstellung von Zahl sondern Zahl vs. Zeichenkette mit Hexdarstellung von Zahl. Ich bin mir ziemlich sicher Du machst da was komisches. Also zum Beispiel dass Du dann diese Zeichenketten mit anderen vergleichst, statt Zahlen zu vergleichen oder Zeichenketten die *ein* Byte enthalten und nicht einen Bytewert als zwei Bytes, die die Hexdarstellung des Bytewerts als ASCII-Zeichen enthalten. Das ist unnötig umständlich und kein Stück verständlicher als Zahlen oder Zeichenketten mit Bytes als Elementen zu vergleichen.

So wirklich in unterschiedliche Richtungen gehen Sirius3 und ich IMHO nicht. Wir beackern da gerade unterschiedliche Problemfelder. Ich die einfachen Sachen, Sirius3 den nebenläufigen Kram. :-)
harryberlin
User
Beiträge: 227
Registriert: Donnerstag 17. Dezember 2015, 12:17

@Sirius3
ich hab mir jetzt mal das mit queue angeschaut. zum einlesen leider nicht brauchbar, da bei get() die werte verloren gehen. sofern eine nachricht fehlerhaft ist, dann schneide ich nur das erste weg. und will alles andere weiter verwenden. so dass eben alles um eins verrutscht. daten dürfen nicht verloren gehen.
empty Sig
harryberlin
User
Beiträge: 227
Registriert: Donnerstag 17. Dezember 2015, 12:17

ok, priorityqueue hab ich teilweise auch verstanden, nur eine frage bleibt offen.
wenn ich neue items dem queue putte, egal ob prio 0 oder 1. bleibt hier dann trotzdem FIFO? was nicht passieren darf, dass sie neue sachen vorn einreihen. dann fährt man unter umstanden mit zwei queues besser?

was mir auch sorgen bereitet, dass nach den 17ms erst das telegram geholt wird. geht das denn ausreichend schnell? wenn es blöd läuft ist das telegramm geholt, aber der bus ist wieder belegt. dann gibts kollisionen und das telegram ist auch weg, weil ja nicht mehr im queue.
empty Sig
Sirius3
User
Beiträge: 17754
Registriert: Sonntag 21. Oktober 2012, 17:20

@harryberlin: das Schreiben der Pakete ist so sicher gelöst, wie es eben geht. Natürlich geht FIFO bei einer PriorityQueue verloren, weil die höher priorisierten Pakete als erstes geliefert werden. Aber das ist doch genau das, was Du willst. Für genau Deinen Fall sind PriorityQueue gemacht, damit man eben nicht umständlich mit mehreren Queues arbeiten muß. Im Gegensatz zu Deiner Lösung, wo an vielen unterschiedlichen Stellen per sleep geschlafen wird, wird bei meiner Lösung exakt nach 17ms und wenn ein Paket zum Senden ansteht das Paket mit der höchsten Priorität gesendet. Das weiß ich deshalb so genau, weil alles, was das Schreiben betrifft in einer Schleife mit einer Handvoll Zeilen steckt und nicht über eine ganze Klasse und mehrere Threads verteilt ist.

Das Lesen von Paketen ist bei mir noch miserabel gelöst, weil ich die Fehlerbehandlung nicht verstehe. Ich glaube kaum, dass einzelnen Bytes weglassen und dann hoffen, dass eine sinnvolle Nachricht kommt, eine stabile Lösung ist.
harryberlin
User
Beiträge: 227
Registriert: Donnerstag 17. Dezember 2015, 12:17

@Sirius3
es darf nicht sein, dass eine spätere high prio nachricht sich vor die andere einreiht. FIFO muss hier dennoch bestehen.

beim lesen ist die fehlerbehandlung wichtig, da so kollisionen beim schreiben erkannt werden können. oder es werden bus fehler erkannt, die man nicht ausschließen sollte. jetzt wird vermutlich blackjack den finger heben, wegen den excepts. zudem gibt es tatsächlich korrupte nachrichten auf dem bus. z.B. beim aufwecken des bus gibt es eine merkwürdige nachricht die keine informationen enthält. eine autokorrektur ist hier nicht sinnvoll. also besser korrupte sachen verwerfen als sie versuchen zu verstehen. deswegen ja auch die checksum.

wenn man wüsste wie weit die daten fehlerhaft sind, kann man natürlich mehr verwerfen, nur weiß man es eben nicht. also schritt für schritt.

bmw ibus ist hier ganz gut erklärt: http://www.alextronic.de/bmw/projects_b ... _ibus.html
empty Sig
DasIch
User
Beiträge: 2718
Registriert: Montag 19. Mai 2008, 04:21
Wohnort: Berlin

Schieb in die PriorityQueue Tupel in der Form (priority, message_count, message). message_count ist dabei einfach ein Integer den du für jede Nachricht erhöhst. Wenn die Priorität gleich ist, wird somit nach message_count sortiert und du erreicht das FIFO Verhalten, das du haben möchtest.
harryberlin
User
Beiträge: 227
Registriert: Donnerstag 17. Dezember 2015, 12:17

@DasIch
Ist nur leider auch endlich. Klar man kann sagen "das programm läuft eh nur zeitweise" dann ist egal. Aber vllt. wirds ja doch mal eine Dauerlösung.
empty Sig
Antworten