Pointer suche

[Pal .. (pal)]

Hallo,

ich bin als blutiger Anfänger auf der Suche nach einem Pointer-Problem. 
ICh würde mich freuen, wenn mir jemand weiterhelfen könnte.

Kurz zu meinem Problem.... Mir ist aufgefallen, wenn ich in der Funktion 
"sendeJoyTastenToDisplay" die Arraylängen der Variablen "deleteTasten", 
"noTasten", "taste1", "taste2", ... "taste56" z.b. mit 40 angebe erhalte 
ich ein flakerndes Bild an dem angeschlossenen Display (siehe Bild im 
Anhang). Lasse ich die Längen weg funktioniert alles. Mit der Eingabe 
der Länge verschiebe ich ja eigentlich nur die Speicherbereiche - was 
auf ein Pointer-Problem deutet. Trotz dieser schönen Erkentnisse kann 
ich mir auf das Ganze leider keinen Reim bilden.

Ich hoffe ihr könnt mir weiterhelfen

Vielen Dank!

[Karl H. (kbuchegg)]

pal ... schrieb:

> Anhang). Lasse ich die Längen weg funktioniert alles. Mit der Eingabe
> der Länge verschiebe ich ja eigentlich nur die Speicherbereiche

mit der Angabe der Länge verbrauchst du vor allen Dingen erst mal 
unendlich viel Speicher für nichts.
Sicher dass du nicht den verfügbaren Speicher überläufst?

* Lass den Compiler die Stringlänge zählen.
  der macht das zuverlässiger als du und dimensioniert die Arrays
  so lang wie benötigt

* das abschliessende \0 Zeichen hängt der Compiler für dich an
  ein String-Literal an. Das brauchst du nicht selber machen

* Höchst wahrscheinlich brauchst du die Arrays in dieser Funktion
  überhaupt nicht. Solange die Send Funktion an den Daten nichts
  ändert, kannst du der auch getrost den String direkt übergeben

void sendeJoyTastenToDispl(const struct JOYSTICK* j) {
  uint8_t key1 = 0;
  uint8_t key2 = 0;
  uint8_t key3 = 0;
  uint8_t key4 = 0;
  uint8_t key5 = 0;
  uint8_t key6 = 0;

    if(j->keys & TASTE1) {
      key1 = 1;
    }
    if(j->keys & TASTE2) {
      key2 = 1;
    }
    if(j->keys & TASTE3) {
      key3 = 1;
    }
    if(j->keys & TASTE4) {
      key4 = 1;
    }
    if(j->keys & TASTE5) {
      key5 = 1;
    }
    if(j->keys & TASTE6) {
      key6 = 1;
    }


  if (key6 == 1 && (key1 == 1 || key2 == 1 || key3 == 1 || key4 == 1 || key5 == 1)) {

    if(key1 == 1 && oldKey != JOY_TASTE_16) {
      sendeDaten("#RL150,120,380,300,");                // Bereich löschen und mit Hintergrundfarbe füllen
      sendeDaten("#UI230,120,7,#UI220,230,12,");                  // Tasten 1 und 6 betätigt
      oldKey = JOY_TASTE_16;
    }


...

Allenfalls macht es noch Sinn, die Strings mittels ein paar #define an 
einer Stelle zusammenzuziehen

#define NO_TASTE      "#UI230,120,6,#UI225,235,13,"
#defein DELETE_TASTE  "#RL150,120,380,300,"
#define TASTE_1_6     "#UI230,120,7,#UI225,235,13,"
...

void sendeJoyTastenToDispl(const struct JOYSTICK* j) {
  uint8_t key1 = 0;
  uint8_t key2 = 0;
  uint8_t key3 = 0;
  uint8_t key4 = 0;
  uint8_t key5 = 0;
  uint8_t key6 = 0;

    if(j->keys & TASTE1) {
      key1 = 1;
    }
    if(j->keys & TASTE2) {
      key2 = 1;
    }
    if(j->keys & TASTE3) {
      key3 = 1;
    }
    if(j->keys & TASTE4) {
      key4 = 1;
    }
    if(j->keys & TASTE5) {
      key5 = 1;
    }
    if(j->keys & TASTE6) {
      key6 = 1;
    }



  if (key6 == 1 && (key1 == 1 || key2 == 1 || key3 == 1 || key4 == 1 || key5 == 1)) {

    if(key1 == 1 && oldKey != JOY_TASTE_16) {
      sendeDaten( DELETE_TASTE );                // Bereich löschen und mit Hintergrundfarbe füllen
      sendeDaten( TASTE_1_6 );                  // Tasten 1 und 6 betätigt
      oldKey = JOY_TASTE_16;
    }

    if(key2 == 1 && oldKey != JOY_TASTE_26) {
      sendeDaten( DELETE_TASTE );
      sendeDaten( TASTE_2_6 );
      oldKey = JOY_TASTE_26;

      aktZustandMesser = 1;
      messer = 1;
      oldZustandMesser = 0;
    }

...

Wenn das dann immer noch zuviel Speicher verbraucht, müsste man darüber 
nachdenken, die Strings ins Flash zu verfrachten(*), damit sie kein SRAM 
verbrauchen.

(*) bzw. eigentlich  nur im Flash zu haben. Denn im Moment braucht jeder 
String Platz im Flash UND im SRAM.

[Pal .. (pal)]

Hallo Karl Heinz,

super... danke für deine ausführliche Beschreibung. Das hab selbst ich 
verstanden :-)

stimmt... die ganzen Variablen kann ich mir sparen - es funktioniert so, 
habe es gerade ausprobiert.
Danke!

Ich denke nicht, dass ich momentan noch zuviel Speicher brauch. Sobald 
ich compeliere bekomme ich diese Anzeige:

Program:    3668 bytes (22.4% Full)
(.text + .data + .bootloader)

Data:        626 bytes (61.1% Full)
(.data + .bss + .noinit)

Das sollte doch noch genügend sein? Das Optimization Level steht auf 
"Os".
Der verwendete Controller ist ein ATmega16.


Mein Problem ist, dass mein Programm schon deutlich ausführlicher (in 
Sachen Quellcode) war und irgendwann genau dieses Problem wieder 
aufgetaucht ist.
Dann habe ich es wieder "weg optimiert" z.B. durch ein "const" vor den 
Variablen. Danach habe ich am Programm weiter geschrieben und irgendwann 
tauchte das Ganze wieder auf. Will jetzt einfach sichergehen, dass es 
kein Pointer-Problem oder sonst einen Denkfehler hab.

mfg pal

[Karl H. (kbuchegg)]

pal ... schrieb:

> Ich denke nicht, dass ich momentan noch zuviel Speicher brauch. Sobald
> ich compeliere bekomme ich diese Anzeige:
>
>

> Program:    3668 bytes (22.4% Full)
> (.text + .data + .bootloader)
> 
> Data:        626 bytes (61.1% Full)
> (.data + .bss + .noinit)
>

>
> Das sollte doch noch genügend sein? Das Optimization Level steht auf
> "Os".
> Der verwendete Controller ist ein ATmega16.

reicht erst mal. 61% lässt noch genug Reserve für den Stack und lokale 
Variablen (solange du es nicht wieder übertreibst).

Bei dir war das ja so:
Die Strings stehen erst mal im Flash und verbrauchen dort Speicher. 
(denn irgendwo müssen die ja beim Programmstart mal herkommen. Der SRAM 
merkt sich ja nichts wenn man den Strom abdreht)

Von dort werden die String-Literale beim Hochfahren des Programms ins 
SRAM umkopiert. Wenn ich jetzt mal alle Strings über den Daumen 
zusammenzähle, dann sind das rund 300 Bytes.

Jetzt versuchst du in der Funktion noch 13 Arrays a 40 Bytes anzulegen, 
macht 520 Bytes

d.h. alleine diese beiden Posten verbrauchen beim Aufruf dieser Funktion 
820 Bytes. Du hast aber nur 1024 Bytes SRAM! Jetzt noch ein paar lokale 
Variablen, ein bischen was am Return-Stack, noch ein paar globale 
Variablen und die 1024 sind voll.


-> auch wenn des Softwerkers Herz schmerzt: keine übermässig grossen 
funktionslokale Variablen! Ein paar uint8_t oder int sind ok (für 
Variablen die man für Schleifensteuerung oder dergleichen braucht), 1 
oder 2 Arrays mit 10 oder 20 Elementen ist auch ok. Aber mehr solls 
nicht sein.

Das Problem: Alles was du hast um dich über den Speicherverbrauch zu 
informieren, ist diese Zusammenfassung. Da tauchen aber lokale Variablen 
nicht auf! Wie denn auch, die werden ja erst erzeugt, wenn die Funktion 
aufgerufen wird. D.h. aber auch: benutzt du lokale Variablen für große 
Sachen, dann ist die Zusammenfassung nutzlos, weil du nicht weißt 
wieviel Speicher dann zur Laufzeit noch zusätzlich verbraucht wird. Du 
musst dir also bei dieser Statistik eine Reserve lassen, die du in den 
meisten Fällen einfach schätzen wirst. Bei vielen und großen Arrays 
verschätzt man sich aber über den Speicherverbrauch ganz schnell.

[Pal .. (pal)]

Hallo Karl Heinz,

das war mir alles gar nicht so bewusst. Für den Speicherverbrauch habe 
ich mir immer die 22.4% betrachtet.

Werde wahrscheinlich deinen Ratschlag mit den defines berücksichtigen. 
Das wird wohl noch etwas Platz einsparen. Ich werde erstmal wieder 
versuchen mit deinen Infos weiter zu programmieren und dich/euch über 
meinen Erfolg oder Misserfolg auf dem laufenden halten :-)

Vielen Dank auch noch mal!

[Karl H. (kbuchegg)]

pal ... schrieb:
> Hallo Karl Heinz,
>
> das war mir alles gar nicht so bewusst. Für den Speicherverbrauch habe
> ich mir immer die 22.4% betrachtet.


Das ist der Teil, den dein Programm-Code im Flash verbraucht.

Aber dein Programm besteht ja nicht nur aus Code, sondern es braucht ja 
auch Variablen. Die liegen im SRAM und in der Statistik taucht das als 
"data" auf.

[Pal .. (pal)]

ok....
ich denke der Controller ist dann für meine Anwendung deutlich zu klein. 
Werde bald auf den ATmega644 gehen - der ist doppelt so groß.

mfg pal

[Karl H. (kbuchegg)]

pal ... schrieb:
> ok....
> ich denke der Controller ist dann für meine Anwendung deutlich zu klein.

:-)
Wenns eng wird, kriegen wir da noch mal 300 Bytes (für die konstanten 
Strings) frei.

'Wissen wie's geht' ist immer besser als 'Hau drauf und hinter mir die 
Sintflut'

[Pal .. (pal)]

super, freut mich :-)

werde noch mal an meinem Programm weiter machen und melde mich wieder. 
Wir müssen sicherlich noch einige Bytes befreien :-)

danke!!!

[Pal .. (pal)]

Hallo,

soooo... ich denke jetzt ist es an der Zeit das Ein oder Andere Byte 
einzusparen. Die Strings hatte ich schon einmal durch defines ersetzt - 
hat aber nichts gebracht.

Ich hoffe ihr könnt mir wieder dabei helfen. Der Teil des Codes, der am 
meisten Speicher frisst habe ich mal online gestellt.

Zur Info... Nach dem compelieren zeigt die Statistik folgende Werte für 
meinen ATmega16

Device: atmega16

Program:    4766 bytes (29.1% Full)
(.text + .data + .bootloader)

Data:        873 bytes (85.3% Full)
(.data + .bss + .noinit)

EEPROM:        1 bytes (0.2% Full)
(.eeprom)

Danke für eure Hilfe!

[Karl H. (kbuchegg)]

Ich nehm jetzt nur willkürlich eine FUnktion heraus

void sendeJoyBewToDispl(const struct JOYSTICK* j) {

    if (j->direction == ypos) {
      if(oldDirection != JOY_VOR) {
        sendeDaten("#RL200,30,350,120,#UI255,35,2,");    // Bereich löschen und mit Hintergrundfarbe füllen - Bild für Joystick vor anzeigen
        oldDirection = JOY_VOR;
      }
    }
    else if (j->direction == yneg) {
      if(oldDirection != JOY_ZURUECK) {
        sendeDaten("#RL200,30,350,120,#UI255,35,3,");
        oldDirection = JOY_ZURUECK;
      }
    }
    else if (j->direction == xneg) {
      if(oldDirection != JOY_LINKS) {
        sendeDaten("#RL200,30,350,120,#UI255,35,5,");
        oldDirection = JOY_LINKS;
      }
    }
    else if (j->direction == xpos) {
      if(oldDirection != JOY_RECHTS) {
        sendeDaten("#RL200,30,350,120,#UI255,35,4,");
        oldDirection = JOY_RECHTS;
      }
    }
    else {
      if(oldDirection != JOY_NULL) {
        sendeDaten("#RL200,30,350,120,");
        oldDirection = JOY_NULL;
      }
    }
}

was fällt dir auf, wenn du die Strings vergleichst?
Also mir fällt auf, das das im Prinzip immer die gleichen Strings sind, 
nur hinten ist eine 'Zahl' anders. Du legst aber für dieses 1 Byte 
Unterschied jeweils einen kompletten String mit 30 Bytes an. 5 Strings 
hast du, alle 5 fangen vollkommen identisch an und bei den restlichen 4, 
gehts dann auch identisch weiter.

Das kann man doch sicher auch so umgestalten, dass diese Tatsache 
ausgenutzt wird und man den kompletten String in 3 Teilstrings zerlegt, 
deren Zusammensetzung dann die gewünschten Strings ergibt.

static const char* ClearJoyArea = "#RL200,30,350,120,";
static const char* SetJoyArea   = "#UI255,35,";


void sendeJoyBewToDispl(const struct JOYSTICK* j) {

    if (j->direction == ypos) {
      if(oldDirection != JOY_VOR) {
        sendeDaten( ClearJoyArea  );
        sendeDaten( SetJoyArea );
        sendeDaten( "2,");    // Bereich löschen und mit Hintergrundfarbe füllen - Bild für Joystick vor anzeigen
        oldDirection = JOY_VOR;
      }
    }
    else if (j->direction == yneg) {
      if(oldDirection != JOY_ZURUECK) {
        sendeDaten( ClearJoyArea  );
        sendeDaten( SetJoyArea );
        sendeDaten( "3,");
        oldDirection = JOY_ZURUECK;
      }
    }
    else if (j->direction == xneg) {
      if(oldDirection != JOY_LINKS) {
        sendeDaten( ClearJoyArea  );
        sendeDaten( SetJoyArea );
        sendeDaten( "5," );
        oldDirection = JOY_LINKS;
      }
    }
    else if (j->direction == xpos) {
      if(oldDirection != JOY_RECHTS) {
        sendeDaten( ClearJoyArea  );
        sendeDaten( SetJoyArea );
        sendeDaten( "4," );
        oldDirection = JOY_RECHTS;
      }
    }
    else {
      if(oldDirection != JOY_NULL) {
        sendeDaten( ClearJoyArea  );
        oldDirection = JOY_NULL;
      }
    }
}

mal rechnen, wie jetzt die Bilanz aussieht.
Vorher hattest du
4 Strings a 31 Bytes       .... 124 Bytes
1 String    19 Bytes       ....  19 Bytes
                              ------------
                                143 Bytes

Jetzt hast du
1 String    19 Bytes             19
1 String    11 Bytes             11
4 String a   4 Bytes             16
                              -----------
                                 46 Bytes

Fazit: Fast 100 Bytes eingespart.



Schau dir deine Funktion sendeMaschAuswToDispl an. Praktisch alle 
Ausgaben fangen mit #MN an. Das kann man durchaus auch vorziehen und nur 
eine einzige Ausgabe von #MN machen lassen an die dann hinten nur noch 
die Nummer angehängt wird.


Und jetzt hab ich mir tatsächlich nur die allerersten beiden Funktionen 
angesehen, die ich in deinem Code gesehen habe. Da schlummern sicherlich 
noch andere Schätze.


Edit: Yep
In sendeJoyTastenToDispl fangen ebenfalls alle Strings mit einer 
identischen Sequenz an bzw. haben signifikant große gleiche Teile.

Zur Not macht man sich dann auch schon mal Spezialfunktionen, die einem 
das Senden eines Strings in mehreren Happen erlauben. Die "#MN" Sache 
müsste man zb ein wenig anders angehen. Man kann ja zb eine 
Spezialfunktion machen, die immer zuerst mal "#MN" sendet und dann den 
übergebenen String noch hinten nach absetzt. Oder FUnktionen 
'BeginSendString', 'SendString', 'EndSendString', wenn da irgendwelche 
Spezialsachen vor und nach dem Senden eines Strings notwendig sind.

[Pal .. (pal)]

Hallo Karl Heinz,

danke für deine Antwort. Leider kann ich die Strings nicht so 
auseinander ziehen.

Du hast mich dabei allerdings auf die Idee gebracht, dass ich den Code 
folgendermaßen änderen könnte:

static const char ClearJoyArea[] = "#RL200,30,350,120,#UI255,35,";

strncat(ClearJoyArea, ",2", 2 );
sendeDaten(ClearJoyArea);

Bringt das etwas? Soweit ich weiß, benötigt "strncat" auch etwas 
Speicher.
Außerdem bekomme ich dann noch Fehlermeldungen.

[Karl H. (kbuchegg)]

Zur Not macht man sich dann auch schon mal Spezialfunktionen, die einem
das Senden eines Strings in mehreren Happen erlauben. Die "#MN" Sache
müsste man zb ein wenig anders angehen. Man kann ja zb eine
Spezialfunktion machen, die immer zuerst mal "#MN" sendet und dann den
übergebenen String noch hinten nach absetzt. Oder FUnktionen
'BeginSendString', 'SendString', 'EndSendString', wenn da irgendwelche
Spezialsachen vor und nach dem Senden eines Strings notwendig sind.

[Pal .. (pal)]

Ich bin beeindruckt was etwas hin und her kopieren von strings 
bewirkt....

Im Anhang sind noch mal die Änderungen.

Hast du sonst noch eine Idee für Optimierungsbedarf?


Zur Info....

Device: atmega16

Program:    5028 bytes (30.7% Full)
(.text + .data + .bootloader)

Data:        305 bytes (29.8% Full)
(.data + .bss + .noinit)

EEPROM:        2 bytes (0.4% Full)
(.eeprom)

568 Byte geschrumpft....


Nachtrag.....
Die Funktion "sendeMaschAuswToDispl" werde ich natürlich auch noch 
ändern...

[Karl H. (kbuchegg)]

Der Teil hier

      strncat(hoehe, hoeheEnd, 2);
      sendeDaten((uint8_t*)hoehe);                    // Balkenhöhe ans Display senden

      sendeDaten(schriftDrehz);                      // Schriftart für Drehzahl senden
      strncat(drehz, &aktDrehzZei[0], 1);
      strncat(drehz, drehzEnd, 2);
      sendeDaten((uint8_t*)drehz);                    // Drehzahl ans Display senden

ist (was ich so gesehen habe) in allen Fällen immer gleich.
Heißer Kandidat für eine Funktion.

[Karl H. (kbuchegg)]

Das hier

    const uint8_t taste16[]  = {ESC, U, I, _2_, _3_, _0_, KOMMA, _1_, _2_, _0_, KOMMA, _7_,      KOMMA,
                  ESC, U, I, _2_, _2_, _0_, KOMMA, _2_, _3_, _0_, KOMMA, _1_, _2_, KOMMA, LF};    // Taste 1 betätigt, Taste 6 betätigt

also so wie du die Initialisierung machst, ist übertrieben.
Da leidet die Lesbarkeit druntern. UNd die geht prinzipiell immer vor.

[Frank M. (ukw) (Moderator)]

pal ... schrieb:
>

> static const char ClearJoyArea[] = "#RL200,30,350,120,#UI255,35,";
> 
> strncat(ClearJoyArea, ",2", 2 );
> sendeDaten(ClearJoyArea);
>

Das ist böse. Du allokierst einen Speicher mit N Bytes und hängst dann 
noch 2 Bytes dran. Das ergibt nicht nur einen satten Overflow, sondern 
ClearJoyArea ist danach wegen str_n_cat auch nicht mehr terminiert. 
Abgesehen davon passt das "const" dazu auch nicht mehr.

Wenn, dann brauchst Du einen Zwischenspeicher:

char Buf[MINDESTENS_SOVIEL_WIE_DER_STRING_INSG_LANG_WERDEN_KANN + 1];

strcpy (Buf, ClearJoyArea);
strcat(ClearJoyArea, ",2");
sendeDaten(Buf);

[Frank M. (ukw) (Moderator)]

Karl Heinz Buchegger schrieb:
> Der Teil hier
>
>

> 
>       strncat(hoehe, hoeheEnd, 2);
>       strncat(drehz, &aktDrehzZei[0], 1);
>       strncat(drehz, drehzEnd, 2);
>

>
> ist (was ich so gesehen habe) in allen Fällen immer gleich.
> Heißer Kandidat für eine Funktion.

str_n_cat() ist hierfür nicht geeignet. Er zerschießt sich damit 
sämtliche String-Terminierungen. Wenn, dann strcat() ohne Längenangabe.

[Karl H. (kbuchegg)]

pal ... schrieb:

> danke für deine Antwort. Leider kann ich die Strings nicht so
> auseinander ziehen.

Anderer Vorschlag.

Es ist ja nicht in Stein gemeisselt, dass du nur eine Send Funktion für 
Strings haben darfst und die nur 1 String ausgeben darf.

Es spricht ja nichts dagegen, dass du dir zb eine SendeString2 FUnktion 
machst, die 2 Strings bekommt und die beiden hintereinander ausgibt.

uint8_t sendeDaten2(const char* data1, const char* data2)
{
  uint8_t bcc;
  uint8_t retNak = 0;
  uint8_t len = 0;
  uint8_t error = 0;


  for(error = 0; error < 3; error++) {

    // Daten senden (schreiben)
    i2c_start_wait(DISPLAY_WRITE);                // setze Adresse vom Display und Schreib-Modus

    i2c_write(DC1);                        // sende DC1

    len = 0;
    if( data1 )
      len += strlen(data1);
    if( data2 )
      len += strlen(data2);

    i2c_write(len);                        // sende Datenlänge

    bcc = DC1 + len;                      // bererchne Prüf-(Teil)Summe aus allen Bytes

    if( data1 ) {
      while( *data1 ) {
        i2c_write( *data1 );
        bcc = bcc + *data1;
        data1++;
      }
    }

    if( data2 ) {
      while( *data2 ) {
        i2c_write( *data2 );
        bcc = bcc + *data2;
        data2++;
      }
    }

    i2c_write(bcc);                        // sende Prüf-Summe

    // Quittierung vom Display holen (lesen)
    i2c_rep_start(DISPLAY_READ);                // setze Adresse vom Display und Lese-Modus
    _delay_us(TIME_TO_WAIT);                   // vor jedem lesen warten
    retNak = i2c_readNak();                    // Quittierung vom Display lesen
    i2c_stop();                          // Übertragung beenden


    if(retNak == ACK) {                      // Übertragung erfolgreich - dann Schleife abbrechen
      error = 3;
    } else {
      _delay_us(100);                      // kurze Pause
    }

  }

  return retNak;
}

Und schon hast du eine Funktion, die du so aufrufen kannst

static const char ClearJoyArea[] = "#RL200,30,350,120,#UI255,35,";

    ...
    SendeDaten2( ClearJoyArea, ",2" );
    ...

und die dann das richtige macht, ganz ohne umkopieren oder sonstigen 
Aufwand (und nicht zu vergessen: potentielle Fehlerquellen!)
Und was für 2 geht, geht auch für 3


(und die sendDaten sieht dann so aus

uint8_t sendeDaten(const char* data)
{
  sendeDaten2( data, NULL );
}

)

Es gibt (fast) immer Mittel und Wege etwas zu vereinfachen. Und wenn man 
sich seine Werkzeuge entsprechend zurechtschnitzen muss, dann muss man 
sich eben manchmal seine Werkzeuge zurechtschnitzen, um an anderen 
Stellen einfacher arbeiten zu können. Es gibt immer 2 Ansatzpunkte:
Entweder beim Aufrufer einer Funktion die Dinge vereinfachen
Oder die Funktionen selbst so zu gestalten, dass sich die Aufrufe 
vereinfachen. Manchmal ist es in Summe simpler, ein wenig Aufwand in die 
Funktionen zu stecken, wenn dadurch quer durch den restlichen Code ein 
Haufen Code wegfällt.

[Pal .. (pal)]

hi,

SORRY!!!!
Ich hab eben den falschen Qullcode gepostet. Sorry.... Der alte 
Quellcode war nicht wirklich gut!

Jetzt ist aber im Anhang der neue...

Sorry für eure Anstrengungen - danke

[Frank M. (ukw) (Moderator)]

pal ... schrieb:
> Ich hab eben den falschen Qullcode gepostet. Sorry.... Der alte
> Quellcode war nicht wirklich gut!

Der ist auch nicht besser, z.B. hier:

  char maschEro[] = "#MN15";
  strncat(maschEro, "1,", 2);

2 Fehler:

maschEro ist ein String mit Speicherlänge 5 + 1 = 6.
strncat() hängt da ohne Terminierung noch 2 Bytes dran.

Daraus ergeben sich 2 Bugs, die in Deinem Programm zu Fehlverhalten 
führen können:

1. Der nachfolgende String maschBin wird überschrieben wegen
   Bufferoverflow.

2. Der String maschEro ist nicht mehr mit '\0' terminiert.

Du brauchst für Deine Aktionen einen Zwischenpuffer, siehe mein Posting

  Beitrag "Re: Pointer suche"

und statt strncat() musst Du strcat() benutzen, damit die Terminierung 
erhalten bleibt.

Warum ignorierst Du meine bisherigen Angaben zu Deinem Code?

[Pal .. (pal)]

Hallo Frank.

sorry, das wollte ich nicht! War eine Kurzschlussreaktion auf meinen 
falsch geposteten Code.

Du hast natürlich recht! Werde den Code natürlich ändern.

Aber danke schon einmal!

Und sorry noch mal....

[Karl H. (kbuchegg)]

pal ... schrieb:
> Hallo Frank.
>
> sorry, das wollte ich nicht! War eine Kurzschlussreaktion auf meinen
> falsch geposteten Code.
>
> Du hast natürlich recht! Werde den Code natürlich ändern.

Zieh auch mal ins Kalkül die Strings überhaupt nicht beim Aufrufer 
zusammenzusetzen. Das muss nicht unbedingt sein, und damit hast du dann 
schon eine Fehlerquelle weniger.

Wenn ich deinen Code so überfliege, dann gibt es viele Stellen die von 
einer Funktion profitieren würden, die einen String ausgeben können, den 
man der Funktion in 2 Teilen vorsetzt.

Beitrag "Re: Pointer suche"

[Pal .. (pal)]

Ja, ich werde mich noch mal an mein Programm setzen und es dann 
natürlich posten.

Aber euch beiden schon mal einen Dank!

[Karl H. (kbuchegg)]

Nur um das klar zu stellen:
Nicht das Frank hier nicht recht hat. Mit Strings umgehen zu können, und 
zwar korrekt umgehen zu können, muss jeder C-Programmierer. Da führt auf 
lange Sicht kein Weg daran vorbei.
Nur um Moment kannst du dich nochmal davor drücken, wenn du dir ein paar 
Hilfsfunktionen baust, weil String Manipulation im Moment noch nicht 
unabwendbar notwendig ist. Ganz im Gegenteil gibt es eine Möglichkeit, 
wie man das Programm sogar um einiges einfacher kriegt, wenn man keine 
Stringmanipulation macht.

[Pal .. (pal)]

Ja klar, das wollte ich auch damit nicht so sagen. :-)
Ich werde natürlich versuchen an geeigneter Stelle Franks Informationen 
zu brücksichtigen

[Frank M. (ukw) (Moderator)]

pal ... schrieb:
> Ja klar, das wollte ich auch damit nicht so sagen. :-)
> Ich werde natürlich versuchen an geeigneter Stelle Franks Informationen
> zu brücksichtigen

Kombiniere meine Tipps besser direkt mit denen von Karl Heinz. Er hat 
nämlich völlig recht: es ist Unsinn, die Strings an N Stellen immer 
wieder zusammenzusetzen. Das kostet nur unnötig viel Code.

Genau dafür gibt es Funktionen, in denen man sich immer wiederholende 
Vorgänge zusammenfasst. Also schreib eine Funktion, die direkt 2 Strings 
bekommt, diese beiden zu einem zusammensetzt und dann die eigentliche 
Funktion aufruft, die Du benutzen willst.

[Pal .. (pal)]

Hallo,

danke noch mal für eure Kritik und guten Ratschläge. Ich denke, dass ich 
jetzt alles entsprechend umgesetzt habe.

Allerdings denke ich, dass dort immer noch Optimierungsbedarf besteht. 
Denn betrachtet man sich beeispielsweise die Funktion 
"sendeDrehzToLeistmod" braucht diese enorm viel Speicherplatz. Ich 
denke, dass es am casten liegt - hätte aber noch keine Idee es anders zu 
lösen

Danke für eure Hilfe


mfg pal

[Karl H. (kbuchegg)]

pal ... schrieb:

> "sendeDrehzToLeistmod" braucht diese enorm viel Speicherplatz. Ich
> denke, dass es am casten liegt - hätte aber noch keine Idee es anders zu
> lösen

Solange du da float Arithmetik benutzt, darfst du dich über den 
Speicherverbrauch nicht wundern.

Brauchst du überhaupt float? Wozu?
Da kommt ein Wert raus, den du ins PWM Register schiebst. Wer braucht da 
Nachkommastellen?

Wenn man die Berechnung
    drehzRed = minDrehz - (((minDrehz - maxDrehz) / 100.0) * aktDrehz);
ein wenig umstellt, kann man das ohne Genauigkeitsverlust auch mit 
INteger-Arithmetik berechnen (Faustregel: Divisionen will man immer 
soweit wie möglich 'nach rechts' schieben, d.h. so spät wie möglich 
machen)

Und fällt dann die Floating Point Arithmetik weg, dann fällt auch der 
dazu notwendige Code weg. Und das ist nicht gerade wenig.

[Karl H. (kbuchegg)]

uint8_t sendeDaten3Str(const char data1[], const char data2[], const char data3[]) {
  uint8_t i = 0;
  uint8_t bcc;
  uint8_t retNak = 0;
  uint8_t len1 = 0;
  uint8_t len2 = 0;
  uint8_t len3 = 0;
  uint8_t error = 0;


  for(error = 0; error < 3; error++) {

    // Daten senden (schreiben)
    i2c_start_wait(DISPLAY_WRITE);                // setze Adresse vom Display und Schreib-Modus

    i2c_write(DC1);                        // sende DC1

    len1 = strlen(data1);
    len2 = strlen(data2);
    len3 = strlen(data3);
    i2c_write(len1 + len2 + len3);                // sende Datenlänge

AUtsch.
Ich hab extra in der Vorlage hier eine Absicherung gemacht. Die war 
nicht einfach so aus einer Lust und Laune heraus. Du DARFST strlen NICHT 
mit einem NULL Pointer aufrufen! Du tust es aber

uint8_t sendeDaten(const char data[]) {
  return sendeDaten3Str(data, NULL, NULL);
}

Gewöhn dir an: Wann immer ein Pointer im Spiel ist, dann muss die erste 
Frage lauten - kann der NULL sein?
Und im Zweifelsfall ist immer davon auszugehen, dass der Pointer NULL 
sein kann.

[Frank M. (ukw) (Moderator)]

pal ... schrieb:
> Denn betrachtet man sich beeispielsweise die Funktion
> "sendeDrehzToLeistmod" braucht diese enorm viel Speicherplatz.

Du musst von den floats weg und alles in Integern machen.

Mache erst die Multiplikation mit aktDrehz und dann erst die Division 
durch 100. Alles mit Integern. Dann kommst Du für msg->bytes.pwmVal auf 
denselben Wert.

Muss sendeDrehzToLeistmod() überhaupt einen float zurückliefern? Ich 
glaube, der PWM-Wert reicht.

Gruß,

Frank

EDIT: Karl Heinz war schneller :-)

[Karl H. (kbuchegg)]

> Gewöhn dir an: Wann immer ein Pointer im Spiel ist, dann muss die erste
> Frage lauten - kann der NULL sein?
> Und im Zweifelsfall ist immer davon auszugehen, dass der Pointer NULL
> sein kann.

PS: Hier in der Funktion #IST# data1 ein Pointer, auch wenn das data[] 
in der Argumentliste etwas anderes suggeriert. Zwischen

uint8_t sendeDaten3Str(const char data1[], const char data2[], const char data3[])

und

uint8_t sendeDaten3Str(const char * data1, const char * data2, const char * data3)

besteht kein Unterschied. Die [] Schreibweise ist nur syntaktischer 
Zucker. Tatsächlich #SIND# das Pointer, die in einer anderen Syntax 
versteckt wurden. Und da das durch die [] Schreibweise mehr schlecht als 
recht verschleiert wird, wird meistens von dieser Schreibweise 
abgeraten.

Gewöhn dich an Pointeroperationen. Je früher desto besser. Es gibt 
keinen Grund sich (auf dieser Ebene) davor zu fürchten.

[Pal .. (pal)]

hallo,

ok, ich gebe mich wieder ran und versuch es umzusetzen.
Sorry Karl Heinz, mir ist nicht aufgefallen das du den NULL-Pointer 
überprüft hast.

Ich habe nur auf die while-Schleifen verzichtet. Arbeite lieber mit 
for's.

[Karl H. (kbuchegg)]

pal ... schrieb:

> Ich habe nur auf die while-Schleifen verzichtet. Arbeite lieber mit
> for's.

Ist zum jetzigen Zeitpunkt noch ok. Aber irgendwann wirst du den Schritt 
machen müssen. Auf dieser Ebene sind Pointer noch leicht zu beherrschen 
und Stringoperationen laufen sowieso immer noch dem gleichen Muster ab.

[Pal .. (pal)]

Hallo,

ich habe mir noch mal eure Ratschläge zu Herzen genommen....
Im Anhang habe ich noch mal die geänderten Quellen. Ich hoffe ihr habt 
wieder gute Kritik. DANKE!!!


Nachtrag....
Die Statistik zeigt nun folgendes....

Device: atmega16

Program:    6742 bytes (41.1% Full)
(.text + .data + .bootloader)

Data:        284 bytes (27.7% Full)
(.data + .bss + .noinit)

EEPROM:        2 bytes (0.4% Full)
(.eeprom)

[Karl H. (kbuchegg)]

Programmiertaktik:

Schau deine Header Files durch.
Welche der #define werden nur in EINEM C-File benutzt und werden auch in 
Zukunft nur in diesem einen C-File benutzt werden?

Diese #define gehören nicht ins Header File, sondern ins jeweilige 
C-File. Du musst das Header File aus Sicht eines 'Verwenders' sehen. Im 
Header File sind nur die Dinge, die einen Verwender interessieren, die 
ER benötigt. Werden #define nur in einem Code File verwendet, dann 
sollen die auch dort bleiben.

ZB die

#define JOY_NULL          0            // Joystick in Nullposition
#define JOY_VOR            2            // Joystickbewegung nach vor
#define JOY_ZURUECK          3            // Joystickbewegung nach zurück
#define JOY_RECHTS          4            // Joystickbewegung nach rechts
#define JOY_LINKS          5            // Joystickbewegung nach links

#define JOY_TASTE_UNBET        0            // Joystick - keine Taste betätigt
#define JOY_TASTE_1          1            // Joystick Taste 1 betätigt
#define JOY_TASTE_2          2            // Joystick Taste 2 betätigt
#define JOY_TASTE_3          3            // Joystick Taste 3 betätigt
#define JOY_TASTE_4          4            // Joystick Taste 4 betätigt
#define JOY_TASTE_5          5            // Joystick Taste 5 betätigt
#define JOY_TASTE_6          6            // Joystick Taste 6 betätigt
#define JOY_TASTE_16        16            // Joystick Taste 1 und 6 betätigt
#define JOY_TASTE_26        26            // Joystick Taste 2 und 6 betätigt
#define JOY_TASTE_36        36            // Joystick Taste 3 und 6 betätigt
#define JOY_TASTE_46        46            // Joystick Taste 4 und 6 betätigt
#define JOY_TASTE_56        56            // Joystick Taste 5 und 6 betätigt

Interessieren die ausserhalb display.c irgend jemanden? Muss jemand 
wissen, dass
a) es diese Konstanten gibt?
b) welche Werte sie haben?

Ich denke nicht. Ergo gehören diese Konstanten nicht ins Header File, 
sondern nach display.c

(und so gibts noch andere #define, die zumindest zweifelhaft sind).

Ansonsten: Schau selber deinen Code durch! Zb hier

uint8_t leseDaten() {
  char dataS[] = {'S'};

Wozu brauchst du da ein Array?


Schreibweisen:
mehrere Leerzeilen hintereinander erhöhen nicht die Übersicht, sie 
ziehen den Code nur in die Länge. Wenn du im Code einen optischen 
Schnitt machen willst, dass hier eine neue Funktion anfängt, dann reicht 
1 Leerzeile durchaus. Wenn dir etwas so wichtig war, dass du es mit 
mehreren Leerzeilen abtrennen musst, dann ist es wohl auch wichtig 
genug, dass es sich einen Kommentar verdient hat.

[Karl H. (kbuchegg)]

zb hier

static uint8_t  oldDirection = 255;                        // Alte Joystickrichtung

...

void sendeJoyBewToDispl(const struct JOYSTICK* j) {
  const char joyArea[] = "#RL200,30,350,120,#UI255,35,";

  if (j->direction == ypos) {
    if(oldDirection != JOY_VOR) {

oldDirection hat also ganz offensichtlich etwas mit den #define

#define JOY_NULL          0            // Joystick in Nullposition
#define JOY_VOR            2            // Joystickbewegung nach vor
#define JOY_ZURUECK          3            // Joystickbewegung nach zurück
#define JOY_RECHTS          4            // Joystickbewegung nach rechts
#define JOY_LINKS          5            // Joystickbewegung nach links
...

zu tun.
Nur: Es gibt kein #define, welches einer 255 einen Wert geben würde. 255 
ist für diese Variable gar nicht erlaubt. Warum initialisierst du dann 
diese Variable damit? Nimm einen Wert der erlaubt ist und am besten 
benutzt du da gleich einen deiner #define-ten Werte dafür

static uint8_t  oldDirection = JOY_NULL;

Schau deine Kommentare durch. Welche sind 0-Kommentare? Das sind 
Kommentare, die genau das gleiche aussagen wie das, was ohnehin schon im 
Code steht. Erzählt mir ein Kommentar etwas neues, was ich nicht im Code 
sehe? Wenn nicht, dann weg mit dem Kommentar - denn im besten Fall 
erfahre ich durch Lesen des Kommentars nichts neues, im schlimmsten Fall 
ist er falsch.

Das zb

        _delay_us(100);                      // kurze Pause

ist ein 0-Kommentar. Das mit _delay_us kurz gewartet wird, sehe ich im 
Code auch. Was mich aber interessieren würde: Warum gerade 100us? Steckt 
da was dahinter oder ist das relativ willkürlich? Nur leider: Genau das 
erzählt mir der Kommentar nicht. Der erzählt mir nur Sachen, die ich 
beim Lesen des Codes auch sehe.
Das hier

    i2c_write(DC1);                        // sende DC1

ist ein 0-Kommentar.

Generell ist es immer verdächtig, wenn man versucht ist, rechts an so 
gut wie alle Anweisungen einen Kommentar zu setzen. Das sind meistens 
0-Kommentare. In deiner send Funktion wäre zb in einem Kommentar zu 
dokumentieren, was da eigentlich genau in welcher Reihenfolge gesendet 
wird. Aber als Blockkommentar vor dem ganzen Code!

...
//   Sendet einen String an das Gerät
//   Gesendet wird
//     Kommando (== DC1)
//     ANzahl der Zeichen
//     die Zeichen selber
//     Prüfsumme
//
// DIe Prüfsumme errechnet sich aus der arithmetischen Summe aller Bytes
// (inklusive der Länge und dem Kommandobyte), wobei nur das Lower-Byte
// gesendet wird
...

Das zb. ist ein Kommentar, der mich weiterbringt. WEnn ich den COde 
lese, dann kann ich die Information aus dem Kommentar verwenden um den 
Programmfluss zu verfolgen. Die ganzen Einzelkommentare brauch ich dann 
gar nicht mehr. Denn die erzählen mir erstens sowieso nicht viel und 
zweitens ist die Absicht im Code damit nicht erkennbar (und drittens 
sind die des öfteren tatsächlich falsch)
Generell: Kommentiere nicht das WIE, sondern das WARUM
WIE eine Operation durchgeführt wird, steht im Code. Der Kommentar muss 
mir Info liefern WARUM etwas gemacht wird.

[Pal .. (pal)]

ja mit den Kommentaren geben ich dir auch vollkommen recht. Die sind 
teilweise nutzlos. Mit deinem Beitrag hast du mir aber schon eine gute 
Vorlage geliefet. Die Kommentare oberhalb der Funktion wollte ich 
eigentlich erst später machen. Aber du hast schon recht - man sollte 
solche Dinge eigentlich nicht aufschieben.

Was würdest du noch zum Code sagen - soweit ok?

Mit der Statistik liege ich jetzt bei 27,7%.


Ich habe jetzt allerdings noch manchmal das Problem, dass wenn ich 
Controller und Display starte (d.h. Spannung ein schalte) bekomme ich 
nicht immer eine Aktion am Display angezeigt. Sobald ich das Display 
dann wieder resette funktioniert es wieder. Hast du eine Idee was das 
sein kann?

Werde mich aber jetzt doch an die Kommentare geben :-)

[Karl H. (kbuchegg)]

Die Funktion

void sendeMaschAuswToDispl(uint8_t id) {
  const char maschEro[] = "#MN15";
  const char maschBin[] = "#MN16";


  if(id != oldMachineSelection) {

    switch(id) {
      case ID_HEIM:
        sendeDrehzToDispl(ID_BETRIEB_READY);                        // Drehzahl nochmal senden
        break;
      case ID_EINS_CANEPRUNER_ERO:
        setEEAndId(id);
        sendeDaten2Str(maschEro, "1,");                            // "#MN151,"
        break;
      case ID_EINS_LAUBHEFTER_ERO:
        setEEAndId(id);
        sendeDaten2Str(maschEro, "2,");
        break;
      case ID_EINS_LAUBSCHNEIDER_ERO:
        setEEAndId(id);
        sendeDaten2Str(maschEro, "3,");
        break;
      case ID_EINS_LAUBSAUGER_ERO:
        setEEAndId(id);
        sendeDaten2Str(maschEro, "4,");
        break;
      case ID_EINS_ENTBLAETTERER_ERO:
        setEEAndId(id);
        sendeDaten2Str(maschEro, "5,");
        break;
      case ID_ZWEIS_LAUBSCHNEIDER_ERO:
        setEEAndId(id);
        sendeDaten2Str(maschEro, "6,");
        break;
      case ID_ZWEIS_LAUBSAUGER_ERO:
        setEEAndId(id);
        sendeDaten2Str(maschEro, "7,");
        break;
      case ID_ZWEIS_ENTBLAETTERER_ERO:
        setEEAndId(id);
        sendeDaten2Str(maschEro, "8,");
        break;
      case ID_EINS_VORSCHNEIDER_BIN:
        setEEAndId(id);
        sendeDaten2Str(maschBin, "0,");                            // "#MN160,"
        break;
      case ID_EINS_ENTLAUBER_BIN:
        setEEAndId(id);
        sendeDaten2Str(maschBin, "1,");
        break;
      case ID_EINS_LAUBSCHNEIDER_BIN:
        setEEAndId(id);
        sendeDaten2Str(maschBin, "2,");
        break;
      case ID_ZWEIS_ENTLAUBER_BIN:
        setEEAndId(id);
        sendeDaten2Str(maschBin, "3,");
        break;
      case ID_ZWEIS_LAUBSCHNEIDER_BIN:
        setEEAndId(id);
        sendeDaten2Str(maschBin, "4,");
        break;
    }
  }
}

sollte überarbeitet werden. Die ist für die Funktionalität zu lang und 
zu unübersichtlich. Der Aufruf von setEEAndId(id); erfolgt in allen 
Fällen und kann daher aus dem ganzen switch-case raus. (Ich hätte ihn 
fast übersehen, als ich nachverfolgen wollte, wie und wo du wieder ins 
EEPROM schreibst)

In dem Fall

  if(id != oldMachineSelection) {

    switch(id) {
      case ID_HEIM:
        sendeDrehzToDispl(ID_BETRIEB_READY);                        // Drehzahl nochmal senden
        break;
...

wird oldMachineSelection NICHT auf id gesetzt und auch nicht ins EEPROM 
geschrieben. Ist das so gewollt?

[Pal .. (pal)]

ja das gewollt, dass bei der ID_HEIM nicht ins EEPROM geschrieben wird.

Mir ist auch aufgefallen, dass die Funktion deutlich zu lange und 
unübersichtlich ist. Mir ist aber nicht ganz klar wie ich diese 
vereinfachen soll

[Pal .. (pal)]

In der Funktion "void sendeMaschAuswToDispl(uint8_t id)" kann man 
"setEEAndId(id)" nicht aus dem case zeihen, weil noch andere IDs 
auftauchen könnten und diese nicht ins EEPROM geschrieben werden sollen

[Karl H. (kbuchegg)]

Ich würde die FUnktion so schreiben.
(Achtung: Ich handle mir damit eine Abhängigkeit zu den Zahlenwerten im 
#define ein. Da du die aber im Nachhinein sowieso nur schlecht ändern 
kannst, würde ich das akzeptieren)

void SendNumString( const char* str, uint8_t num )
{
  char numStr[] = "0,";

  numStr[0] += num;
  sendeDaten2Str( str, numStr );
}

void sendeMaschAuswToDispl(uint8_t id) {

  const char maschEro[] = "#MN15";
  const char maschBin[] = "#MN16";

  if( id == oldMachineSelection)
    return;

  if( id == ID_HEIM )
    sendeDrehzToDispl(ID_BETRIEB_READY);

  else if( id >= ID_EINS_CANEPRUNER_ERO && id <= ID_EINS_ENTBLAETTERER_ERO ) {
    setEEAndId(id);
    sendeDaten2Str( maschEro, id - ID_EINS_CANEPRUNER_ERO + 1 );
  }

  else if( id >= ID_ZWEIS_LAUBSCHNEIDER_ERO && id <= ID_ZWEIS_ENTBLAETTERER_ERO ) {
    setEEAndId(id);
    sendeDaten2Str( maschEro, id - ID_ZWEIS_LAUBSCHNEIDER_ERO + 6 );
  }

  else if( id >= ID_EINS_VORSCHNEIDER_BIN && id <= ID_EINS_LAUBSCHNEIDER_BIN) {
    setEEAndId(id);
    sendeDaten2Str( maschBin, id - ID_EINS_VORSCHNEIDER_BIN + 1 );
  }

  else if( id >= ID_ZWEIS_ENTLAUBER_BIN && id <= ID_ZWEIS_LAUBSCHNEIDER_BIN) {
    setEEAndId(id);
    sendeDaten2Str( maschBin, id - ID_ZWEIS_ENTLAUBER_BIN + 3 );
  }
}

Da du deine Werte nicht fortlaufend vergeben hast, gibt es leider keine 
Möglichkeit, das alles noch mehr Datengesteuert zu machen, so dass man 
den Code noch kompakter kriegt.

[Karl H. (kbuchegg)]

Karl Heinz Buchegger schrieb:

> Da du deine Werte nicht fortlaufend vergeben hast, gibt es leider keine
> Möglichkeit, das alles noch mehr Datengesteuert zu machen, so dass man
> den Code noch kompakter kriegt.

Egal.
Probieren wir einfach mal, wo uns das hinführt

struct idSel
{
  uint8_t id_;
  uint8_t num_;
}

// Übersetzungstabelle für Error-Ids zu Codes die zum Display
// zu schicken sind
struct idSel idSelError[] =
{ 
  { ID_EINS_CANEPRUNER_ERO,     1 ),
  { ID_EINS_LAUBHEFTER_ERO,     2 ),
  { ID_EINS_LAUBSCHNEIDER_ERO,  3 ),
  { ID_EINS_LAUBSAUGER_ERO,     4 ),
  { ID_EINS_ENTBLAETTERER_ERO,  5 ),
  { ID_ZWEIS_LAUBSCHNEIDER_ERO, 6 ),
  { ID_ZWEIS_LAUBSAUGER_ERO,    7 ),
  { ID_ZWEIS_ENTBLAETTERER_ERO, 8 ),
};

struct idSel idSelBin [] =
{
  { ID_EINS_VORSCHNEIDER_BIN,   1 ),
  { ID_EINS_ENTLAUBER_BIN,      2 ),
  { ID_EINS_LAUBSCHNEIDER_BIN,  3 ),
  { ID_ZWEIS_ENTLAUBER_BIN,     4 ),
  { ID_ZWEIS_LAUBSCHNEIDER_BIN, 5 ),
};

#define ARRAY_SIZE(x)  (sizeof(x)/sizeof(*x))

void sendeMaschAuswToDispl(uint8_t id) {

  const char maschEro[] = "#MN15";
  const char maschBin[] = "#MN16";

  if( id == oldMachineSelection)
    return;

  if( id == ID_HEIM ) {
    sendeDrehzToDispl(ID_BETRIEB_READY);
    return;
  }

  // ist es ein Error Code?
  for( i = 0; i < ARRAY_SIZE( idSelError ); i++ ) {
    if( idSelError[i].id_ == id ) {
      setEEAndId(id);
      SendNumString( maschEro, idSelError[i].num_ );
      return;
    }
  }

  // Nope, kein Error Code. Bin Code?
  for( i = 0; i < ARRAY_SIZE( idSelBin ); i++ ) {
    if( idSelBin [i].id_ == id ) {
      setEEAndId(id);
      SendNumString( maschEro, idSelBin [i].num_ );
      return;
    }
  }
}

Ist nicht wirklich kürzer. Vorteil ist höchstens, dass man die Codes 
erweitern kann indem man einfach zum jeweiligen Array zufügt.

[Karl H. (kbuchegg)]

Karl Heinz Buchegger schrieb:

> (Achtung: Ich handle mir damit eine Abhängigkeit zu den Zahlenwerten im
> #define ein. Da du die aber im Nachhinein sowieso nur schlecht ändern
> kannst, würde ich das akzeptieren)
>

Da hab ich aber ordentlich geschlampt und Fehler eingebaut :-)

void sendeMaschAuswToDispl(uint8_t id) {
 
  const char maschEro[] = "#MN15";
  const char maschBin[] = "#MN16";
 
  if( id == oldMachineSelection)
    return;
 
  if( id == ID_HEIM )
    sendeDrehzToDispl(ID_BETRIEB_READY);
 
  else if( id >= ID_EINS_CANEPRUNER_ERO && id <= ID_EINS_ENTBLAETTERER_ERO ) {
    setEEAndId(id);
    SendNumString( maschEro, id - ID_EINS_CANEPRUNER_ERO + 1 );
  }
 
  else if( id >= ID_ZWEIS_LAUBSCHNEIDER_ERO && id <= ID_ZWEIS_ENTBLAETTERER_ERO ) {
    setEEAndId(id);
    SendNumString( maschEro, id - ID_ZWEIS_LAUBSCHNEIDER_ERO + 6 );
  }
 
  else if( id >= ID_EINS_VORSCHNEIDER_BIN && id <= ID_EINS_LAUBSCHNEIDER_BIN) {
    setEEAndId(id);
    SendNumString( maschBin, id - ID_EINS_VORSCHNEIDER_BIN + 1 );
  }
 
  else if( id >= ID_ZWEIS_ENTLAUBER_BIN && id <= ID_ZWEIS_LAUBSCHNEIDER_BIN) {
    setEEAndId(id);
    SendNumString( maschBin, id - ID_ZWEIS_ENTLAUBER_BIN + 4 );
  }
}

Ich denke, ich würde die Version nehmen. Dadurch dass zwischen Error-Id 
und Bin-Id unterschieden wird, wird die tabellengesteuerte Variante 
nicht wirklich kürzer oder einfacher. Und auf die Variante, diese Info 
auch noch mit in die Tabelle aufzunehmen kann ich mich (noch) nicht 
erwärmen.

So wie hier, mit der Erkennung der jeweiligen 'Gruppe' und 
zusammenstoppeln des zu sendenden Codes aus der Gruppenposition, das 
scheint mir ein guter Kompromiss zu sein. Vielleicht noch eine kleine 
Prise Makro-Magie, aber mehr würde ich nicht tun.

[Pal .. (pal)]

So eine ähnlich Idee hatte ich auch schon...

Noch habe ich die Möglichkeit die IDs näher zusammen zurücken. Ich denke 
damit können wir den Code noch vereinfachen.

Ich gebe mich einfach mal dran und poste ihn schließlich wieder....

Danke für deine/eure Hilfe!!!

[Karl H. (kbuchegg)]

pal ... schrieb:
> So eine ähnlich Idee hatte ich auch schon...
>
> Noch habe ich die Möglichkeit die IDs näher zusammen zurücken. Ich denke
> damit können wir den Code noch vereinfachen.

Wichtig ist vor allen Dingen, dass sie fortlaufend sind!
Alle xxx_ERO Codes mit aufsteigender Nummerierung. Alle xxx_BIN Codes 
mit aufsteigender Nummerierung.

Dann geht:

void sendeMaschAuswToDispl(uint8_t id) {
 
  const char maschEro[] = "#MN15";
  const char maschBin[] = "#MN16";
 
  if( id == oldMachineSelection)
    return;
 
  if( id == ID_HEIM )
    sendeDrehzToDispl(ID_BETRIEB_READY);
 
  // Error Code?
  else if( id >= ID_EINS_CANEPRUNER_ERO && id <= ID_ZWEIS_ENTBLAETTERER_ERO ) {
    setEEAndId(id);
    SendNumString( maschEro, id - ID_EINS_CANEPRUNER_ERO + 1 );
  }
 
  // Bin Code?
  else if( id >= ID_EINS_VORSCHNEIDER_BIN && id <= ID_ZWEIS_LAUBSCHNEIDER_BIN ) {
    setEEAndId(id);
    SendNumString( maschBin, id - ID_EINS_VORSCHNEIDER_BIN + 1 );
  }
}

und besser wirds nicht mehr.

[Pal .. (pal)]

ja, so war auch meine Idee.

Bin gerade kräftig am umschreiben. :-)

[Pal .. (pal)]

Ich denke so könnte es funktionieren

...header....
#define ID_EINS_CANEPRUNER_ERO    30
#define ID_EINS_LAUBHEFTER_ERO    31
#define ID_EINS_LAUBSCHNEIDER_ERO  32
#define ID_EINS_LAUBSAUGER_ERO    33
#define ID_EINS_ENTBLAETTERER_ERO  34
#define ID_ZWEIS_LAUBSCHNEIDER_ERO  35
#define ID_ZWEIS_LAUBSAUGER_ERO    36
#define ID_ZWEIS_ENTBLAETTERER_ERO  37
#define ID_EINS_VORSCHNEIDER_BIN  38
#define ID_EINS_ENTLAUBER_BIN    39
#define ID_EINS_LAUBSCHNEIDER_BIN  40
#define ID_ZWEIS_ENTLAUBER_BIN    41
#define ID_ZWEIS_LAUBSCHNEIDER_BIN  42

...funktion...
void sendeMaschAuswToDispl(uint8_t id) {
   char buffer[3];
   const char masch[] = "#RL5,5,150,160,#UI10,10,";

   if(id == oldMachineSelection) {
      return;
   }

   if(id == ID_HEIM ) {
      sendeDrehzToDispl(ID_BETRIEB_READY);

   } else if( id >= ID_EINS_CANEPRUNER_ERO && id <= ID_ZWEIS_LAUBSCHNEIDER_BIN ) {
      setEEAndId(id);
      sprintf(buffer, "%d", id);
      sendeDaten3Str(masch, buffer, ",");
   }

}

Werde es gleich mal probieren und meinen Miss-/Erfolg posten :-)

Ich habe den Code jetzt so geändert, dass alle empfangenen ID identisch 
mit den zu sendenden String sind und die IDs zusammen liegen Das 
vereinfach das Ganze noch ein wenig.

mfg

[Karl H. (kbuchegg)]

pal ... schrieb:

> void sendeMaschAuswToDispl(uint8_t id) {
>    char buffer[3];
>    const char masch[] = "#RL5,5,150,160,#UI10,10,";

Huch, wo kommt denn jetzt auf einmal dieser String her.

Vorher wars doch noch

* für Error Codes    #MN15
* für Bin Codes      #MN16

[Karl H. (kbuchegg)]

Ich hoffe, du hast dir die letzte funktionierende Version aufgehoben. 
:-) Dann kannst du immer zurück, wenn du dich verfranst hast.

[Pal .. (pal)]

Ja, ich habe mir der Teil im Display angesehen und denke, dass es so 
funktionieren könnte :-)

Werde es gleich noch prüfen.

Dank dir!!! Melde mich aber noch mal :-)

[Pal .. (pal)]

Momentan funktioniert gar nichts mehr bei mir....

Ich bin mir ziemlich sicher, dass die Software funktioniert. Betrachte 
ich mir das Display, welche über I2C angesteuert wird leuchtet lediglich 
die SCL Leuchte (rot) - SCA ist dunkel.

Was mich noch wundert, dass ich plötzlich durch

uint8_t eeMasch EEMEM = 0;

einen Fehler bekomme (siehe Anhang). Sobald ich dieses durch

uint8_t eeMasch = 0;

ersetze sind meine Fehler weg.

[Karl H. (kbuchegg)]

#include <eeprom.h>
vergessen?

[Karl H. (kbuchegg)]

> Ich bin mir ziemlich sicher, dass die Software funktioniert.

Das sind keine guten Voraussetzungen. 'ziemlich sicher' ist zu wenig.
Manchmal passiert es schon, dass man zuviele Änderungen auf einmal macht 
und dabei Fehler einbaut.

Hatte wohl jeder schon mal und irgendwann ist der Punkt erreicht an dem 
nur noch eines bleibt:
Zurück zur letzten Version, die ganz sicher noch funktioniert hat und 
dieselben Änderungen nochmal NEU machen. Aber diesmal: in kleineren 
Änderungseinheiten arbeiten! Und nach jeder Änderung ausgiebig testen! 
Egal wie banal die Änderung auch ausgesehen haben mag - denn irgendwas 
übersieht man immer.

Die Einstellung "Ich mache keinen dummen Fehler" ist die falsche. Jeder 
macht dumme Fehler. Und diese muss man so früh wie möglich detektieren. 
Daher:
Immer in kleinen Einheiten arbeiten.
Keine ungetesteten Codemonster produzieren.
Sich immer überlegen, wie man sich möglichst schnell wieder in einen 
Codestand manövrieren kann, den man testen kann - selbst wenn noch nicht 
alle geplanten Änderungen durchgezogen sind oder Einzelfunktionen 
gefaked werden müssen.
Keine Fehler vor sich herschieben. Logikfehler beheben sich 
normalerweise nicht selbst. Schiebt man die Fehlersuche und Behebung zu 
lange vor sich her, wird die Sache nicht einfacher sondern schwieriger.

Die bist Codeproduzent und Controller in Personalunion und musst dich 
selber kontrollieren und an die Leine legen.

[Pal .. (pal)]

das #include <eeprom.h> ist im HeaderFile

Ja eigentlich bin ich auch für die kleinen Einheiten, aber ich habe 
eigentlich nur die Funktion "sendeMaschAuswToDispl" vereinfacht.

Warum aber jetzt plötzlich mein Bus nichts mehr macht ist für mich 
unbegreiflich

[Pal .. (pal)]

Das Problem saß vor dem Rechner :-)

Ich hatte noch einen Fehler in der Funktion "leseDaten".

Die Funktion "sendeMaschAuswToDispl" habe ich ebenfalls noch deutlich 
vereinfacht.

@Karl Heinz & Frank: Ich bedanke mich noch mal ganz herzlich für eure 
Hilfen!!!
Wie funktioniert das in einem Forum, darf ich euch als Dank eine 
Bewertungen abgeben? (Ebay ähnlich)

[Karl H. (kbuchegg)]

pal ... schrieb:

> Wie funktioniert das in einem Forum, darf ich euch als Dank eine
> Bewertungen abgeben? (Ebay ähnlich)

Nö. Sowas gibts hier nicht.

[Frank M. (ukw) (Moderator)]

pal ... schrieb:
> Wie funktioniert das in einem Forum, darf ich euch als Dank eine
> Bewertungen abgeben? (Ebay ähnlich)

Schick dem Karl Heinz ein virtuelles Bierchen von mir, ich schulde ihm 
noch eines ;-)

[Pal .. (pal)]

:-) OK, den Gefallen werde ich dir gerne machen :-)

Danke noch mal!!!

[Karl H. (kbuchegg)]

>     sprintf(buffer, "%d", aktDrehz);

Funktioniert das auf deinem Display mit dem Überschreiben, wenn die 
Drehzahl abnimmt? Wenn also die Drehzahl von 10 auf 9 abfällt, steht 
dann auch tatsächlich "9" auf dem Display und nicht "90"?

Persönlich finde ich es immer eine gute Idee, wenn Zahlenangaben auf 
Displays nicht hin und her hüpfen. Das bringt so eine Unruhe ins Display 
und ist auch gerade in den Bereichen des Wechsels der Stellenanzahl 
ermüdend abzulesen. Die Lösung: die Einerstelle der Zahlen ist immer an 
der gleichen Position. ALso nicht

  10           |
  9            |
  8            | Zeit
  11           v
  ...

sondern

  10
   9
   8
  11
  ....

sprintf macht dir das leicht, weil du ihm nur sagen musst, dass die 
Ausgabe mindestens 2 stellig sein soll.

     sprintf(buffer, "%2d", aktDrehz);

sprintf füllt dann eigenständig links mit Leerzeichen auf. Damit ist 
sichergestellt, dass die Einerstelle im String immer an der gleichen 
Position ist.
(Und nebenbei löst das dann auch gleich noch das Überschreibproblem)

Will man mit führenden 0-en auffüllen lassen, dann eben

     sprintf(buffer, "%02d", aktDrehz);

[Pal .. (pal)]

Hallo Karl Heinz,

ja das funktioniert und die Stellen springen nicht hin und her. Das 
passt schon...

Aber danke für den Hinweis...

@Karl Heinz: Im Anhang dein virtuelles Bierchen :-)

[Karl H. (kbuchegg)]

Merci