FHEM Forum

CUL - Entwicklung => Fehlerberichte => Thema gestartet von: noansi am 22 Dezember 2015, 21:38:01

Titel: Bug in write_eeprom
Beitrag von: noansi am 22 Dezember 2015, 21:38:01
Hallo Rudolf,

beim Umstellen der Kommandointerpretation (siehe hier http://forum.fhem.de/index.php/topic,24436.msg377742.html#msg377742 (http://forum.fhem.de/index.php/topic,24436.msg377742.html#msg377742)) ist mir in fncollection.c in write_eeprom noch ein Bug und ein ungünstiges Fehlerverhalten aufgefallen.

void
write_eeprom(char *in)
{
  uint8_t hb[6], d = 0;

#ifdef HAS_ETHERNET
  if(in[1] == 'i') {
    uint8_t *addr = 0;
           if(in[2] == 'm') { d=6; fromhex(in+3,hb,6); addr=EE_MAC_ADDR;
    } else if(in[2] == 'd') { d=1; fromdec(in+3,hb);   addr=EE_USE_DHCP;
    } else if(in[2] == 'a') { d=4; fromip (in+3,hb,4); addr=EE_IP4_ADDR;
    } else if(in[2] == 'n') { d=4; fromip (in+3,hb,4); addr=EE_IP4_NETMASK;
    } else if(in[2] == 'g') { d=4; fromip (in+3,hb,4); addr=EE_IP4_GATEWAY;
    } else if(in[2] == 'p') { d=2; fromdec(in+3,hb);   addr=EE_IP4_TCPLINK_PORT;
    } else if(in[2] == 'N') { d=4; fromip (in+3,hb,4); addr=EE_IP4_NTPSERVER;
    } else if(in[2] == 'o') { d=1; fromhex(in+3,hb,1); addr=EE_IP4_NTPOFFSET;
#ifdef HAS_NTP
      extern int8_t ntp_gmtoff;
      ntp_gmtoff = hb[0];
#endif
    }
    for(uint8_t i = 0; i < d; i++)
      eub(addr++, hb[i]);

  } else
#endif
  {
    uint16_t addr;
    d = fromhex(in+1, hb, 3);
    if(d < 2)
      return;
    if(d == 2)
      addr = hb[0];
    else
      addr = (hb[0] << 8) | hb[1];
     
    eub((uint8_t*)addr, hb[d-1]);

    // If there are still bytes left, then write them too
    in += (2*d+1);
    while(in[0]) {
      addr++;
      if(!fromhex(in, hb, 1))
        return;
      eub((uint8_t*)addr++, hb[0]);
      in += 2;
    }
  }
}


Hier
    // If there are still bytes left, then write them too
    in += (2*d+1);
    while(in[0]) {
      addr++;
      if(!fromhex(in, hb, 1))
        return;
      eub((uint8_t*)addr++, hb[0]);
      in += 2;
    }

wird bei jedem Schleifendurchlauf die Schreibadresse 2 mal erhöht. Ich denke nicht, dass das so gedacht ist, oder?

Ich würde folgenden Code vorschlagen:
void
write_eeprom(char *in)
{
  uint8_t hb[6], d;

  uint8_t c = *(++in);

#ifdef HAS_ETHERNET
  if(c == 'i') {
    d = *(++in);
    in++;
    uint8_t *addr = 0;
    uint8_t e;
           if(d == 'm') { d=6; e=12 ; fromhex(in,hb,6); addr=EE_MAC_ADDR;
    } else if(d == 'd') { d=1; e=1  ; fromdec(in,hb);   addr=EE_USE_DHCP;
    } else if(d == 'a') { d=4; e=7  ; fromip (in,hb,4); addr=EE_IP4_ADDR;
    } else if(d == 'n') { d=4; e=7  ; fromip (in,hb,4); addr=EE_IP4_NETMASK;
    } else if(d == 'g') { d=4; e=7  ; fromip (in,hb,4); addr=EE_IP4_GATEWAY;
    } else if(d == 'p') { d=2; e=1  ; fromdec(in,hb);   addr=EE_IP4_TCPLINK_PORT;
    } else if(d == 'N') { d=4; e=7  ; fromip (in,hb,4); addr=EE_IP4_NTPSERVER;
    } else if(d == 'o') { d=1; e=2  ; fromhex(in,hb,1); addr=EE_IP4_NTPOFFSET;
#ifdef HAS_NTP
      extern int8_t ntp_gmtoff;
      ntp_gmtoff = hb[0];
#endif
    } else {
      return;  // invalid command
    }

    // check for end of String even to find unexpected uneven number of characters in buffer with hex numbers
    while (e--)
    {
      if (!*(in++))
        return;
    }
    // now we can write data to eeprom
    for(c = 0; c < d; c++)
      eub(addr++, hb[c]);

  } else
#endif
  {
    uint16_t addr;
    d = fromhex(in, hb, 3);
    if(d < 2)
      return;
    if(d == 2)
      addr = hb[0];
    else
      addr = (hb[0] << 8) | hb[1];
     
    c = d - 1;  // index of byte to write in buffer

    d *= 2;  // expected number of characters from buffer done
    while(1) {
      // check for end of String even with uneven number of characters in buffer
      while (d--)
      {
        if (!*(in++))
          return;
      }
      eub((uint8_t*)(addr++), hb[c]);  // full byte, we can write it

      if(!in[0] || !fromhex(in, hb, 1))
        return;
      // If there are still bytes left, then write them too
      c = 0;  // index of byte to write in buffer
      d = 2;  // 2 characters from buffer expected done
    }
  }
}

Damit wird beim vergessen oder "verschlucken" eines HEX-Nibbles auf der seriellen Schnittstelle eher ein falscher Eintrag ins EEPROM vermieden. Das könnte so manchen Hinweis auf Zurücksetzen des EEPROMS vermeiden helfen.

Viele Grüße und frohes Fest,

Ansgar.
Titel: Antw:Bug in write_eeprom
Beitrag von: rudolfkoenig am 23 Dezember 2015, 10:12:22
Zitatwird bei jedem Schleifendurchlauf die Schreibadresse 2 mal erhöht. Ich denke nicht, dass das so gedacht ist, oder?
Da hast du recht, vielen Dank.  Ganz tragisch ist das nicht: dieser Feature ist weder dokumentiert, noch wird sie mWn irgendwo verwendet.

ZitatIch würde folgenden Code vorschlagen:
Leider ist dieser Vorschlag eine (gefuehlte) Neuimplementation, und ich habe nicht die Energie um sie auf semantische Aequivalenz mit dem alten Code zu pruefen. Daher habe ich einen "minimalinvasiven" Fix eingecheckt.
Titel: Antw:Bug in write_eeprom
Beitrag von: noansi am 23 Dezember 2015, 11:05:10
Hallo Rudolf,

ZitatDa hast du recht, vielen Dank.  Ganz tragisch ist das nicht: dieser Feature ist weder dokumentiert, noch wird sie mWn irgendwo verwendet.

Da ja bald Weihnachten ist wäre der passende Feature Nutzungswunsch ein regelmäßiger EEPROM Backup mit Restore Funktion.  ::)
Ist allerdings nicht ganz so einfach, da beim Abfragen des EEPROM Inhalts schon mal eine z.B. Temperatur Message eines Empfangsprotokolls dawischen funken kann. (Beim Neustart und Abfragen der Version kommt da schon mal was falsches, weil der Empfangsbuffer nicht immer hundertprozentig leer gemacht sein kann bevor der Read Befehl  bei CUL ankommt. Da hilft nur alle Protokolle temporär abzuschalten und hinterher das vorher aktive wieder einzuschalten, oder eine Erweiterung des read_eeprom() um das senden des kompletten EEPROM Inhalts).

Zitatsie auf semantische Aequivalenz mit dem alten Code zu pruefen
Es wird nur auf eine (minimal) erwartete Anzahl von Zeichen geprüft, ansonsten nicht geschrieben und abgebrochen. Bei HEX Daten wird die Erwartung auf die passende gerade Anzahl von Zeichen gesetzt und bei Dezimal oder IP auf die jeweils minimal nötige Anzahl Zeichen.

Kostet natürlich auch etwas mehr Speicherplatz (den Du ja bei ZWave schon frei vermisst) und ist bei richtiger Nutzung und fehlerfreier Übertragung (kleiner Pferdefuss) zu CUL nicht notwendig.

Zum freien Speicherplatz: Meine Umstellung der Kommandointerpretation in meiner TimeStamp Firmware Variante http://forum.fhem.de/index.php/topic,24436.msg377742.html#msg377742 (http://forum.fhem.de/index.php/topic,24436.msg377742.html#msg377742) hat nochmal einige xx Bytes gebracht. Ist allerdings bei konzentrierter Arbeit mit einem Tag Arbeit verbunden, da fast alle .c Dateien in clib entsprechend angepasst werden müssen.
Ich habe in culfw-code-459-trunk_lufa_rf_cr_sd_75_63.zip alles mit #ifdef USE_TTYDATA_NO_FUNCTION_CODE mein neuer Code #else mein alter Code #endif versehen. Damit ist es zumindest leicht zu finden und zu vergleichen.

Wegen meiner vielen sonstigen Änderungen ist es mit einem diff leider nicht mehr getan, um das allgemein verfügbar zu machen.

Gruß und Danke, Ansgar.