Messwerte in Datenbank kann mal jemand drüberschauen

[Hugo66]

Hallo,

gelesen hab ich schon reichlich in diesem Forum geschrieben jetzt zum 1. mal. wie so eineige andere auch hab ich mir ein script gebastelt und codeschnippsel verwendet.

Als Vorhaben möchte ich gern (mit dem PI) Messwerte von 1wire in eine Datenbank schreiben. ich möchte nicht wie viele anderen jeden Wert in eine separate Spalte sondern alles schön klassisch untereinander.
ID // Datum // HardwareID // Messwert
so kann ich es später mit einem kleinen Webinterface besser auswerten.

Mein Script läuft gut.... naja, für mich sieht es gut aus. Ich möchte es Euch hier gern mal zeigen und wünsche mir Kritik. Bin mir sicher Verbesserungspotential ist vorhanden und lernbereitschaft sowieso.

Code: Alles auswählen

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

# Import sys module
import sys
import MySQLdb
import datetime
import time

# Open 1-wire slaves list for reading
file = open('/sys/devices/w1_bus_master1/w1_master_slaves')

# Read 1-wire slaves list
w1_slaves = file.readlines()

# Close 1-wire slaves list
file.close()

#Open Mysql connection 
mysql = MySQLdb.connect(host="localhost",user="USER",passwd="PASS",db="TESTDATENBANK")

# Repeat following steps with each temperature sensor
for line in w1_slaves:

  # Extract 1-wire slave
  w1_slave = line.split("\n")[0]

  # Open 1-wire slave file
  file = open('/sys/bus/w1/devices/' + str(w1_slave) + '/w1_slave')

  # Read content from 1-wire slave file
  filecontent = file.read()

  # Close 1-wire slave file
  file.close()

  # Extract temperature string
  stringvalue = filecontent.split("\n")[1].split(" ")[9]

  # Convert temperature value
  temperature = float(stringvalue[2:]) / 1000

  # Formate Date
  datum = time.strftime("%Y-%m-%d %H:%M:%S")
  
  # write into database	
  cur = mysql.cursor()
  cur.execute("INSERT INTO `TESTDATENBANK`.`sensors_data` (`timestamp`, `sensor_id`, `value`) VALUES ('%s', '%s', '%f')" % (datum, w1_slave, temperature))
  mysql.commit()

# Quit python script
sys.exit(0)

Wo ich mir absulut nicht sicher bin ist die formatierung des Datums. hier muss es doch einen einfacherern Weg geben. Hab da schon einige zeit rumprobiert. Meine Mysql Tabelle kann es nur so lesen und die Formatierung der Spalte ist "varchar".......

nunja, vlt. gebt Ihr mir ein paar Tips um die Gechsichte zu optimieren.

LG Hugo

[Sirius3]

@Hugo66: Der Typ einer Datenbankspalte die ein Datum enthält sollte nicht varchar sein, sondern timestamp. Dateien werden mit with geöffnet. Line ist schon eine Zeile, also der Split mit '\n' überflüssig, statt dessen solltest Du strip verwenden. Dateipfade werden nicht mit + sondern mit os.path.join zusammengesetzt, der str-Aufruf ist auch überflüssig, da es sich schon um einen String handelt. Datenbankbefehle sollten nicht per String-Formatierung zusammengesetzt werden: die DB-API von Python unterstützt Wildcards. Das sys.exit am Schluß ist unnötig.

[BlackJack]

@Hugo66: Kommentare sollten dem Leser einen Mehrwert über den Code geben, also etwas nicht-triviales aussagen was man nicht sowieso schon am Code ablesen kann. Also vor einem ``import sys`` zu kommentieren dass dort das `sys`-Modul importiert wird ist definitiv überflüssig. Insgesamt ist das Programm überkommentiert. Jede Zeile zu kommentieren und jeweils Leerzeilen dazwischen zu setzen zieht alles unnötig in die Länge.

Einrücktiefe ist per Konvention vier Leerzeichen pro Ebene. Zeilen sollten nicht zu lang werden. Auch wenn der Style Guide mittlerweile etwas gelockert ist an der Stelle, würde ich trotzdem maximal 80 Zeichen als Grenze anstreben.

Auf Modulebene sollten nur Konstanten, Funktionen, und Klassen definiert werden. Alles andere gehört in Funktionen. Mit dem ``if __name__ == '__main__':``-Idiom kann man dann dafür sorgen, das man das Modul ohne Seiteneffekte importieren *und* als Modul importieren kann.

Namen von eingebauten Funktionen und Datentypen, wie `file`, sollte man nicht an andere Werte binden. Das kann verwirrend sein, und führt spätestens wenn man den eingebauten Wert mal benötigt, zu Problemen.

Dateien sollte man mit der ``with``-Anweisung zusammen öffnen, um sicherzustellen, das sie in jedem Fall auch wieder geschlossen werden.

Statt eine Datei komplett in den Speicher zu lesen die man dann zeilenweise verarbeiten möchte, kann man direkt über das Dateiobjekt iterieren, denn das liefert die Zeilen als Elemente.

``line.split("\n")[0]`` ist in diesem Fall eine ziemlich komplizierte Art und Weise ``line.rstrip()`` zu schreiben.

Zeichenketten und Werte mit ``+`` und `str()` zusammenzusetzen ist eher BASIC als idiomatisches Python. Zumal `str()` bei `w1_slave` noch nicht einmal einen Effekt hat. In Python würde man an der Stelle Zeichenkettenformatierung mit der `format()`-Methode oder den ``%``-Operator verwenden.

Bei der `execute()`-Methode begehst Du einen Fehler: Du formatierst die Werte *selber* in die Zeichenkette mit dem SQL. Das ist eine Sicherheitslücke und zum Beispiel bei Datumswerten wie Du ja gemerkt hast nicht wirklich robust. `execute()` kennt ein zweites Argument für die Werte. Und als Datum kann man dort ein `datetime.datetime`-Objekt übergeben wenn die Spalte in der Datenbank einen entsprechenden Typ hat, also zum Beispiel ``DATETIME`` oder ``TIMESTAMP``.

Man muss nicht für jeden SQL-Befehl ein neues Cursor-Objekt erstellen.

Ein ``sys.exit(0)`` am Programmende ist überflüssig. Das passiert an der Stelle ja sowieso.

Ich lande dann bei so etwas (ungetestet):

Code: Alles auswählen

#!/usr/bin/env python
# -*- coding: utf-8 -*-
from datetime import datetime as DateTime
import MySQLdb


def main():
    connection = MySQLdb.connect(
        host='localhost', user='USER', passwd='PASS', db='TESTDATENBANK'
    )
    with open('/sys/devices/w1_bus_master1/w1_master_slaves') as lines:
        cursor = connection.cursor()
        for line in lines:
            w1_slave_name = line.strip()
            with open(
                '/sys/bus/w1/devices/{0}/w1_slave'.format(w1_slave_name)
            ) as sensor_lines:
                next(sensor_lines)  # Skip first line.
                temperature = float(next(sensor_lines).split()[9][2:]) / 1000
            cursor.execute(
                'INSERT INTO TESTDATENBANK.sensors_data'
                ' (`timestamp`, sensor_id, value)'
                ' VALUES (%s, %s, %s)',
                (DateTime.now(), w1_slave_name, temperature)
            )
            connection.commit()


if __name__ == '__main__':
    main()

Nächster Schritt wäre vielleicht den Code sinnvoll auf Funktionen zu verteilen.

[/me]

Dateien werden mit with geöffnet.

Das sollte man vielleicht präzisieren. Dateien werden schon mit open geöffnet, aber die Verwendung des Kontextmanagers mit with führt dazu, dass man sich nicht um Aufräumarbeiten (wie hier das Schließen der Datei) kümmern muss.

[Hugo66]

ohh mein Gott. Vielen Dank an Euch alle. und vielen Dank an @BlackJack. Der Code sieht ja nun volkommen anders aus.

ich werde es mir merken. zuviele Kommentare sind auch schlecht....

Hab Deinen Code mal eingebunden und natürlich die SQL Tabelle angepasst. Läuft genial! Ich glaub ich muss noch viel lernen. Probiere glaub grad noch am Python "Hello World 1x1" rum.

nochmal zum skript:

Wenn ich jetzt eine Prüfung auf ungültige Werte einfügen möchte würde ich dazu

Code: Alles auswählen

mintemp = -30
if temperature > mintemp:

ganzer Code:

Code: Alles auswählen

#!/usr/bin/env python
# -*- coding: utf-8 -*-
from datetime import datetime as DateTime
import MySQLdb
     
     
def main():
    connection = MySQLdb.connect(
        host='localhost', user='root', passwd='gsadaten', db='test'
    )
    with open('/sys/devices/w1_bus_master1/w1_master_slaves') as lines:
        cursor = connection.cursor()
        for line in lines:
            w1_slave_name = line.strip()
            with open(
                '/sys/bus/w1/devices/{0}/w1_slave'.format(w1_slave_name)
            ) as sensor_lines:
                next(sensor_lines)  # Skip first line.
                temperature = float(next(sensor_lines).split()[9][2:]) / 1000
                mintemp = -30
                if temperature > mintemp:
            	    cursor.execute(
                        'INSERT INTO test.sensors_data'
                        ' (`timestamp`, sensor_id, value)'
                        ' VALUES (%s, %s, %s)',
                        (DateTime.now(), w1_slave_name, temperature)
                    )
                    connection.commit()
     
     
if __name__ == '__main__':
    main()

nehmen.... richtig? -->funktionieren tut es zumindest

Was wäre wenn ich noch einen zweiten und dritten Wert angeben möchte? also z.B.
Schreibe nur Werte die Größer -30 und kleiner +30 sind aber auch nicht exakt 0 sein dürfen.

kommt zwar nicht vor, aber interresieren würde es mich.

[BlackJack]

@Hugo66: Dann musst Du den Ausdruck für die Bedingung entsprechend erweitern und mehrere Tests mit den entsprechenden logischen Operatoren verbinden. Also für das Beispiel kann man Testoperatorverkettung und ``and`` verwenden: ``if -30 <= temperature <= 30 and temperature != 0:``.

[Hugo66]

@BlackJack,

ja herzlichen Dank. es läust alles prima. jetzt weis ich gar nicht so recht was ich noch am besten für sinnvolle Funktionen einfügen könnte.

Hugo

[BlackJack]

@Hugo66: Ich hoffe ich hatte mich nicht missverständlich ausgedrückt: Ich meinte nicht das man *Funktionalität* *hinzufügen* sollte, sondern das man diesen Haufen Code der *alles* macht, sinnvoll auf Funktionen *aufteilen* könnte, die jeweils eine in sich geschlossene Aufgabe erledigen. Also Beispielsweise eine Funktion die den Namen von einem Sensor übergeben bekommt und die Temperatur ausliest und zurück gibt. Und eine Funktion die einen DB-Cursor und Name des Sensors und Wert übergeben bekommt und das mit der aktuellen Zeit in die Datenbank schreibt, und so weiter.

[Hugo66]

ok. ich verstehe. zur zeit ist es ja so das eine Schleife die Dateinen ausliest und dann eine Mysql verbindung öffnet und die Daten reinschreibt. Nach Deiner Variante würde dann eine Funktion die Daten in eine Art Array schreiben, wenn alle Daten da sind würde die Funktion beendet werden. Die nächste Funktion nimmt den Array, öffnet nur einmal eine Verbindung zur Datenbank und schreibt alle daten zusammen in die DB. Dies würde dann (so wie ich es verstehe) eine Menge Zeit sparen weil die Verbindung mit Mysql nur einmal hergestellt werden muss.

ist das so korrekt? Bis ich sowas hinbekomme muss ich aber noch ne ganze Weile üben....

Hugo

[BlackJack]

@Hugo66: Die Verbindung erstellst Du jetzt auch nur einmal. Man kann das auch so schreiben das sich vom Datenfluss her nicht viel ändert, denn man kann die Funktion zum Auslesen der Werte auch so schreiben das die Funktion nicht erst alles ausliest und als Liste zurück gibt, sondern ein iterierbares Objekt liefert welches die Werte erst beim iterieren tatsächlich abfragt. Stichworte hier sind Generatorfunktion (``yield``-Anweisung) oder Generatorausdruck.

Das Aufteilen auf Funktionen macht das Programm ”nur” leichter zu lesen und zu testen, und man kann Teile wiederverwenden.

Edit: Mal als Beispiel ohne das Lesen und das Schreiben aller Werte zu trennen (ungetestet):

Code: Alles auswählen

#!/usr/bin/env python
# -*- coding: utf-8 -*-
from datetime import datetime as DateTime
import MySQLdb
 

def iter_sensor_names():
    with open('/sys/devices/w1_bus_master1/w1_master_slaves') as lines:
        for line in lines:
            yield line.strip()


def read_temperature_sensor(name):
    with open('/sys/bus/w1/devices/{0}/w1_slave'.format(name)) as lines:
        next(lines)  # Skip first line.
        return float(next(lines).split()[9][2:]) / 1000


def insert_temperature(cursor, sensor_name, temperature):
    cursor.execute(
        'INSERT INTO TESTDATENBANK.sensors_data'
        ' (`timestamp`, sensor_id, value) VALUES (%s, %s, %s)',
        (DateTime.now(), sensor_name, temperature)
    )


def main():
    connection = MySQLdb.connect(
        host='localhost', user='USER', passwd='PASS', db='TESTDATENBANK'
    )
    cursor = connection.cursor()
    for sensor_name in iter_sensor_names():
        temperature = read_temperature_sensor(sensor_name)
        insert_temperature(cursor, sensor_name, temperature)
        connection.commit()
 
 
if __name__ == '__main__':
    main()