ATmega 8 hängt sich auf

[Hannes E. (k1ngarthur)]

Ich habe einen Atmega8 in Arbeit.
Leider hängt er sich (ausschließlich) innerhalb folgender Schleife auf:

while( !btn_enter && !btn_menue ){

    if( btn_up && *constant < i ) *constant = *constant + 1;
    if( btn_down && *constant > 0 ) *constant = *constant - 1;

    btn_down = btn_up = 0;

    set_cursor(x, 1);

    // Zeit oder Datum anzeigen?
    if( time ) show_time();
    else {

      show_wkday();
      show_date();

    }



    if( btn_menue ) return;

    _delay_ms(100);

  }

Das passiert rein zufällig.

btn_... wird über einen Interrupt von einem Portexpander gesetzt.
Ansonsten wird nur ein Wert erhöht, bzw. erniedrigt und dann wird er 
angezeigt.

Aktiviert sind zudem beide Externe-Interrupts, 3 Timer für SoftPWM etc.
Fällt jemanden ein Grund ein, warum der µC sich an dieser Stelle 
aufhängen kann oderliegt es an einem anderen Codeteil?

[Stefan B. (stefan)]

Leider kann man bei diesem Codefetzen nur raten. Es fehlen die 
Definitionen aller Variablen und die Definitionen der aufgerufenen 
Funktionen.

> btn_... wird über einen Interrupt von einem Portexpander gesetzt.

Und sind deshalb volatile gekennzeichnet?

Hängt sich die nackische Variante, d,h. keine Funktionsaufrufe auch auf? 
Um das zu erkennen, kannst du z.B. vor dem while eine Debug-LED 
anschalten und dannach ausschalten. Wenn du ein Oszi oder ein LA hast, 
reicht das Schalten eines freien Portpins natürlich auch.

  while( !btn_enter && !btn_menue )
  {
    if( btn_up && *constant < i ) 
      *constant = *constant + 1;

    if( btn_down && *constant > 0 ) 
      *constant = *constant - 1;

    btn_down = 0;
    btn_up = 0;

#if 0
    set_cursor(x, 1);
    // Zeit oder Datum anzeigen?
    if( time ) 
    {
      show_time();
    } 
    else 
    {
      show_wkday();
      show_date();
    }
#endif

    if( !btn_menue ) 
      _delay_ms(100);
  }

[Hannes E. (k1ngarthur)]

> Und sind deshalb volatile gekennzeichnet?
Jup,ich brauche Sie später noch in der main();
Die Variablen sind im übrigen alle int.
*constant zeigt auf ein Element einer Struktur.

Durch das ständige auftreten von Interrupts (habe alle 3 Timer an) 
könnte der Programmablauf bei entsprechend langen Interruptroutinen ja 
blockiert werden.
Könnte das ein Grund sein. Dann müsste das aber jedes mal passieren und 
nicht rein zufällig.
Oft lässt sich der Wert auch ein paar ma ändern und dann hängt der AVR 
sich auf.

[Oliver (Gast)]

>Leider hängt er sich (ausschließlich) innerhalb folgender Schleife auf:

Wie hast du das denn herausgefunden?

Oliver

[Christian H. (netzwanze)]

Könnte es sein, dass constant urplötzlich in den Stack zeigt?
Falls ja, ist Aufhängen vorprogrammiert.

Es wäre ansonsten viel besser, wenn man den kompletten Code sieht.
Du glaubst gar nicht, wie viele Leute schon behaupteten, dass die 
anderen Funktionen ihres Programms garantiert fehlerfrei seien. Meist 
steckte der Fehler dann in diesen Funktionen.

Btw:
Lange Interrupts sollte es nicht geben. In einem INT wird nur das 
nötigste gemacht (kein delay!); idealerweise nur Flags gesetzt, die dann 
in der Hauptschleife ausgewertet werden.

[Hannes E. (k1ngarthur)]

> Wie hast du das denn herausgefunden?
Die Funktion ändert einen Wert (z.B. eine Stelle der Uhrzeit). Die 
Gesamte Uhrzeit ist in einer Struktur gespeichert, welche dann mittels 
show_time() ausgegeben wird. Drück man jetzt also eine Taste, erhöht 
sich der Wert und der neue Wert wird angezeigt. Manchmalpassiert aber 
gar nix, obwohl der Interrupt ausgelöst wird.
Der rest funktioniert eigendlich so wie er soll, ich poste aber mal 
alles nötige:

// ----------------------------------------------------------
// Interrupt für Tastatur
ISR(INT_KEYBRD){

  uint8_t keybrd;
  keybrd = twi_read( IO_ADDR, 0x00 );

  if( keybrd == 0x10 ) btn_menue = 1;
  if( keybrd == 0x08 ) btn_enter = 1;
  if( keybrd == 0x04 ) btn_up = 1;
  if( keybrd == 0x02 ) btn_down = 1;
  if( keybrd == 0x01 ) btn_snooze = 1;

  if( keybrd ) btn_menue_key = 1;

}
// ----------------------------------------------------------

// ----------------------------------------------------------
// AUSSCHNITT AUS main()

// ggf.Menü aufrufen
    if( btn_menue_key == 1 ){

      menue();

      btn_menue_key = 0;
      edit_time = 0;

      lcd_clear();

    }
// -----------------------------------------------------------

// -----------------------------------------------------------
// Funktionen für die Menünavigation
void menue(){

  struct menueP *tmp;
  void (*function)(uint8_t);

  tmp = start;

  while( tmp ){

    // Innerhalb des Menüs
    function = tmp -> func;
    btn_menue = 0;
    btn_enter = 0;
    btn_up = 0;
    btn_down = 0;
    btn_menue_key = 0;

    
    // Anzeigen des Menüpunktes
    lcd_bg(1);            // Displaybeleuchtung an
    lcd_clear();          // Display löschen
    lcd_home();

    lcd_string(tmp -> name);


    while( !btn_menue_key ){

      set_cursor(7,2);
      lcd_string(">");

      // ggf.Funktion aufrufen
      if( btn_enter && *function  ){
        
        lcd_clear();

        (*function)(tmp -> fdata);

        btn_menue_key = 1;
        btn_menue = 0;
        btn_enter = 0;
        btn_up = 0;
        btn_down = 0;        

      }

      // Warten auf Benutzereingabe
      if( btn_menue ) tmp = tmp -> parent;
      if( btn_enter && !function ) tmp = tmp -> child;
      if( btn_up ) tmp = tmp -> prev;
      if( btn_down ) tmp = tmp -> next;

    }

  }

  btn_menue = 0;
  btn_enter = 0;
  btn_up = 0;
  btn_down = 0;

  return;

}
// -----------------------------------------------------------

// -----------------------------------------------------------
// Aufruf der funktion zum Setzen einer Stelle der Uhrzeit (Beispiel)
  m_set_time_constant(&new_timestamp.thours, 2, 4, 1);
// -----------------------------------------------------------

// -----------------------------------------------------------
void m_set_time_constant(int *constant, int i,int x, int time){
  
  //Einstellen einer Zeitkonstante
  while( !btn_enter && !btn_menue ){

    if( btn_up && *constant < i ) *constant = *constant + 1;
    if( btn_down && *constant > 0 ) *constant = *constant - 1;

    btn_down = btn_up = 0;

    set_cursor(x, 1);

    // Zeit oder Datum anzeigen?
    if( time ) show_time();
    else {

      show_wkday();
      show_date();

    }

    if( btn_menue ) return;

    _delay_ms(100);

  }

  btn_enter = 0;

  return;

}
// -----------------------------------------------------------

// -----------------------------------------------------------
// -- Ausgabe des Wochentags
void show_wkday(){

  get_time();        // aktuelle Zeit
  edit_timestamp = timestamp;
  if( edit_time ) edit_timestamp = new_timestamp;

  // Wochentag anzeigen
  switch (edit_timestamp.wkday){

    case 0:
      lcd_string("So.");
      break;
    
    case 1:
      lcd_string("Mo.");
      break;
      
      
    case 2:
      lcd_string("Di.");
      break;
      
    case 3:
      lcd_string("Mi.");
      break;
      
    case 4:
      lcd_string("Do.");;
      break;
      
    case 5:
      lcd_string("Fr.");
      break;  
      
    case 6:
      lcd_string("Sa.");
      break;  

  }

}
// -----------------------------------------------------------

// -----------------------------------------------------------
// -- Ausgabe des Datum
void show_date(){

  // Auslesen der Daten aus dem Uhrenbaustein
  char ASCII[ASCII_L];

  get_time();        // Aktuelle Zeit
  edit_timestamp = timestamp;

  if( edit_time ) edit_timestamp = new_timestamp; 

  lcd_string( itoa(edit_timestamp.tday, ASCII, 10) );
  lcd_string( itoa(edit_timestamp.nday, ASCII, 10) );

lcd_string( "." );  

  lcd_string( itoa(edit_timestamp.tmon, ASCII, 10) );
  lcd_string( itoa(edit_timestamp.nmon, ASCII, 10) );
  lcd_string(".");              


}
// -----------------------------------------------------------

// -----------------------------------------------------------
// -- Ausgeben der Uhrzeit
void show_time(){

  // Auslesen der Daten aus Uhrenbaustein
  char ASCII[ASCII_L];

  if( !flash_time ) get_time();    // Aktuelle Zeit   
  edit_timestamp = timestamp;

  if( edit_time ) edit_timestamp = new_timestamp;

  
  lcd_string( itoa(edit_timestamp.thours, ASCII, 10) );  
  lcd_string( itoa(edit_timestamp.nhours, ASCII, 10) );
  lcd_string(":");              

  lcd_string( itoa(edit_timestamp.tmin, ASCII, 10) );
  lcd_string( itoa(edit_timestamp.nmin, ASCII, 10) );
  lcd_string(":");              

  lcd_string( itoa(edit_timestamp.tsec, ASCII, 10) );  
  lcd_string( itoa(edit_timestamp.nsec, ASCII, 10) );

}
// -----------------------------------------------------------

Die Interrupts (bis auf Tastatur-Interrupt) habe ich jetzt so geändert, 
dass sie nur noch Flags setzen.
Wenn ich die Funktionsaufrufe zum Anzeigen auslasse, konnte den Fehler 
bisher nicht reproduzieren. Diese Funktionen werden aber auch regülärzum 
Anzeigen der Uhrzeit im Hauptprogramm genutzt.Dort gab es damit noch nie 
Probleme.

[Peter (Gast)]

wo constant declariert ist wissen wir aber immer noch nicht.

[Hannes E. (k1ngarthur)]

*constant ist doch im Funktionskopf definiert
> [...] (int *constant, [...]
Reicht das nicht, oder habe ich deine Frage falsch verstanden?

Die Struktur für die Zeit ist folgende:

// Zeit-Struktur
struct time_struc{
  int thours;
  int nhours;

  int tmin;
  int nmin;

  int tsec;
  int nsec;

  int tday;
  int nday;

  int tmon;
  int nmon;

  int wkday;

  int year;
};

An *constant wird nur die Adresse eines Elements der Struktur übergeben.

[Stefan B. (stefan)]

Hannes Eilers schrieb:

> Die Variablen sind im übrigen alle int.

Und alle Zugriffe im gezeigten Code nicht nicht atomar... s. 
Interrupt

> char ASCII[ASCII_L];

Ich sehe keine Definition von ASCII_L. Reicht der Platz für das spätere 
itoa()?

Diese lokale Variable belastet den Stack. Wenn der Stack bereits eng ist 
und eine ISR angesprungen wird, kann die ISR der Todesstoss sein. Ich 
würde die versuchsweise global anlegen. Wieviel RAM braucht dein 
Programm ungefähr (=> AVR-GCC => avr-size)?

[Hannes E. (k1ngarthur)]

> Und alle Zugriffe im gezeigten Code nicht nicht atomar...
Daran habe ich gar nicht gedacht. Werde den Interrupt während des 
Zugriffs auf die Variabeln mal ausschalten.

#define ASCII_L 9

Ich brauche aber ind er Regel weniger Platz. Könnte das also nich 
optimieren. Ist noch so hoch, weil ich beim "anlegen" der Def. nicht 
wusste,wie viel Platz ich brauche.
Das Programm benötigt inzwischen 86,3% (7072 Byte) des 
Programmspeichers.

[Karl H. (kbuchegg)]

Die Sache mit den Einzelvariablen für die Buttons finde ich keine so 
gute Idee. Die würde ich alle beisammen in einem uint8_t lassen (du bist 
überhaupt mit int ziemlich freigiebig!)

Wenn dir zb hier

       btn_menue_key = 1;
        btn_menue = 0;
        btn_enter = 0;
        btn_up = 0;
        btn_down = 0;

ein Interrupt dazwischenknallt, der einzelne btn_xxx Variablen setzt, 
dann können, je nachdem wo der Interrupt auftritt, Tastendrücke 
verlorengehen.

[Stefan B. (stefan)]

Hannes Eilers schrieb:

> Das Programm benötigt inzwischen 86,3% (7072 Byte) des
> Programmspeichers.

Das ist die Größe im Flash also .text plus .data (plus 
Spezialsektionen)? diese Zahl ist weniger interessant.

Wie groß ist der statische Bedarf an RAM, also .data plus .bss? 
RAM-Größe minus statischer Bedarf ist ca. max. Stackgröße. Wenn der 
statische Bedarf in Richtung >3/4 des RAM geht, fange ich an, die Sache 
kritisch zu beäugen.

[Karl H. (kbuchegg)]

Hannes Eilers schrieb:

> Das Programm benötigt inzwischen 86,3% (7072 Byte) des
> Programmspeichers.

Das kriegt man schon noch runter.
Du bist da an einigen gezeigten Stellen ziemlich suboptimal unterwegs.
Du hast zb einige Zahlenausgaben und benutzt dafür jedesmal die Sequenz

  lcd_string( itoa(edit_timestamp.thours, ASCII, 10) );

Warum nicht für Zahlenausgabe eine eigene Funktion machen

void lcd_number( int num )
{
  char buf[10];

  itoa( num, buf, 10 );
  lcd_string( buf );
}

und dann an den verwendenden Stellen

  lcd_number( edit_timestamp.thours );

die Funktion braucht zwar auch etwas Platz, aber dafür sparst du an der 
Stelle der Verwendung Code ein. Und da du die Funktion oft verwendest 
spart das dann in Summe mehr Code ein als durch die zusätzliche Funktion 
aufgebraucht wird.

Aber im Grunde ist die Menge des verwendeten Flash uninteressat. Die 
Data Section ist das interessante.

[Hannes E. (k1ngarthur)]

> du bist überhaupt mit int ziemlich freigiebig!
Das stimmt,ich finde esso übersichtlicher.DerDatenspeicher ist 
gerademalbei 35% da geht also nich was. Aber man könnte das sicherlich 
ändern und optimieren.

>je nachdem wo der Interrupt auftritt, Tastendrücke verlorengehen
Wäre ja nicht so schlimm,man müsste halt nochmal auf die Taste drücken 
:-)

Was meinst du denn,könnte es Probleme mit dem Stack und der ASCII_L 
geben?

Ich werde mal versuchen den Code weiterb zu optimieren. Vielleicht leigt 
es daran. Ich werde aber trotzdem noch etwas Platz für dasProg brauchen, 
da noch einige Funktionen fehlen.

[Karl H. (kbuchegg)]

Hannes Eilers schrieb:
>> du bist überhaupt mit int ziemlich freigiebig!
> Das stimmt,ich finde esso übersichtlicher.

Was ist da dran übersichtlicher? gar nichts!

struct time_struc{
  uint8_t thours;
  uint8_t nhours;

  uint8_t tmin;
  uint8_t nmin;

  uint8_t tsec;
  uint8_t nsec;

  uint8_t tday;
  uint8_t nday;

  uint8_t tmon;
  uint8_t nmon;

  uint8_t wkday;

  int year;
};

ist genauso übersichtlich wie deine Version. In der verwendenden 
Programmierung gibt es keinen Unterschied, ausser das du dir um atomaren 
Zugriff auf einzelne Member der Struktur keine Gedanken mehr zu machen 
brauchst.

Lass es dir noch einmal eindringlich gesagt sein:
Auf einem 8-Bit µC, wie einem Mega8, ist der Datentyp uint8_t oder 
int8_t immer deine erste Wahl!
* das ist der Datentyp mit dem der µC am besten umgehen kann
* alles andere erfordert mehr Aufwand als notwendig
* du hast bei allen anderen Typen immer das Problem, dass du
  Zugriff unter Umständen atomar absichern musst.
* du blockierst unnötigerweise Speicher, der dir an anderer Stelle
  manchmal schmerzlich fehlen kann


(Warum gibt es eigentlich in einem Datum jeweils einen t und einen n 
Eintrag? Das erscheint mir nicht wirklich logisch)


>>je nachdem wo der Interrupt auftritt, Tastendrücke verlorengehen
> Wäre ja nicht so schlimm,man müsste halt nochmal auf die Taste drücken
> :-)

Doch das ist schlimm.

>
> Was meinst du denn,könnte es Probleme mit dem Stack und der ASCII_L
> geben?

Bei 35% RAM Verbrauch: unwahrscheinlich.
Da wird wohl irgendwo ein Pointer im Wald stehen.

[Karl H. (kbuchegg)]

Hannes Eilers schrieb:

> Ich werde mal versuchen den Code weiterb zu optimieren.

Schlechte Strategie.
Durch optimieren wird es selten besser, solange das Problem nicht 'Out 
of memory' lautet.

[Peter D. (peda)]

Hannes Eilers schrieb:
> Die Variablen sind im übrigen alle int.

Damit verschenkst Du schonmal ein Großteil des Speicherplatzes, oder hat 
bei Dir die Stunde 65535 Minuten?

Nimm besser uint8_t, wos ausreicht.


Peter

[Hannes E. (k1ngarthur)]

Also ich habe jetzt alle Variabeln angepasst, was mir mächtig Platz 
spart.
Hierfür erstmal danke!

>(Warum gibt es eigentlich in einem Datum jeweils einen t und einen n Eintrag? Das 
erscheint mir nicht wirklich logisch)

Das t ist für die Zehnerstelle das n für die Einerstelle.Ich brauche die 
einzeln um die Dtaen hinterher in den Uhrenbaustein zu schieben, will 
die Stellen leider einzeln haben.

Meine Tastur habe ich jetzt wie folgt realisiert:

// ----------------------------------------------------------
// Interrupt für Tastatur
ISR(INT_KEYBRD){

  keybrd = twi_read( IO_ADDR, 0x00 );

}
// ----------------------------------------------------------

In den Funktionen wird jetzt der Hex-Wert von keybrd abgefragt:

void m_set_time_constant(uint8_t *constant,uint8_t i,uint8_t x,uint8_t time){
  
  //Einstellen einer Zeitkonstante

  keybrd = 0x00;

  while( keybrd != 0x10 && keybrd != 0x08 ){

    if( keybrd == 0x04 && *constant < i ){
      *constant = *constant + 1;
      keybrd = 0x00;
    }

    if( keybrd== 0x02 && *constant > 0 ){
      *constant = *constant - 1;
      keybrd = 0x00;
    }

    set_cursor(x, 1);

    // Zeit oder Datum anzeigen?
    if( time ) show_time();
    else {

      show_wkday();
      show_date();

    }

    if( keybrd == 0x10 ) return;

    _delay_ms(100);

  }

  keybrd = 0x00;

  return;

}

Bisher hebt das aber das Problem mit dem Aufhängen nicht auf.

[Karl H. (kbuchegg)]

Hannes Eilers schrieb:

>>(Warum gibt es eigentlich in einem Datum jeweils einen t und einen n Eintrag? 
Das
> erscheint mir nicht wirklich logisch)
>
> Das t ist für die Zehnerstelle das n für die Einerstelle.Ich brauche die
> einzeln um die Dtaen hinterher in den Uhrenbaustein zu schieben, will
> die Stellen leider einzeln haben.

Das ist jetzt aber nicht dein Ernst, oder?
(Und dann noch für Zahlen die nur von 0 bis 9 laufen können einen 
eigenen int :-)

Du zwirbelst dir hier eine Menge 'Ungemach' auf, nur damit du die 
Zerlegung einer Zahl in Zehner und Einer direkt beim Verschicken an den 
Uhrenbaustein einsparst?

Hast du Zehner und Einer, dann errechnet sich die dadurch dargestellte 
Zahl durch:

   Zahl = 10 * Zehner + Einer

und die Umkehrung, die Zerlegung ist

   Zehner = Zahl / 10
   Einer = Zahl - 10 * Zehner;

Problem gelöst und du brauchst beim Erhöhen/Erniedrigen der Uhrezeit, 
sowie bei Vergleichen oder sonstigen Rechnereien keine 'Durch die Brust 
von hintern ins Auge' Aktionen mit Zehnern und Einern machen.

Einzig bei der Ausgabe auf LCD muss man ein wenig darauf achten, dass 
führende Nullen mit ausgegeben werden. Aber da die Ausgabe in einer 
einzigen Funktion gesammelt ist, ist auch das kein wirkliches Problem 
:-)

Jetzt versteh ich auch, warum da so viele lcd_string( itoa ... 
Konstruktionen sind

[Karl H. (kbuchegg)]

Hannes Eilers schrieb:

> Bisher hebt das aber das Problem mit dem Aufhängen nicht auf.

Das Problem wird auch nicht in dieser Funktion sein.
Was du siehst, sind die Symptome. Die Ursache kann ganz woanders 
stecken.

[Peter D. (peda)]

Hannes Eilers schrieb:
> Bisher hebt das aber das Problem mit dem Aufhängen nicht auf.

Nur Stochern im Nebel bringts eben nicht.
Poste endlich mal was compilierbares als Anhang!

Aber mach erstmal die tausende Leerzeilen weg, ich hab schon Brandblasen 
am Zeigefinger vom vielen Scrollen.


Peter

[Hannes E. (k1ngarthur)]

Es sind mehrere Dateien.

Der AVR soll am Ende folgendes können(damit ihr versteht was ich mit dem 
Code eig. bezwekcne will):

- Anzeige von Uhrzeit und Datum (ohne Jahr) im Wechsel (4sec.)
- Alarmfunktion
- Ansteuerung von RGB-LEDs über RGB-Werte (inkl-. Helligkeit)
- Anzeige der Daten auf einem LCD
- programmierbarer Alarm (best.Wochentage, Tägl. oder aus)
- Steuerung eines MP3-Player (über Simulation eines Tastendrucks)

Dazu habe ich einen PCF8583 und einen PCA9555 am I2C.
Letzterer übernimmt die Ports für den MP3-Player und ide Tastatur. Beide 
sind über Interrupts angeschlossen.

Ich bin euch dankbar für die vielen Tipps, wie man etwas besser oder 
einfacher realisieren kann.
Aber jetzt alles zu überarbeiten ist nicht Zweck dieses Forum, das muss 
ich schon selber machen *freu, seufz* ;-)
Wenn jemand also einen Fehler findet, der dazu führen könnte, dass sich 
der AVR aufhängt wäre ich ihm sehr dankbar.

p.s.: Ja,Wecker wird ander geschrieben :-))

[Karl H. (kbuchegg)]

Kurzer Zwischenbericht:

Du arbeitest mit malloc. Damit sind alle Aussagen bezüglich 
Speicherverbrauch und den 35% zu Makulatur verkommen. Die Schlacht um 
einen möglichen Stacküberlauf ist damit wieder eröffnet.

-> Versuch ohne malloc auszukommen.
Ein Menüsystem muss nicht die Menüs zur Laufzeit zusammenbauen. Das kann 
man auch statisch machen.
Und dann stimmen die vom Compiler ausgeworfenen Zahlen bezgl. 
Speicherverbrauch wieder und sind aussagekräftig.

[Karl H. (kbuchegg)]

in menu.h

>      if( keybrd == 0x08 && *function  ){

       if( keybrd == 0x08 && function  ){

[Hannes E. (k1ngarthur)]

> Das kann man auch statisch machen.
Ich möchte jedoch gerne die Menüstruktur als Liste machen, dann bietet 
sich dieses Verfahrne doch an.
Wie würdest du es denn machen, dammit die Menüeinträge als Liste nicht 
erst zur Laufzeit angelegt werden? Hab schon drüber nachgedacht sie ins 
EEPROM zu legen, aber das ist auch nicht gerade so komfortabel.

Das Menü lässt sich auch prima aufrufen und durchblättern. Nur, wenn ich 
die Funktion zum Verändern von Werten aufrufe hackts manchmal. manchmal 
funktioniert es aber auch.

[Karl H. (kbuchegg)]

Hannes Eilers schrieb:
>> Das kann man auch statisch machen.
> Ich möchte jedoch gerne die Menüstruktur als Liste machen, dann bietet
> sich dieses Verfahrne doch an.

Kannst du doch.
Leg meinetwegen die einzelnen Knoten statisch an und verkette sie zur 
Programmlaufzeit.

Aber verzichte auf malloc!

[Hannes E. (k1ngarthur)]

Also z.B. so?:

struct menueP m1, m2, m3, m4;

  // -----------------------------
  new_menueP("01 Zeit - Datum", &m1);
  new_menueP("02 Wecker", &m2);
  new_menueP("03 Farbe", &m3);
  new_menueP("04 Musik", &m4);

  m1.next = m3.prev = &m2;
  m2.next = m4.prev = &m3;
  m3.next = m1.prev = &m4;
  m4.next = m2.prev = &m1;

  start = &m1;
  // -----------------------------

Der Aufruf des ersten Elements funktioneirt auch. Das nächste in der 
Liste wird aber irgendwie nicht richtig gefunden, es tauchen dann als 
bezeichnung auf dem Display nur Zeichenfragmente auf (da scheint der 
Pointer irgendwie nicht richtig zu sein).

[Karl H. (kbuchegg)]

Hier

    while( !keybrd ){

      set_cursor(7,2);
      lcd_string(">");

      // ggf.Funktion aufrufen
      if( keybrd == 0x08 && *function  ){

        lcd_clear();

        (*function)(tmp -> fdata);

        keybrd = 0x20;

      }

      // Warten auf Benutzereingabe
      if( keybrd == 0x10 ) tmp = tmp -> parent;
      if( keybrd == 0x08 && !function ) tmp = tmp -> child;
      if( keybrd == 0x04 ) tmp = tmp -> prev;
      if( keybrd == 0x02 ) tmp = tmp -> next;

solltest du unbedingt noch 'function' nachziehen. Sonst kann es dir 
passieren, dass du die falsche Funktion aufrufst. Am besten jedoch, 
eliminerst du die Variable function und greifst immer über tmp zu
(und bitte gewöhn dir an dieser Stelle die Leerzeichen wieder ab. Der 
Pointer und der Member auf den über den Pointer zugegriffen werden soll, 
gehören zusammen. Der -> ist Trenner genug.
Ich bin auch versucht zu sagen: Gewöhn dir auch das weiterschreiben in 
einer Zeile nach einem if wieder ab. Die abhängige Anweisung kommt 
eingerückt eine Zeile tiefer. Dann sieht man auch aus 3m Entfernung, das 
da irgendetwas besonderes passiert)

Auch wimmeln mir da ein bischen zuviele HEX-Konstanten rum. Mach dir 
dafür ein paar #define und lesbare Namen.

// Funktionen für die Menünavigation
void menue() {

 struct menueP *tmp = start;

  while( tmp ) {

    keybrd = 0x00;

    lcd_bg( 1 );      // Displaybeleuchtung an
    lcd_clear();
    lcd_home();
    lcd_string( tmp->name );

    while( !keybrd ) {
      set_cursor(7,2);
      lcd_string(">");

      if( ( keybrd & KEY_ENTER ) ) {
        if( tmp->function ) {
          lcd_clear();
          (*tmp->function)( tmp->fdata );
          keybrd = 0x20;   // was ist 0x20?
        }
        else
          tmp = tmp->child;
      }

      else if( keybrd & KEY_PARENT )
        tmp = tmp->parent;

      else if( keybrd & KEY_PREV )
        tmp = tmp->prev;

      else if( keybrd & KEY_NEXT )
        tmp = tmp->next;
    }
  }
}

Muss das wirklich sein, dass da bei jedem Durchlauf durch die äussere 
while-Schleife das Display gelöscht und neu beschrieben wird? Das wird 
ganz schön flackern :-)

[Karl H. (kbuchegg)]

Hannes Eilers schrieb:
> Also z.B. so?:

Genau


> Der Aufruf des ersten Elements funktioneirt auch. Das nächste in der
> Liste wird aber irgendwie nicht richtig gefunden, es tauchen dann als
> bezeichnung auf dem Display nur Zeichenfragmente auf (da scheint der
> Pointer irgendwie nicht richtig zu sein).

Da hast du irgendeinen Fehler gemacht.
Allerdings ist der ganze 'start' Pointer suspekt. Den braucht kein 
Mensch.
Übergib der menue Funktion die Adresse des ersten Menüknotens und gut 
ists.
Du scheinst höllische Angst davor zu haben, Argumente an Funktionen zu 
übergeben. Statt dessen hast du dann eine Menge globale Variablen, die 
den Sacheverhalt auch nicht besser machen.

[Hannes E. (k1ngarthur)]

OK, habe jetzt alles geändert.
Navigation durch die Menüs funktioniert auch.

.data + .bss +.noinit = 79,6%
.text + .data +.bootloader = 61,1%


Ich habe zwar nciht so viel Ahnung wie viele Daten man maximal haben 
sollte, damit der Stack nicht voll ist aber ~80% sind wohl zu viel oder?

[edit]
Achso, lasse ich die Untermenüs weg, sind es statt ~80% nur ~15%.
Ev. könnte man die Hauptnavigation statisch machen und die Unterpunkte 
dynamisch generieren, dass kostet mehr Programmzeilen aber erheblich 
weniger Platz auf dem Stack oder?
[/edit]

[Peter D. (peda)]

Hannes Eilers schrieb:
>

> struct menueP m1, m2, m3, m4;
> 
>   // -----------------------------
>   new_menueP("01 Zeit - Datum", &m1);
>   new_menueP("02 Wecker", &m2);
>   new_menueP("03 Farbe", &m3);
>   new_menueP("04 Musik", &m4);
> 
>   m1.next = m3.prev = &m2;
>   m2.next = m4.prev = &m3;
>   m3.next = m1.prev = &m4;
>   m4.next = m2.prev = &m1;
> 
>   start = &m1;
>   // -----------------------------
>

Wozu die Speicherverschwendung mit dem .next und .prev?
Du willst doch immer der Reihe nach zählen, dann zähl doch einfach nur 
den Index:

#define MENUES 4
struct menueP m[MENUES];
uint8_t menue_idx = 0;
// ...
  next = menue_idx >= (MENUES - 1) ? menue_idx + 1 : 0;
  prev = menue_idx ? menue_idx - 1 : MENUES - 1;

Peter

[Karl H. (kbuchegg)]

Hannes Eilers schrieb:

> Ich habe zwar nciht so viel Ahnung wie viele Daten man maximal haben
> sollte, damit der Stack nicht voll ist aber ~80% sind wohl zu viel oder?

Welcher Prozessor?

80% ist schon eine Menge Holz.

> [edit]
> Achso, lasse ich die Untermenüs weg, sind es statt ~80% nur ~15%.
> Ev. könnte man die Hauptnavigation statisch machen und die Unterpunkte
> dynamisch generieren, dass kostet mehr Programmzeilen aber erheblich
> weniger Platz auf dem Stack oder?
> [/edit]

Den meisten Platz werden die Texte brauchen.
Jeweils ein 17-er char Array für einen Text ist schon ziemlich viel.
Mach da mal einen char* Pointer rein und setze den auf den in der 
Erzeugungsroutine auf den konstanten Text den du bekommst. Das sollte 
den Speicher schon mal drücken. Wenn das immer noch nicht reicht könnte 
man die Texte ins Flash auslagern. Das geht immer noch relativ billig 
ohen allzuviele Änderungen im Programm.

[Karl H. (kbuchegg)]

Wie hoch bist du jetzt mit dem Speicherverbrauch?

[Karl H. (kbuchegg)]

struct menueP {
  const char * name;
  
  void (*func)(uint8_t);  
  uint8_t fdata;

  uint8_t prevIndex;
  uint8_t nextIndex;
  struct menueP* child;
};

struct menuP soundMenu[] = {
   { "MP3-Player",       NULL, 0,   2, 1,  NULL },
   { "Lautstärke",       NULL, 0,   0, 2,  NULL },
   { "Weck-Song",        NULL, 0,   1, 0,  NULL },
};

struct menuP colorMenu[] = {
   { "Farbe setzen",     NULL, 0,   5, 1,  NULL },
   { "auto Farbwechsel", NULL, 0,   0, 2,  NULL },
   { "Weckfarbe",        NULL, 0,   1, 3,  NULL },
   { "auto. WeFarbWechs",NULL, 0,   2, 4,  NULL },
   { "norm. Helligkeit", NULL, 0,   3, 5,  NULL },
   { "Weck-Helligkeit",  NULL, 0,   4, 0,  NULL },
};

struct menuP alarmMenu[] = {
    ...
};

struct menuP timeMenu[] = {
   { "11 Zeit",          m_time, 0, 1, 1,  NULL },
   { "12 Datum",         m_date, 0, 0, 0,  NULL },
};

struct menuP mainMenu[] = {
   { "01 Zeit - Datum",  NULL, 0,   3, 1,  timeMenu  },
   { "02 Wecker",        NULL, 0,   0, 2,  alarmMenu },
   { "03 Farbe",         NULL, 0,   1, 3,  colorMenu },
   { "04 Musik",         NULL, 0,   2, 0,  soundMenu },
};

so würde ich das Menüsystem aufbauen. Wenn du den parent noch haben 
willst, dann mach in die Menüstruktur noch den Pointer dafür rein und 
füll den Parent Pointer dynamisch in der Menü Funktion aus. Wenn in ein 
Childmenü gewechselt wird, wird der parent Pointer in diesem Childmenü 
auf den Meneüpointer gesetzt, der für den Aufruf gesorgt hat.

Diese Darstellung oben hat auch den Vorteil gegenüber einer dynamischen 
Allokierung, dass man einen besseren Überblick über den Menüaufbau hat, 
als wie wenn sich der Aufbau in 40 Zeilen Code verbergen.

[Hannes E. (k1ngarthur)]

> Mach da mal einen char* Pointer rein und setze den auf den in der
> Erzeugungsroutine auf den konstanten Text den du bekommst.

Damit komme ich auf 53,2%
µC ist ein ATmega8-16

Fehler tritt aber immer noch auf.

[Karl H. (kbuchegg)]

Hannes Eilers schrieb:
>> Mach da mal einen char* Pointer rein und setze den auf den in der
> Erzeugungsroutine auf den konstanten Text den du bekommst.
>
> Damit komme ich auf 53,2%
> µC ist ein ATmega8-16

OK.
Damit sollte man auf der sicheren Seite sein.

> Fehler tritt aber immer noch auf.
Es wäre einfacher, wenn ich hier das Zeug nachstellen könnte :-)

So bleibt nur: Alle Pointer Zugriffe absuchen. Sicherheitshalber immer 
auf NULL Pointer testen.

[Karl H. (kbuchegg)]

Nachmal zum Fehlerbild.
Was machst du genau nach dem Einschalten, damit man zum Fehler kommt 
(und wie macht sich der Fehler bemerkbar?)

[Hannes E. (k1ngarthur)]

Na dann mal ran.(Das wird ein Spaß!) seufz

[Karl H. (kbuchegg)]

Solche Abfragen

if( keybrd == 0x04 && *constant < i ){
    **************

sind nicht sehr schlau. Du hast deine Tasten bitcodiert. Jede Taste hat 
ihr eigenes Bit. Das kannst du benutzen um defensiv zu programmieren

 if( ( keybrd & 0x04 ) && *constant < i )

Der Unterschied:
Bei deiner Version MUSS keybrd exakt 0x04 sein. Ist irgendein anderes 
Bit aus irgendwelchem Grund gesetzt, dann geht deine Version nicht mehr.
Bei meiner Version wird getestet, ob das zur Taste gehörende Bit gesetzt 
ist. Diese Version funktioniert also auch dann noch, wenn in keybrd 
irgendein anderes Bit zusätzlich gesetzt wurde. Bei den erhöh und 
erniedrig-Tasten ist das wahrscheinlich egal, aber die Tasten für 
'Aussteigen' können davon profitieren, weil dein Programm dann auch aus 
einer 'kleinen' Fehlersituation heraus noch sinnvoll arbeiten kann.

[Karl H. (kbuchegg)]

Hannes Eilers schrieb:
> Na dann mal ran.(Das wird ein Spaß!) *seufz*

Nochmal:
Was musst du exakt machen, damit es zum Fehler kommt?
Und wie sieht der Fehler aus?

(und poste nochmal deinen jetzigen Code)

[Karl H. (kbuchegg)]

Hannes Eilers schrieb:
> Na dann mal ran.(Das wird ein Spaß!) *seufz*



Na ja.
So schlimm auch wieder nicht.
Du hast ein LCD.
Das kannst du benutzen um dir aus dem Programm heraus, Statusausgaben 
machen zu lassen. Wenn dein Programm nicht mehr auf Tastendrücke 
reagiert, aber in einer Schleife sein müsste, ist es zb eine gute Idee 
innerhalb dieser Schleife sich die keybrd Variable an einer Stelle im 
LCD ausgeben zu lassen.
usw. Wenn du nicht weißt was dein Programm tut, dann verpass ihm 
Ausgaben, so dass dir das Programm erzählt was es gerade tut :-)

[Hannes E. (k1ngarthur)]

Also der Fehler muss irgendwo in der Schleife liegen.
Außerhalb tritt nie ein Fehler auf.

Wenn ich aber innerhalb einfach mal wild auf die Tasten drücke, bleibt 
der AVR stehen und erhöht den Wert nicht mehr. Der Interrupt wird auch 
nicht mehr aufgerufen.
verwende ich jetzt aber Statusmeldungen um die Fehlerquelle zu 
lokalisieren, hängt er sich nicht mehr auf.

Ich habe daher einmal das delay am Ende auf 250ms gesetzt (ist ja eine 
kaum spürbare Zeit) und siehe da: Kein Fehler mehr.
Wenn ich gar kein delay rein mache hängt er sich im übrigen sofort auf.

Auf jeden Fall habe ich es heute geschafft, dass mein Code viel weniger 
Platz weg nimmt (jetzt ~53%).

[Peter D. (peda)]

ISR(INT_KEYBRD){

  keybrd = twi_read( IO_ADDR, 0x00 );

Oh oh oh !

Wenn dann im Mainprogramm auch I2C gemacht wird, krachts.
I2C ist quasi ein Device, wie z.B. das LCD. Ein Transfer darf nicht 
durch eine anderen unterbrochen werden.

Wenn Du z.B. in Word eine Datei öffnest, kann da auch kein anderer 
schreiben. Es würde krachen, daher blockiert der PC Devices und Dateien, 
bis der anfordernde Prozess fertig ist.


Man sollte für I2C auch immer ein Timeout aufsetzen (1-10ms). Falls mal 
ne Störung erfolgt und der Bus sich verklemmt, würde sonst Dein Program 
für immer hängen.


Peter

[Hannes E. (k1ngarthur)]

> Oh oh oh !
Das wird nur vor der Aktivierung der Interrupts aufgerufen, da der 
PCA9555 nach dem Start bereits einen Interrupt erzeugt. Er geht nämlich 
davon aus, dass an den Pins ein High liegen soll, tut's aber nciht >> 
Interrupt.
Dann beschreibe ich ihn, dass Low normal ist udn nur bei high ein 
interrupt kommen soll.
Der erste Interrupt verschwindet aber erst, nachdem sich der Status des 
Pins ändert oder man ihn einmal ausliest.
Da sollte es also keine Probleme geben.



Habe jetzt einmalmit dem Eintritt in's Menü die Timer ausgeschaltet 
(bzw. die Interrupts dafür deaktiviert), dann lässt sich das delay noch 
ein wenig nach unten korrigieren.

[Peter D. (peda)]

Hannes Eilers schrieb:
> Habe jetzt einmalmit dem Eintritt in's Menü die Timer ausgeschaltet
> (bzw. die Interrupts dafür deaktiviert), dann lässt sich das delay noch
> ein wenig nach unten korrigieren.

Dann scheine ich ja richtig zu liegen.

Ich weiß zwar nicht, welches Delay Du meinst, aber für ein sicheres 
Programmkonzept darf davon nicht die Funktion abhängen.

Ich würde dieses gefährliche Konzept in jedem Fall ändern.
Die Verwendung einer nicht reentranten Funktion in Main und Interrupt 
ist pfui-bäh und wird Dir auch immer wieder auf die Füße fallen.

Setze einfach im Interrupt ein Bit und im Main wird dieses getestet und 
dann an sicherer Stelle das I2C aufgerufen.
Tastendrücke sind nichts eiliges, da ist kein Interrupt nötig.


Peter

[Hannes E. (k1ngarthur)]

> Die Verwendung einer nicht reentranten Funktion in Main und Interrupt ist > 
pfui-bäh
Da muss ich zustimmen. Ich habe das jetzt geändert, was den Programmcode 
etwas größer macht, da ja jede Funktion die laufend auf Tastatureingaben 
wartet eine Zeile mehr braucht, aber dafür läuft jetzt alles stabil und 
ohne Fehler (auch ohne delays).

Manchmal vergisst man eben diese elementaren (wichtigen!) Dinge.
Danke für eure Hilfe.