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

[DasIch]

Das ist endlich nur insofern dass message_count irgendwann nicht mehr in den Arbeitsspeicher passt. An die Grenze wirst du so schnell nicht stoßen. Das ist vielleicht offensichtlicher wenn du überlegst dass mein Vorschlag äquivalent dazu ist einen timestamp zu benutzen.

[harryberlin]

@DasIch
Keine schlechte Idee.
Doch Timestamp ist nicht so gut, da ich die Zeit vom Bus lese und im Pi dann entsprechend setze.

[Sirius3]

@harryberlin: wenn Du eine PriorityQueue brauchst, die auch noch die Reihenfolge innerhalb einer Priorität einhält, dann schreib Dir doch einfach selbst eine:

Code: Alles auswählen

from collections import defaultdict, deque
from Queue import Queue

class PriorityQueue(Queue):
    '''Variant of Queue that retrieves open entries in priority order (lowest first).

    Entries are tuples of the form:  (priority number, data).
    '''

    def _init(self, maxsize):
        self.queue = defaultdict(deque)

    def _qsize(self):
        return sum(len(q) for q in self.queue.iteritems())

    def _put(self, item):
        self.queue[item[0]].append(item)

    def _get(self):
        prio = min(self.queue)
        result = self.queue[prio].popleft()
        if not self.queue[prio]:
            del self.queue[prio]
        return result

Wie schon geschrieben, hast Du darauf ja in Deiner ursprünglichen Implementierung auch keinen Wert gelegt.

[harryberlin]

...dann schreib Dir doch einfach selbst eine...

Danke für die Mühe.
"einfach" hast du gut formuliert. Das hätte ich nie auf einfache Weise zustande gebracht.
Darf man denn den Name "PriorityQueue" so nutzen, da es doch eigentlich der von einem modul ist.

[BlackJack]

@Sirius3: `self.queue` ist ein irreführender Name, und `_qsize()` ist fehlerhaft. Da müsste man über `itervalues()` iterieren, denn die `iteritems()` haben alle die Länge 2, egal wie viele Elemente in der Queue stecken.

Ich denke die Variante von DasIch ist besser, denn da bleiben die (Laufzeit)Eigenschaften der Heapqueue erhalten, die dort als Implementierung dahinter steckt.

@harryberlin: Welches Modul hat den Namen `PriorityQueue`? In der Standardbibliothek gibt es keines das so heisst.

[harryberlin]

@BlackJack
meine meinung: wer weit unten im code arbeitet, geht davon aus, dass "PriorityQueue" importiert wurde, und erwartet ein verhalten, was nicht dem entspricht.

man könnte doch der lösung von DasIch eine routine dazu bastseln, die feststellt, wenn die queue leer ist, und den counter zurücksetzt.
das liese sich evtl. sogar über die "except Empty:" lösen.

[BlackJack]

@harryberlin: Das ist ein ziemlich generischer Name einer allgemein bekannten abstrakten Datenstruktur. Ich denke nicht, dass man sich dafür einen anderen Namen ausdenken muss, nur weil es den auch in einem Modul in der Standardbibliothek gibt. Dann müsste man bei *jedem* Namen, den man selbst vergibt, erst einmal im Index von der Python-Dokumentation nachschauen. Und da sind *viele* Namen. Teilweise ja auch innerhalb der Standardbibliothek mehrfach verwendete (`open()`), oder dann auch in (bekannten/verbreiteten) Bibliotheken von Drittanbietern (`Image`). Und so wirklich viel anders verhält sich die `PriorityQueue` von Sirius3 vom Effekt her ja auch nicht. Es ist halt eine Prioritätswarteschlange, die zusätzlich innerhalb der Prioritäten noch eine Reihenfolge beachtet.

Bevor man den Zähler zurück setzt, sollte man da vielleicht erst einmal nachweisen, dass das ein Problem ist/wird. Ab welcher Byteanzahl wäre Dir eine ganze Zahl an der Stelle denn zu gross? Wenn der Zähler innerhalb eines Jahres Dauerbetrieb 64-Bit erreicht, entspricht das über 580 Milliarden Packets *pro Sekunde*. Wenn das technisch überhaupt über die Leitung gehen würde, was ich bei 9600 Baud sehr stark bezweifle.

[Sirius3]

@BlackJack: ja, die Flüchtigkeitsfehler, itervalues ist natürlich richtig. self.queue benutzen alle Queue-Implementationen zum Speichern der Elemente, daher der Name. Ob ich eine Laufzeit von O(n) erwarte oder wie bei der "normalen" Queue von O(1) ist wohl Geschmacksache. Bei wenigen Prioritäten, wie bei diesem Problem, bin ich nahe an O(1), was mir persönlich sympatisch ist.

So ein Jahr kann ziemlich schnell vorbei sein. Daher würde ich eher sagen, dass, wenn man alle 17ms eine Nachricht verschickt, ein 64bit-Zähler gerade mal 993 Millionen Jahre reicht.

[BlackJack]

Hm, ob das dann reicht? Was ist denn die Lebenserwartung von einem qualitativ guten BMW im Dauerbetrieb?

[harryberlin]

So ihr beiden Scherzkekse

Hab nun mal die Tasten klimpern lassen und größtenteils den code umgeschrieben.
Auch Hex-String-hin-und-her wollte ich vermeiden, dennoch vorerst noch zurück geben beim lesen und zum schreiben übergeben.

Das öffnen des Serial Ports habe ich geändert.
Ich habs noch nicht getestet, da ich aktuell zwei Probleme sehe:
1. in der funktion "write_bus_packet", die so aufgerufen werden soll:

Code: Alles auswählen

ibus.write_bus_packet('80', '3B', ['02', '36'])

funktioniert das denn überhaupt mit der checksum,

Code: Alles auswählen

checksum = src ^ (len(data) + 2) ^ dst

oder sollte es so aussehen?:

Code: Alles auswählen

checksum = int(src, 16) ^ (len(data) + 2) ^ int(dst, 16)

2. in der funktion "read_bus_packet", bei der rückgabe:
wie mache ich das mit der data liste

Code: Alles auswählen

'data': message[3:data_length],  # TODO: Convert to hex string array(exp. ['0A', 'AD']) in one line?

edit:
ich habs

Code: Alles auswählen

'data': ['%02X' % i for i in message[3:data_length]],

und hier mal der ganze code. bitte keine anmerkungen zu den log, note, und dialog_ok funktionen und monitor class. das wäre grad "out of topic"

Code: Alles auswählen

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

###### intall these pakets #####
# sudo apt-get install python-serial

import xbmc, xbmcgui, xbmcaddon
import time
from threading import Thread
import os
import serial
from Queue import PriorityQueue, Empty

__author__ = 'harryberlin'

ADDON = xbmcaddon.Addon()
ADDONNAME = ADDON.getAddonInfo('name')
ADDONID = ADDON.getAddonInfo('id')
ADDONPATH = ADDON.getAddonInfo('path')

ICON = os.path.join(ADDONPATH, 'icon.png')


def log(string, lvl=1):
    thread = Thread(target=_log_thread, args=(string, lvl))
    thread.setDaemon(True)
    thread.start()


def _log_thread(string, lvl=1):
    return xbmc.log('IBUSCOMMUNICATOR: ' + string)


def note(heading, message=" ", time=5000):
    thread = Thread(target=_note_thread, args=(heading, message, time))
    thread.setDaemon(True)
    thread.start()


def _note_thread(heading, message=" ", time=5000):
    xbmcgui.Dialog().notification(heading=' %s' % heading, message=' %s' % message, icon=ICON, time=time)
    log('NOTIFICATION: ' + heading + ' - ' + message)


def dialog_ok(label1, label2='', label3=''):
    thread = Thread(target=_dialog_ok_thread, args=(label1, label2, label3))
    thread.setDaemon(True)
    thread.start()


def _dialog_ok_thread(label1, label2='', label3=''):
    xbmcgui.Dialog().ok('IBusCommunicator', label1, label2, label3)


def set_kodi_prop(property, value, id=10000):
    xbmc.executebuiltin('SetProperty(%s,%s,%s)' % (property, value, id))


def get_kodi_prop(property, id=10000):
    value = xbmcgui.Window(id).getProperty(property)
    return value


class Monitor(xbmc.Monitor):
    def __init__(self):
        xbmc.Monitor.__init__(self)


class IBusFace(object):
    def __init__(self, device_path):
        self.serial_port = serial.Serial()
        self.serial_port.port = device_path
        self.serial_port.baudrate = 9600
        self.serial_port.bytesize = serial.EIGHTBITS
        self.serial_port.parity = serial.PARITY_EVEN
        self.serial_port.stopbits = serial.STOPBITS_ONE

        self.read_error_counter = 0  # counter for read errors
        set_kodi_prop('msgerr_cnt', '0')

        self.read_buffer = []
        self.read_access = False  # set False to trim packet_buffer

        self.write_buffer = PriorityQueue()
        self.write_counter = 0

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

        # Writing Thread
        self.cancel_write_thread = False
        self.write_thread = Thread(target=self._writing)
        self.write_thread.setDaemon(True)

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

    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 _wait_free_bus(self, waiting=17, timeout=1000):
        cts_counter = 0
        while cts_counter < waiting:
            if self.serial_port.getCTS():
                cts_counter += 1
                if not timeout > 0:
                    return False
                timeout -= 1
            else:
                cts_counter = 0
                if not timeout > 0:
                    return False
                timeout -= 1
            time.sleep(0.001)
        return True

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

    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(ord(self.serial_port.read()))
        except:
            log('IBUS: Hit a serialException')
            return

    def _writing(self):
        cts_count = 0
        while not self.cancel_write_thread:
            if self.serial_port.getCTS():
                cts_count += 1
            else:
                cts_count = 0
            if cts_count > 17:
                try:
                    prio, write_counter, data = self.write_buffer.get(timeout=0.001)
                    self.serial_port.write(data)
                    self.serial_port.flush()
                    cts_count = 0
                except Empty:
                    pass
            else:
                time.sleep(0.001)

    def connect(self):
        try:
            self.serial_port.open()
            self.serial_port.setRTS(False)

            if not self._wait_free_bus(120, 2000):
                log('IBUS: Can not locate free Bus')
                return False

            self.serial_port.flushInput()

            # start reading
            self.read_thread.start()
            self.write_thread.start()
            return True
        except:
            return False

    def disconnect(self):
        self.cancel_read_thread = True
        self.cancel_write_thread = True

        time.sleep(200)
        if self.serial_port.isOpen():
            self.serial_port.close()

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

    def set_rts(self, rts_state):
        self.serial_port.setRTS(rts_state)

    def get_rts(self):
        return self.serial_port.rts

    def read_bus_packet(self):
        try:
            data_length = self.read_buffer[1]
        except:
            # list is not big enough
            return None

        if data_length < 3 or data_length > 37:
            #log('IBUS: Length of +37 found (%s), %s' % (data_length, self.read_buffer), 2)
            log('IBUS: Length of +37 found (%s)' % data_length)
            self._cut_read_buffer()
            self.read_error_counter += 1
            set_kodi_prop('msgerr_cnt', '%s' % self.read_error_counter)
            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': '%02X' % message[0],
                    'len': '%02X' % data_length,  # message[1]
                    'dst': '%02X' % message[2],
                    'data': ['%02X' % i for i in message[3:data_length]],
                    'xor': '%02X' % message[-1],
                }
            else:
                #log('IBUS: Corrupt packet found (%s), %s' % (data_length, self.read_buffer), 2)
                log('IBUS: Corrupt packet found')
                self._cut_read_buffer()
                return None

    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, self.write_counter, data))
        self.write_counter += 1

    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 main():
    monitor = Monitor()

    ibus = IBusFace('/dev/ttyUSB0')

    log('IBus connect')
    if not ibus.connect():
        note('IBus Error', 'Can not open serial device')
        return

    note('IBus connected')

    while not monitor.abortRequested():
        packet = ibus.read_bus_packet()
        if packet:
            log('%s %s %s %s %s' % (packet['src'], packet['len'], packet['dst'], ' '.join(packet['data']), packet['xor']))
        time.sleep(0.001)

    log("IBus disconnect")
    ibus.disconnect()


if __name__ == '__main__':
    main()

[harryberlin]

Habs nun getestet.
Es war noch ein Fehler drin. Bei erfolgreich erkannten Packet wurde es nicht aus dem buffer gelöscht.
Und bei "'data': ['%02X' % i for i in message[3:data_length + 1]]," hat noch + 1 gefehlt.
und das lesen läuft schon mal.

[Sirius3]

@harryberlin: die natürlich Repräsentation von Zahlen in einem Programm sind Zahlen und nicht Hex-Strings. Und die natürliche Repräsentation von Bytes sind Bytestrings und nicht Hex-Strings. Bei src könnte man sich noch streiten, was denn das ist, String oder Zahl, aber bei len!

Ein Aufruf von write_bus_packet kann genausogut so aussehen, ohne die Probleme, die Deine Hex-Repräsentation überall macht:

Code: Alles auswählen

ibus.write_bus_packet(0x80, 0x3B, '02 36'.replace(' ', '').decode('hex'))

Und wenn Du eine Methode write_hex_message brauchst, dann halt so:

Code: Alles auswählen

    def write_hex_message(self, hexstring):
        data = ''.join(hexstring.split()).decode('hex')
        self.write_bus_packet(src=ord(data[0]), dst=ord(data[1]), data=data[3:-1])

Warum startest Du zum Loggen einen eigenen Thread? Und wohin returned _log_thread was auch immer?
Die Monitor-Klasse ist sinnfrei.
Die Parameterübergabe gleich beim Serial-Aufruf fand ich übersichtlicher. read_access hat immer noch die gleichen Probleme mit Nebenläufigkeit. Die korrekte Lösung ist auch hier eine Queue zu benutzen!
_wait_free_bus ist umständlich, wenn nicht sogar fehlerhaft. Du hast jetzt zwei unabhängige cts-Counter. Warum bist Du von meiner Lösung hier abgewichen?
Busy-Waiting bei _read_char ist schlecht. Warum bist Du da von meiner Lösung abgewichen?
connect ersetzt eine sinnvolle Exception durch einen nichtssagenden Returnwert. BlackJack hat ja dieses Thema schon ausführlichst erörtert. try-except ersartzlos löschen!
set_rts und get_rts würde man schöner als Property schreiben.
read_bus_packet: welche Exception erwartest Du da? None ist kein guter Rückgabewert bei einem Fehler. Ich würde ja so lange warten, bis ein korrektes Paket gelesen wurde, eventuell noch mit einem TimeOut-Parameter, falls man nicht solange warten will.

[harryberlin]

@Sirius3
1.
die message ist bei mir angekommen, dass hex strings nicht gut sind. jedoch aktuell ist es nun mal so(großteil vom code habe ich nicht gezeigt). warum habe ich bereits erklärt.

Code: Alles auswählen

ibus.write_bus_packet('80', '3B', ['02', '36'])

dass da noch was passieren muss, ist angekommen.

2.
wie erwähnt, keine diskussion zu log, note, dialog_ok und monitor klasse.
returnt wird der aufruf einer xbmc funktion.
die monitor klasse wird noch erweitert und soll schon mal im code so stehen.

3.
ich finde es mit connect besser, da ich unabhängig von init connecten und disconnecten kann.

4.
wie mache ich ein property daraus?

5.
welche nebenläufigkeit meinst du genau? ich habe einmal den loop, der mir die daten in den buffer schiebt. andermal will ich den buffer kürzen und will das rein schaufeln von daten also überspringen.
wo siehst du da problem mit dem flag?
eine queue ist hier fehl am platz, weil wenn ich z.B. 10 zeichen aus der queue geholt habe und merke "uh, das kannst ja gar nicht gebrauchen", dann fange ich an mir einen buffer zu basteln, weil die 10 zeichen aus dem queue raus sind aber nicht verworfen werden dürfen. unter umständen fängt 2 zeichen später ein brauchbares telegram an.

6.
ich muss am anfang auf eine große lücke warten, um einen guten einstieg zu finden. ich könnte natürlich auch gleich direkt einsteigen, aber erzeuge unter umständen readerrors, die man von vorherein vermeiden kann.
diese funktion wird auch nur einmal aufgerufen und vor dem start der threads.
so sehe ich hier keine probleme.
die cts counter brauchen keinen zusammenhang mehr.

7.
ich bin abgewichen, weil eine queue nicht sinnvoll ist. siehe 4.

8.
der return-wert bei connect ist True oder False. eindeutiger als schwarz oder weiß erkenne ich hier nicht.
zudem weiß ich nicht so recht mit excepts umzugehen. wenn ich nach sinnvollen quellen frage, bekomme ich aus erfahrung keine rückmeldung(ja ich kann mit google umgehen).
also erspare ich es mir und gehe dem mal nach, wenn ich richtig lust dazu habe.
ihr solltet es nicht immer gleich als 100% fehlerfreie sache ansehen, was hier vorgelegt wird. es sieht vllt. fast aus wie copy/paste. nur ich versuche auch zu verstehen, was ich da kopiere. und war eigentlich vorerst ganz zufrieden mit dem was ich da nun zusammen gebastelt hab.

9.
bei read_bus_packet wird None returnt wenn eben nix brauchbares dabei ist. da dies über ein loop aufgerufen wird, wäre es nicht sinnvoll zu warten. ein timeout rein zu stricken, macht die funktion nur komplexer.

unterm strich möchte ich sagen, so ausführungen mit kleinen codeschnippseln (wie zu hex und string), sowas verstehe ich besser. dem kann ich besser folgen, als text den ich mir 10 mal durchlesen muss und nach fremdwörtern googlen muss. aber kein problem, das mache ich auch gern.
und danke für die unterstützung.

[Sirius3]

ad 1: wenn Du diese Signatur für Dein externes Interface noch brauchst, dann schreib Dir eine kurze Wrapper-Methode, aber intern kannst Du schon sauber arbeiten und aller neuer Code arbeitet dann mit der sauberen Schnittstelle.

ad 2: warauf ich hinaus wollte: der Returnwert wird nirgends verwendet, weil wohin soll der Thread das denn schicken? Die Threads sehen einfach nur so aus, als ob sie reichlich überflüssig sind.

ad 3: versteh den Zusammenhang mit meinem Post nicht

ad 4: mit @property

ad 5: ich muß nicht verstehen, was Du genau mit welchen Zeichen machst, um zu erkennen, dass sobald mehrere Threads ungeschützt auf die selbe Struktur zugreifen, es kracht. Eine Queue ist die einfachste Lösung, jetzt mußt Du nur noch Dein restliches Programm so umschreiben, dass es mit der Queue umgehen kann. Das sauberste wäre, das Erkennen der Telegramme innerhalb des read-Threads zu erledigen und read_bus_packet in ein einfaches read_queue.get() zu verwandeln. Dann ist das mit dem TimeOut auch nicht komplex.

ad 6: race-conditions sind komplex. cts-counter wird hier nicht mit dem Counter aus dem write-Thread synchronisiert und damit ist die Methode fehlerhaft. Lösung ein cts-counter, der solange er verändert wird durch ein Lock geschützt ist.

ad 7: siehe ad 5

ad 8: try-except ist der Ersatz für return True/False. Da Du ersteres schon halbwegs einsetzt, mach den Fehler des nackten excepts besser im Hauptprogramm und Log die Fehelermeldung wenigstens mit.

ad 9: siehe ad 5

[harryberlin]

zu 1.
step by step. alles auf einmal lösen ist nicht meine stärke.

zu 2.
die threads sind da, um nicht unnötig aufzuhalten. ob das der fall ist, darüber lässt sich streiten.

zu 3.
zu Aufruf des Serial

zu 4.
du meinst so:

Code: Alles auswählen

@property
def rts(self):
    return self.serial_port.rts

@rts.setter
def rts(self, rts_state):
    self.serial_port.setRTS(rts_state)

und aufruf zum setzen dann:
ibus.rts = True
und abfrage einfach
rtslevel = ibus.rts

zu. 5
a) gelesen wird nur von einem thread.
b) wenn die daten aus dem queue geholt(get) wurden, und nicht brauchbar sind, sind sie weg. aber das darf nicht sein! es muss ein zeichen wegfallen und erneut geprüft werden, ggf. weitere nachgeholt werden. wenn von einer geldscheinrolle ein schein zerissen ist, verwirfst du doch auch nicht gleich die ganze rolle, sondern schaust dir die anderen scheine an. oder habe ich beim verstehen der englischen queue doku was falsch verstanden?
c) ich verstehe nicht, warum du mir da gedanklich nicht folgen kannst?
ok, ich habs noch mal versucht zu verstehen, aber ich komme hier nicht weiter, wie bring ich lock und den thread in verbindung?
oben im init habe ich noch zusätzlich

Code: Alles auswählen

self.read_lock = Lock()

und dann die funktion "_cut_read_buffer" geändert. geht das denn? lock weiß doch gar nicht welchen thread es anhalten soll.

Code: Alles auswählen

def _cut_read_buffer(self, offset=1):
        with self.read_lock:
            self.read_buffer = self.read_buffer[offset:]

zu 6.
die cts-counter sind völlig unabhängig. die funktion wait_free_bus könnte man nun genauso gut direkt in die connect funktion rein schreiben.

zu 8.
du meinst return Exception und dann wie gehts weiter im hauptprogramm? so vielleicht?

Code: Alles auswählen

...
if packet == Exception:
    log(packet)
else:
    log('%s %s %s %s %s' % (packet['src'],
                                    packet['len'],
                                    packet['dst'],
                                    ' '.join(packet['data']),
                                    packet['xor']))
...

[BlackJack]

@harryberlin: Ausnahmen werden nicht mit ``return`` zurückgegeben, dann muss der Aufrufer das ja tatsächlich prüfen damit es nicht einfach unter den Tisch fällt, dann hättest Du auch `True` und `False` zurück geben können. Ausnahmen werden mit ``raise`` ausgelöst. Und man kann sie mit ``try``/``except`` behandeln. Aber auch wirklich nur da wo man das auch tatsächlich sinnvoll machen kann, und auch nur die Ausnahemen die man an der Stelle erwartet und deshalb sicher sein kann, dass die Behandlung *tatsächlich* sinnvoll ist.

[harryberlin]

also "return None" mit "raise 'fehler so und so'" ersetzen?

und im hauptprogramm?:

Code: Alles auswählen

try:
    packet = ibus.read_bus_packet()
except:
    log(Exception)

sorry, ich versteh nicht was ihr von mir verlangt.

ich habe diese funktion hier:

Code: Alles auswählen

def read_bus_packet(self):
        if self.cancel_read_thread:
            return None
        try:
            data_length = self.read_buffer[1]
        except:
            # list is not big enough
            return None

        if data_length < 3 or data_length > 37:
            #log('IBUS: Length of +37 found (%s), %s' % (data_length, self.read_buffer), 2)
            log('IBUS: Length of +37 found (%s)' % data_length)
            self._cut_read_buffer()
            self.read_error_counter += 1
            set_kodi_prop('msgerr_cnt', '%s' % self.read_error_counter)
            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)
                self._cut_read_buffer(data_length + 2)
                return {
                    'src': '%02X' % message[0],
                    'len': '%02X' % data_length,  # message[1]
                    'dst': '%02X' % message[2],
                    #'data': message[3:data_length],  # TODO: Convert to hex string (exp. '0A') in one line?
                    'data': ['%02X' % i for i in message[3:data_length + 1]],
                    'xor': '%02X' % message[-1],
                }
            else:
                #log('IBUS: Corrupt packet found (%s), %s' % (data_length, self.read_buffer), 2)
                log('IBUS: Corrupt packet found')
                self._cut_read_buffer()
                return None

1. return None tritt ein, wenn der read_thread nicht läuft bzw. dabei ist abzubrechen.
2. return None tritt ein, wenn die liste noch nicht die größe hat um aus [1] zu lesen
3. return None tritt ein, wenn eine telegrammlänge erkannt wurde, die kleine 3 ist(da es solche nachrichten nicht gibt) und größer 37(da es keiner längeren telegramme gibt) und sogar wird dazu noch geloggt. aktuell ohne daten aber das kommt wieder.
4. return None tritt ein, wenn der read_buffer noch nicht groß genug ist
5. return gibt das packet zurück
6. return None tritt ein, wenn das packet korrupt ist.

mein gedankengang ist folgender: warum soll ich all diese sachen zurückgeben und loggen? das ballert mir doch in einer sekunde tausend zeilen ins logfile. da wo es interessant(aus meiner sicht) ist, wird der log-eintrag erzeugt.

edit:
ok hier hab ich jetzt was brauchbares gefunden, wie man raise anwenden kann: viewtopic.php?f=1&t=38778
ist das jetzt richtigerer als "return None"? von der aufgabe her ist es ziemlich das gleiche. aus meiner sicht des anfängers -> schöner wohnen.

[BlackJack]

@harryberlin: Fang doch bitte als erstes mal an keine nackten ``except:``\s zu schreiben! Das ist in den allermeisten Fällen ein *Programmfehler*. Nicht nur irgendwie unschön, sondern schlicht *falsch*.

Die Funktion ist ein schlechtes Beispiel, weil wie Sirius3 schon schrieb die API schlecht ist. Wenn `read_bus_packet()` auch unter ganz normalen Umständen `None` zurückgeben kann, nur weil gerade kein Packet empfangen wurde, dann muss man ja zwangsläufig immer wieder Methode aufrufen. Das zwingt einen an jeder Stelle wo man die Methode verwenden will um das nächste Packet zu lesen, eine Schleife zu schreiben. Statt dass das jeder Aufrufer machen muss, gehört das *einmal* in so eine Methode. Dann gibt's auch keine Fälle mehr wo `None` geliefert werden muss, weil die Methode entweder ein Packet zurück gibt oder eine Ausnahme auslöst.

Dann wären da auch keine Ausnahmen dabei die Dir das Logfile zuballern, denn den Fall „noch nicht genug Daten, ruf noch (tausend) mal auf“, gibt es dann ja nicht mehr. Und ob der Lesethread nicht läuft, oder Pakete kaputt sind, kann den Aufrufer durchaus interessieren und an einem `None` erkennt er das nicht. Eventuell interessiert es den direkten Aufrufer auch gar nicht, weil der sowieso an der Stelle nichts sinnvolles damit anfangen kann. Dann müsste er auf `None` prüfen und abbrechen und des seinem Aufrufer weiterreichen, falls der etwas damit anfangen kann. Und dort das gleiche. Da muss man immer Code für schreiben der das berücksichtigt, prüft, und die Fehlerwerte weiter nach oben durchreicht. Das vergisst man mal, oder man hat keinen Bock sich darum zu kümmern weils aufwändig ist, und Fehler ja sowieso nicht auftauchen, das passt schon irgendwie. Tut es halt in der Praxis nicht. Um das alles sicherer zu machen und für den Programmierer zu vereinfachen, wurde das Konzept von Ausnahmen erfunden. Das ist nicht ”schöner wohnen”, sondern Fehler ignorieren oder als spezielle Fehlerwerte in der Gegend herum zu reichen als wenn es normale Rückgabewerte wären, ist ”schlechter wohnen”.

Aus Deiner Liste stimmt 2. nicht ganz: `None` wird zurückgegeben wenn *irgendeine* Ausnahme auftritt, nicht nur wenn die Liste nicht mindestens zwei Elemente enthält. Das Attribut `read_buffer` könnte durch einen Fehler an einer anderen Stelle nicht, oder nicht mehr, existieren. `self` oder `read_buffer` könnten einen Schreibfehler enthalten. `read_buffer` könnte durch einen Fehler an etwas anderes als eine Liste gebunden sein, was nicht mit dem Indexzugriff klar kommt. In all den Fällen läuft Dein Programm in dem `read_bus_packet()` immer und immer wieder aufgerufen wird, es aber nie zum sinnvollen lesen eines Packets kommen kann. Viel Spass beim finden von dem Fehler. Insbesondere wenn Du solche nackten ``except:``\s regelmässig im Programm stehen hast.

[harryberlin]

@harryberlin: Fang doch bitte als erstes mal an keine nackten ``except:``\s zu schreiben! Das ist in den allermeisten Fällen ein *Programmfehler*. Nicht nur irgendwie unschön, sondern schlicht *falsch*.....

Dann gib mir bitte den Ansatz einer Lösung. Ich raffs nicht wirklich.
Ich höre immer nur raus "hier dies und das machst du nicht richtig." "Das ist nicht gut, weils nicht gut ist. Ein Programmierer macht das so nicht."
Ist auch was gutes dabei? Sorry, bin grad bissl aufgewühlt, weils sehr wenig posiitive Kritik gibt.

Soll ich raise SomeError("what ever error") nehmen und

Code: Alles auswählen

try:
   print my_function(args)
except SomeError as msg:
   print msg
   # and fail...
# and now continue safely here...

oder ist SomeError nicht das richtige?

wenn self oder read_buffer falsch geschrieben ist, dann müsste syntax error kommen.

[harryberlin]

besser?:

Code: Alles auswählen

def read_bus_packet(self):
        if self.cancel_read_thread:
            raise Warning('<read_thread> was canceled')
        try:
            data_length = self.read_buffer[1]
        except:
            raise IndexError('<read_buffer> list is not big enough')

        if data_length < 3 or data_length > 37:
            self._cut_read_buffer()
            self.read_error_counter += 1
            set_kodi_prop('msgerr_cnt', '%s' % self.read_error_counter)
            raise ValueError('IBUS: Length of < 3 or > 37 found (%s)' % data_length)
        else:
            buffer_len = len(self.read_buffer)
            if buffer_len < 5 or buffer_len < data_length + 2:
                raise IndexError('<read_buffer> list < %s to get input values' % data_length + 2)
            message = self.read_buffer[:data_length + 2]

            if self._calculate_checksum(message) == 0:
                #log('IBUS-READ: %s' % ' '.join(message), 5)
                self._cut_read_buffer(data_length + 2)
                return {
                    'src': '%02X' % message[0],
                    'len': '%02X' % data_length,  # message[1]
                    'dst': '%02X' % message[2],
                    'data': ['%02X' % i for i in message[3:data_length + 1]],
                    'xor': '%02X' % message[-1],
                }
            else:
                self._cut_read_buffer()
                raise ValueError('IBUS: Corrupt packet found')

Code: Alles auswählen

try:
    packet = ibus.read_bus_packet()
except IndexError:
    pass
except Warning:
    pass
except ValueError as e:
    log(e.args)
finally:    
    log('%s %s %s %s %s' % (packet['src'],
                                     packet['len'],
                                     packet['dst'],
                                     ' '.join(packet['data']),
                                     packet['xor']))