Tasterabfrage nach Dannegger – plötzlich unplausible Resultate

[Moritz A. (moritz_a)]

Moin,

ich bin gerade etwas am Verzeifeln, wobei ich vermutlich den Wald vor 
lauter Bäumen nicht mehr sehe. Immer wenn ich ein bestimmtes Stück Code 
aktiv habe, rennt die Programmlogik Amok, also ob permanent Tasten 
gedrückt würden.

Auch ein probeweises entfernen der kompletten Kabel an den Pins sowie 
mit dem Oszi beobachten haben nichts ergeben, die Pegel sind permanent 
sauber auf +5V durch die internen Pull-Ups.

Hardwareaufbau: Attiny261a@8MHz, ISP angeschlossen, sonst:
PB0-2, PA4-7 LCD
PA0: LED
PB4-6: 3 Taster nach Masse
VCC/GND und AVCC/AGND nach +5V/GND, auf jeder Seite einen 100nF

Das Programm ist gerade im Prinzip nur ein bisschen Code zwischen 
Beitrag "Universelle Tastenabfrage" und Peter Fleury's 
LCD-Routinen.

lcd.h Pindefinitionen:

#define LCD_PORT         PORTA        /**< port for the LCD lines   */
#define LCD_DATA0_PORT   LCD_PORT     /**< port for 4bit data bit 0 */
#define LCD_DATA1_PORT   LCD_PORT     /**< port for 4bit data bit 1 */
#define LCD_DATA2_PORT   LCD_PORT     /**< port for 4bit data bit 2 */
#define LCD_DATA3_PORT   LCD_PORT     /**< port for 4bit data bit 3 */
#define LCD_DATA0_PIN    4            /**< pin for 4bit data bit 0  */
#define LCD_DATA1_PIN    5            /**< pin for 4bit data bit 1  */
#define LCD_DATA2_PIN    6            /**< pin for 4bit data bit 2  */
#define LCD_DATA3_PIN    7            /**< pin for 4bit data bit 3  */
#define LCD_RS_PORT      PORTB        /**< port for RS line         */
#define LCD_RS_PIN       0            /**< pin  for RS line         */
#define LCD_RW_PORT      PORTB        /**< port for RW line         */
#define LCD_RW_PIN       1            /**< pin  for RW line         */
#define LCD_E_PORT       PORTB        /**< port for Enable line     */
#define LCD_E_PIN        2            /**< pin  for Enable line     */

Der Code ist leider noch nicht der sauberste, sobald Z.106-Z.128 aktiv 
sind flackert das Menü durch als würden permanent die Tasten gedrückt 
werden. Ich habe die Taster testweise auch an andere Ports gehängt, 
selbes Problem, so dass ich vermutlich irgendwo im Code ein Problem 
habe, welches ich aber gerade beim besten willen nicht finde.

Danke
Moritz

[Claude M. (stoner)]

> Der Code ist leider noch nicht der sauberste, sobald Z.106-Z.128 aktiv

Hatte etwas gedauert bis ich deine State Maschine (case statement) 
verstanden habe. Sprechende Konstanten würden uns und auch dir das Lesen 
vereinfachen und Fehler vermeiden helfen. Ich denke aber, die State 
Maschine ist soweit ok, ausser das du bei beiden Navigationstasten in 
die selbe Richtung navigierst.

> sind flackert das Menü durch als würden permanent die Tasten gedrückt

Also auch wennn du nie eine Taste gedrückt hattest? Was heisst flackert? 
Dein Cursor rast durch?

> werden. Ich habe die Taster testweise auch an andere Ports gehängt,
> selbes Problem, so dass ich vermutlich irgendwo im Code ein Problem
> habe

Das sehe ich auch so (der code von Peter Dannegger ist auf jeden Fall 
ok)...

Gruss
Claude

[Moritz A. (moritz_a)]

Claude M. schrieb:
>> Der Code ist leider noch nicht der sauberste, sobald Z.106-Z.128
> aktiv
>
> Hatte etwas gedauert bis ich deine State Maschine (case statement)
> verstanden habe. Sprechende Konstanten würden uns und auch dir das Lesen
> vereinfachen und Fehler vermeiden helfen. Ich denke aber, die State
> Maschine ist soweit ok, ausser das du bei beiden Navigationstasten in
> die selbe Richtung navigierst.

Ja, das meinte ich mit unsauber, sowas kommt bei mir dann meistens erst 
später beim aufräumen. Aber da ging es jetzt ja mal schnell. An dem 
Punkt mit dem Navigieren war ich noch gar nicht, daher war es mir nicht 
aufgefallen.

>> sind flackert das Menü durch als würden permanent die Tasten gedrückt
>
> Also auch wennn du nie eine Taste gedrückt hattest? Was heisst flackert?
> Dein Cursor rast durch?

Genau, bzw ich wechsel wild in das Menü (state >10) und zurück, sowie 
cur_mode wird hin- und herinvertiert.

Allerdings bekomme ich den Fehler jetzt gerade, mit zusätzlichen 
Debugausgaben, nicht mehr hin. Vorhin konnte man aber in keys noch sehr 
schön die Schalterbits toggeln sehen:/

Im Moment kann ich noch nicht ganz Eingrenzen, ob es am geänderten Code 
liegt oder daran, dass ich dem Steckbrett noch einen 10µF-Kondensator 
auf der Versorgungsleiste und einen 10k an !RST spendiert habe.

Mal schauen was der Code gleich im Sollzustand wieder sagt.

Die mainloop jetzt:

    while(1) {
        uint8_t keys;
        keys = get_key_press(0xff);
        lcd_gotoxy(0,1);
        lcd_itoa(keys);

        // menue system
#define DISPLAY_NORM_INIT   0
#define DISPLAY_NORM_WAITKEY    1
#define DISPLAY_MENU_INIT       10
#define DISPLAY_MENU_CHANGED    11
#define DISPLAY_MENU_WAITKEY    12
        switch( display ) {
            case DISPLAY_NORM_INIT:
                display = DISPLAY_NORM_WAITKEY;
                lcd_clrscr();
                lcd_gotoxy(1, 0);
                lcd_puts(cur_amps);
            case DISPLAY_NORM_WAITKEY:
                if( keys & ( 1<<KEY0 )) {
                    cur_mode ^= 1;
                    PORTA ^= (1<<PA0);
                    // lastline();
                }
                if( get_key_long( 1<<KEY1 )){  
                    // TODO: aktuellen wert modifizieren
                }
                if( keys & ( 1<<KEY2 )) {
                    display = DISPLAY_MENU_INIT;
                }
                
            break;
            case DISPLAY_MENU_INIT:
                lcd_clrscr();
                menuepos = 0;
                display = DISPLAY_MENU_CHANGED;
            case DISPLAY_MENU_CHANGED:
                draw_menue( menuepos );
                display = DISPLAY_MENU_WAITKEY;
            case DISPLAY_MENU_WAITKEY:
                if( keys & ( 1<<KEY0 )) {
                    menuepos = menue_prev( menuepos );
                    display = DISPLAY_MENU_CHANGED;
                }
                if( keys & ( 1<<KEY2 )) {
                    menuepos = menue_next( menuepos );
                    display = DISPLAY_MENU_CHANGED;
                }
                /* short/long wg debug ausser betrieb
                if( get_key_short( 1<<KEY1 )){  
                    // TODO: menuefkt aufrufen
                }
                if( get_key_long( 1<<KEY1 )) display = DISPLAY_NORM_INIT;
                */
                if( keys & ( 1<<KEY1 )) {
                    display = DISPLAY_NORM_INIT;
                }
            break;
            default:
                display = DISPLAY_NORM_INIT;
            break;
        }
        //lastline();
        _delay_ms(100);
    }

[Moritz A. (moritz_a)]

So, und jetzt bin ich wieder am Punkt der Ratlosigkeit. Eigentlich 
wollte ich nur die untere Displayzeile wieder von Debugausgaben 
befreien, und da verhält es sich, als ob mindestens KEY0 und KEY2 
gedrückt werden:

--- main.c.tut  2013-10-08 16:32:07.097961609 +0000
+++ main.c  2013-10-08 16:32:36.750330709 +0000
@@ -93,8 +93,8 @@
     while(1) {
         uint8_t keys;
         keys = get_key_press(0xff);
-        lcd_gotoxy(0,1);
-        lcd_itoa(keys);
+//        lcd_gotoxy(0,1);
+//        lcd_itoa(keys);
 
         // menue system
 #define DISPLAY_NORM_INIT   0
@@ -112,7 +112,7 @@
                 if( keys & ( 1<<KEY0 )) {
                     cur_mode ^= 1;
                     PORTA ^= (1<<PA0);
-                    // lastline();
+                    lastline();
                 }
                 if( get_key_long( 1<<KEY1 )){  
                     // TODO: aktuellen wert modifizieren
@@ -152,7 +152,7 @@
                 display = DISPLAY_NORM_INIT;
             break;
         }
-        //lastline();
+        lastline();
         _delay_ms(100);
     }
 }

[Moritz A. (moritz_a)]

Ich konnte das Auftreten des Fehlers auf die Zeile

lcd_puts(" OFF  W");

reduzieren.

Ist sie auskommentiert, tut alles – ist sie drin, habe ich den Fehler. 
Nur schlauer macht mich das auch nicht :/

    while(1) {
        // menue system
#define DISPLAY_NORM_INIT   0
#define DISPLAY_NORM_WAITKEY    1
#define DISPLAY_MENU_INIT       10
#define DISPLAY_MENU_CHANGED    11
#define DISPLAY_MENU_WAITKEY    12
        switch( display ) {
            case DISPLAY_NORM_INIT:
                display = DISPLAY_NORM_WAITKEY;
                lcd_clrscr();
                lcd_gotoxy(1, 0);
                lcd_puts(cur_amps);
            case DISPLAY_NORM_WAITKEY:
                if( get_key_press ( 1<<KEY0 )) {
                    cur_mode ^= 1;
                    PORTA ^= (1<<PA0);
                    lastline();
                }
                if( get_key_long( 1<<KEY1 )){  
                    // TODO: aktuellen wert modifizieren
                }
                if( get_key_press ( 1<<KEY2 )) {
                    display = DISPLAY_MENU_INIT;
                }
                
            break;
            case DISPLAY_MENU_INIT:
                lcd_clrscr();
                menuepos = 0;
                display = DISPLAY_MENU_CHANGED;
            case DISPLAY_MENU_CHANGED:
                draw_menue( menuepos );
                display = DISPLAY_MENU_WAITKEY;
            case DISPLAY_MENU_WAITKEY:
                if( get_key_press ( 1<<KEY0 )) {
                    menuepos = menue_prev( menuepos );
                    display = DISPLAY_MENU_CHANGED;
                }
                if( get_key_press ( 1<<KEY2 )) {
                    menuepos = menue_next( menuepos );
                    display = DISPLAY_MENU_CHANGED;
                }
                if( get_key_short( 1<<KEY1 )){  
                    // menu item action
                }
                if( get_key_long( 1<<KEY1 )) display = DISPLAY_NORM_INIT;
            break;
            default:
                display = DISPLAY_NORM_INIT;
            break;
        }
        lastline();
        _delay_ms(100);
    }
}

void lastline() {
    lcd_gotoxy(1, 1);
    
    // zwischen volt und watt alle sekunde wechseln
    if ( ( toggletimer & (1<<TT1s) ) == ( 1<<TT1s ) ) {
        lcd_puts(cur_volt);
    } else {
        if ( ( cur_mode & 1 ) == 0 ) {
        //    lcd_puts(" OFF  W");
        } else {
            lcd_puts(cur_watt);
        }
    }
}

[Bernd N (Gast)]

Nicht genügend RAM und im LCD Code kein progmem verwendet ? dann hat man 
auch  schöne Seiteneffekte...

http://www.nongnu.org/avr-libc/user-manual/pgmspace.html

Laß mal den LCD code sehen.

[Peter D. (peda)]

uint8_t key_state;
uint8_t key_press;
uint8_t key_rpt;

Mach mal diese 3 Variablen volatile. Der Code wurde mit einem älteren 
WINAVR entwickelt.
Neuere GCC machen ein sehr agressives Inlining und optimieren daher bei 
kleinen Programmen schnell mal wichtige Zugriffe weg.

Die neueren AVRs können auch für T0 den CTC-Mode, d.h. das Reload im 
Interrupt kann man dadurch ersetzen.

Ich sehe grad, Dein LCD benutzt Pins vom Tastenport. Vielleicht ist da 
was unsauber und die Tastenpins werden von der LCD-Lib geändert.

[Peter D. (peda)]

Bernd N schrieb:
> Nicht genügend RAM und im LCD Code kein progmem verwendet ? dann hat man
> auch  schöne Seiteneffekte...

Stimmt, beim Attiny261 muß man sparsam mit RAM umgehen, 128Byte sind 
schnell weg.

Obs daran liegt, kann man mit einem Attiny861 leicht testen.

[Moritz A. (moritz_a)]

Bernd N schrieb:
> Laß mal den LCD code sehen.

Das ist die LCD-Library von 
http://homepage.hispeed.ch/peterfleury/avr-software.html

Peter Dannegger schrieb:
> uint8_t key_state;
> uint8_t key_press;
> uint8_t key_rpt;
>
> Mach mal diese 3 Variablen volatile. Der Code wurde mit einem älteren
> WINAVR entwickelt.
> Neuere GCC machen ein sehr agressives Inlining und optimieren daher bei
> kleinen Programmen schnell mal wichtige Zugriffe weg.

Hat es zumindest auf den ersten Blick verbessert, ausführlich testen ist 
dann wohl erst morgen.

> Die neueren AVRs können auch für T0 den CTC-Mode, d.h. das Reload im
> Interrupt kann man dadurch ersetzen.

Ich lasse fremden Code meistens erstmal so lange in Ruhe, bis meiner 
sauber tut. Danach kann man immernoch aufräumen. Aber genau den Punkt 
hatte ich mir noch für später vorgenommen.

> Ich sehe grad, Dein LCD benutzt Pins vom Tastenport. Vielleicht ist da
> was unsauber und die Tastenpins werden von der LCD-Lib geändert.

Da sah der Code auf den ersten Blick sauber aus. Komplett auf PB geht 
leider nicht, da ich einen OC1x noch brauche, und auf PA brauche ich 
3xADC.

Peter Dannegger schrieb:
> Obs daran liegt, kann man mit einem Attiny861 leicht testen.

Ich hätte jetzt gedacht dass einen gcc hier warnt, der sollte doch 
wissen wann ihm der RAM ausgeht?

Aber der Code kann noch ein paar __flash vertragen, darauf hatte ich 
aufgrund obiger Annahme noch nicht so viel Wert gelegt, erstmal wollte 
ich den grundlegenden Programmfluss hinbekommen.

Ich habe nur noch einen 461 da, aber der hat ja auch erstmal etwas mehr.

[Bernd N (Gast)]

Yep, bin mal über den Code geflogen und ich denke dein RAM ist aus. 
Deine lcd_puts Funktion schreibt ins RAM, ergo, weniger text = weniger 
RAM, siehe der Fehler ist weg.

[Claude M. (stoner)]

Ja, der Hinweis mit dem Memory ist sehr gut. Ich hatte wegen Memory 
Problemen auch schon sehr ähnliche Probleme (schon das Einfügen von i++ 
auf einer nicht verwendeten Variablen hatte seltsame Nebeneffekte). Da 
war die Ursache allerdings ein fehlerhafter Zugriff auf das Flash.

Moritz A. schrieb:

> Ich hätte jetzt gedacht dass einen gcc hier warnt, der sollte doch
> wissen wann ihm der RAM ausgeht?

Nein, das kann er gar nicht. Der Compiler kann nur prüfen ob dein 
Programm in den Flash Speicher passt. Beim RAM sind die Möglichkeiten 
sehr begrenzt. Dazu müsste er ja wissen welchen Pfad dein Programm zur 
Laufzeit nehmen wird.

[Moritz A. (moritz_a)]

Claude M. schrieb:
>> Ich hätte jetzt gedacht dass einen gcc hier warnt, der sollte doch
>> wissen wann ihm der RAM ausgeht?
>
> Nein, das kann er gar nicht. Der Compiler kann nur prüfen ob dein
> Programm in den Flash Speicher passt. Beim RAM sind die Möglichkeiten
> sehr begrenzt. Dazu müsste er ja wissen welchen Pfad dein Programm zur
> Laufzeit nehmen wird.

So mit Abstand betrachtet, ja, so lange nicht alles mit globalen 
Variablen gemacht wird könnte das schwer werden.

[Moritz A. (moritz_a)]

So, ich greife den Thread wieder auf, ich habe das Problem dass er mir 
offensichtlich noch immer zu viel wegoptimiert:

void lastline() {
    static uint8_t state;
    lcd_gotoxy(0, 1);
    state = get_key_press(1<<KEY0);
    lcd_itoa2(state);
    if (state) {
        cur_mode ^= CUR_MODE_IS_ACTIVE;
        LED_PORT ^= (1<<LED_PIN);
    }
}
uint8_t display_norm_waitkey( void ){
     lcd_gotoxy(0, 0);
     lcd_puts_P("wait");
     uint8_t state;
     state = get_key_press( 1<<KEY0 );
     lcd_gotoxy(0, 1);
     lcd_itoa2(state);
     if (state) {
 //    if( get_key_press ( 1<<KEY0 )) {
         cur_mode ^= CUR_MODE_IS_ACTIVE;
         LED_PORT ^= (1<<LED_PIN);
         lcd_clrscr();
         _delay_ms(500);
     }
     return DISPLAY_NORM_WAITKEY;
}

Ich rufe wahlweise lastline() (da bringe ich immer mal wieder Debugausgaben drin unter) _oder_ display_norm_waitkey() auf.

Nach meinem Verständnis müsste bei beiden Funktionen bei einem Druck von KEY0 die LED getoggelt werden.

lastline() funktioniert immer, display_norm_waitkey() nur wenn ich -Os auf -O3 abändere.

Peter Dannegger schrieb:
> uint8_t key_state;
> uint8_t key_press;
> uint8_t key_rpt;

Die 3 sind volatile, zusätzlich bin ich auf einen atmega328 umgestiegen 
um erstmal während der Entwicklung keinen Ärger mehr mit dem RAM zu 
haben.

Grüße
Moritz

[Peter D. (peda)]

Codepfitzelchen ohne Kontext sind schlecht zu debuggen.
Wozu gibts Dateianhang.
Und was Du mal auskommentiert hast, wollen wir nicht wissen, lösche es.

->     lcd_itoa2(state);
     if (state) {
 //    if( get_key_press ( 1<<KEY0 )) {
         cur_mode ^= CUR_MODE_IS_ACTIVE;
         LED_PORT ^= (1<<LED_PIN);
->         lcd_clrscr();

Anzeigen und sofort löschen, kein Wunder, daß Du nichst siehst.

Ob was wegoptimiert wird, siehst Du schnell im Listing.

[Peter D. (peda)]

Moritz A. schrieb:
> _delay_ms(500);

Sowas läuft bei mir unter extrem gewaltiges Delay. Da muß man sehr 
vorsichtig sein, daß einem sowas nicht den gesamten Ablauf zerstört.

[Moritz A. (moritz_a)]

Peter Dannegger schrieb:
> Anzeigen und sofort löschen, kein Wunder, daß Du nichst siehst.

Im Normalfall würde ich es sehen, aber falls der Tastendruck erkannt 
wird sehe ich es nicht. Auch eine Form von Anzeige, wenn auch vielleicht 
nicht die intuitivste.

> Ob was wegoptimiert wird, siehst Du schnell im Listing.

Das habe ich mittlerweile auch gefunden. Ich bemühe mich morgen nochmal 
alles sauber ohne auskommentierten Kruscht aufzubereiten und dann 
komplett hochzuladen.

Peter Dannegger schrieb:
>> _delay_ms(500);
>
> Sowas läuft bei mir unter extrem gewaltiges Delay. Da muß man sehr
> vorsichtig sein, daß einem sowas nicht den gesamten Ablauf zerstört.

Das sind im Normalfall 50ms, hier nur beim Debugging so hochgedreht.

Grüße
Moritz

[Juergen G. (jup)]

Moritz A. schrieb:
> lcd_puts_P("wait");

Bist Du Dir sicher das der Compiler damit das macht was Du moechtest?

Ich meine das sollte so aussehen

lcd_puts_P(PSTR("wait"));

[Moritz A. (moritz_a)]

Juergen G. schrieb:
> Bist Du Dir sicher das der Compiler damit das macht was Du moechtest?

Bin ich, siehe 
http://homepage.hispeed.ch/peterfleury/group__pfleury__lcd.html#gd33c0d74b983e3f2c36f3278a462be15

#define lcd_puts_P(__s)         lcd_puts_p(PSTR(__s))

[Juergen G. (jup)]

oops,

dann habe ich das mit der lcd_puts_p() verwechselt, das macro ist mir 
noch nie aufgefallen.

[Moritz A. (moritz_a)]

So, jetzt mal mit aktualisiertem Programm.

In der angehängten Form bekomme ich komplett unreproduzierbare 
Ergebnisse, eigentlich sollte ich mit KEY0 die LED toggeln sowie mit 
KEY2 ins Menü (DISPLAY_MENU_INIT) springen.

Statdessen ist KEY0 ohne Funktion, und mit KEY2 lande ich in einem 
komplett kaputten Zustand (teilweise anzeige von DISPLAY_VAL, dann 
veränderte Werte in val_cur).

Mit auskommentiertem Aufruf von lastline() in Z. 106 funktioniert alles 
soweit ich es überblicken kann, nach den alten Symptomen würde ich 
wieder von zu wenig RAM ausgehen.

Dafür dass das Programm noch fast auf einem attiny261 lief, kommt es mir 
nun sehr seltsam vor, dass es auf dem atmega328p solche Probleme macht.

Pinbelegung wurde natürlich leicht angepasst:
Schalter an PD2, PD4, PD7
LCD an PB0-PB4, PC0, PC1
LED an PB5.

[Claude M. (stoner)]

Nachdem du nun auf den atmega328p aufgerüstet hast, würde ich schon fast 
ausschliessen, dass es an zu wenig RAM liegt. Trotzdem tönt das nach 
einem Memory Problem. Und zwar nach genau dem, das ich weiter oben mal 
erwähnt habe:

Claude M. schrieb:
> Ja, der Hinweis mit dem Memory ist sehr gut. Ich hatte wegen Memory
> Problemen auch schon sehr ähnliche Probleme (schon das Einfügen von i++
> auf einer nicht verwendeten Variablen hatte seltsame Nebeneffekte). Da
> war die Ursache allerdings ein fehlerhafter Zugriff auf das Flash.

Ich habe nochmals kurz über Deinen Code geschaut und bin mir nun zu 99% 
sicher, dass das Problem darin liegt, dass du Datenstrukturen im Flash 
anlegst und diese dann falsch ausliest. So z.B.

const char __flash val_unit[3] = { 'A', 'V', 'W' };
...
lcd_uint16_unit( val_set[  v  ], val_unit[ v ] );

oder

const uint16_t __flash powten[] = { 10000, 1000, 100, 10, 1 };
...
val_set[ (cur_mode & CUR_MODE_CONST_MASK) >> CUR_MODE_CONST_SHIFT ] += powten[*mp-1];

oder

const MENU_ENTRY __flash menue_b[] = {
  { get_target, 3, 1,       set_target, VAL_AMPS }, 
  { get_target, 0, 2,       set_target, VAL_VOLT }, 
  { get_target, 1, 3,       set_target, VAL_WATT }, 
  { get_config_menue, 2, 0, config_menue, VAL_NONE }, 
};
...
VoidFnct Funktion = (VoidFnct) menue_b[pos].tp;
Funktion( menue_b[pos].arg );

Dein Memory-Zugriff liefert so völlig wirre Ergebnisse und die wirken 
sich beim letzten Beispiel genau so aus, wie du es beschireben hast 
(falsche Funktionen werden aufgerufen, etc.).

Entweder du arbeitest ohne Flash oder liest im AVR-GCC-Tutorial im 
Kapitel "Programmspeicher (Flash)" nach wie man das richtig macht. Da du 
mit dem atmega328p ja nun 'massig' RAM hast, würde ich versuchsweise / 
vorübergehend mal auf Flash verzichten (also überall __flash 
rauslöschen).

Edit: Mit 'überall' meine ich natürlich nur da, wo es nachher falsch 
verwendet wird (siehe Beispiele oben). Da wo es sich um Strings handelt, 
die nachher mit puts_p auf das Display geschrieben werden, ist das Flash 
schon ok.


Viele Grüsse
Claude

[Moritz A. (moritz_a)]

Claude M. schrieb:
> Ich habe nochmals kurz über Deinen Code geschaut und bin mir nun zu 99%
> sicher, dass das Problem darin liegt, dass du Datenstrukturen im Flash
> anlegst und diese dann falsch ausliest. So z.B.
>
> const char __flash val_unit[3] = { 'A', 'V', 'W' };
> ...
> lcd_uint16_unit( val_set[  v  ], val_unit[ v ] );
>
> oder
>
> const uint16_t __flash powten[] = { 10000, 1000, 100, 10, 1 };
> ...
> val_set[ (cur_mode & CUR_MODE_CONST_MASK) >> CUR_MODE_CONST_SHIFT ] +=
> powten[*mp-1];


Im Artikel unter "__flash, progmem und Portierbarkeit" findet sich das 
Beispiel

static const __flash char x[] = { 'A', 'V', 'R' };
 
char foo (void)
{
    return x[2];
}

Hier sehe ich beim besten Willen keinen Unterschied im Zugriff.

Für menue_b finde ich jetzt so kein 1:1 übertragbares Beispiel, generell 
ist die Dokumentation zu __flash etwas rar gesät. Ich teste jetzt aber 
erstmal nur mit RAM, wie vorgeschlagen.

Grüße
Moritz

[Moritz A. (moritz_a)]

Eigentlich sind nun alle __flash aufgeräumt bis auf die für lcd_puts_p:

diff --git a/main.h b/main.h
index fc82195..b1f37b9 100644
--- a/main.h
+++ b/main.h
@@ -28,9 +28,9 @@ uint8_t display_val_waitkey(  uint8_t *mp);
 uint16_t val_cur[3] = { 0xffff, 0xffff, 0xffff };
 uint16_t val_set[3] = { 0x0000, 0x0000, 0x0000 };
 
-const char __flash val_unit[3] = { 'A', 'V', 'W' };
+const char val_unit[] = { 'A', 'V', 'W' };
 
-const uint16_t __flash powten[] = {
+const uint16_t powten[] = {
     10000,
      1000,
       100,
diff --git a/menu.c b/menu.c
index d4bd293..57779eb 100644
--- a/menu.c
+++ b/menu.c
@@ -26,7 +26,7 @@ typedef struct MENU {
 } MENU_ENTRY; 
 
 
-const MENU_ENTRY __flash menue_b[] = {
+const MENU_ENTRY menue_b[] = {
   { get_target, 3, 1,       set_target, VAL_AMPS }, 
   { get_target, 0, 2,       set_target, VAL_VOLT }, 
   { get_target, 1, 3,       set_target, VAL_WATT }, 


-> ack-grep __flash *h *c
lcd.h    
251:extern void lcd_customchar(const uint8_t addr, const unsigned __flash char *data);

main.h
8:static const char __flash p_off[] = " -.-- ";

lcd.c
471:void lcd_customchar(const uint8_t addr, const unsigned __flash char *data)

-> ack-grep p_off *h *c
main.h
8:static const char __flash p_off[] = " -.-- ";

main.c
17:        lcd_puts_p( p_off );

Das Fehlerbild ist aber absolut gleich geblieben.

Grüße
Moritz

P.S.: Ich weiß nicht was bei Änderungen die bevorzugte Form ist, diff 
oder die Datei nochmal komplett, daher habe ich einfach mal beides 
geliefert.

[Peter D. (peda)]

Moritz A. schrieb:
> diff
> oder die Datei nochmal komplett

Also mit diff kann ich nichts anfangen, ich muß immer den Kontext sehen. 
Die (eine) Datei hilft mir auch nicht viel.

Am besten immer das gesamte Projekt gezipt, dann kann man es durch den 
Compiler jagen und das Listing ansehen.

Mit 2kB RAM kann man schon ne Menge anfangen. Flash-Variablen würde ich 
daher nur da nehmen, wo ich absolut sicher bin, daß es funktioniert.

[W.S. (Gast)]

Moritz A. schrieb:
> ich bin gerade etwas am Verzeifeln,

Ja, ich verZeifele auch beim Versuch deine main-Quelle zu lesen. Junge, 
SO wird das nix außer Spaghetti mit Sauerkraut. Die CPU rast ständig 
durch den switch-Block und zerrt auch ständig am LCD herum. So macht man 
das nicht, wenn man sich nicht mit Gewalt graue Haare zulegen will.

Also, trenne um Gotteswillen bloß die verschiedenen Programmteile, etwa 
so:
bool IstNeTasteGedrückt(void) {.. code hier rein ..}
int  LiefereTastenCode(void) {.. code hier rein ..}
void ZeichneDisplayGanzNeu(void) {.. eben LCD setzen ..}

main (...)
{ int i;

 alle Initialisierungen tun..

immerzu:
  if IstNeTasteGedrückt()
  { i = LiefereTastenCode()
    switch (i)
    { case rauf:   MachCursorRauf(); break;
      case runter: MachCursorRunter(); break;
      ...usw
    }
   ZeichneDisplayGanzNeu();
   i = 1000;
   while (i)
   { --i;
     if (IstTasteGedrückt()) i = 1000;
   }
   goto immerzu;
}

Hierbei sollen die "MachCursorRauf" usw. nur den inneren Zustand des 
Systems setzen, aber NICHT bereits selber im Display herumkratzen. Das 
macht dann zum Schluß nur die eine Routine, die dafür zuständig ist - 
und sonst hat kein anderer programmteil am LCD was verloren.

Also: Gewöhne dir so bald wie möglich eine weniger chaotische 
Programmierweise an. Sonst kriegst du graue Haare..

W.S.

[Moritz A. (moritz_a)]

Peter Dannegger schrieb:
> Am besten immer das gesamte Projekt gezipt, dann kann man es durch den
> Compiler jagen und das Listing ansehen.

Ich kann auch gerne gleich das main.lss mit Anhängen, der Gedanke ist es 
euch so komfortabel wie möglich zu machen.

Aber dann behalte ich das mit dem zip bei.

> Mit 2kB RAM kann man schon ne Menge anfangen. Flash-Variablen würde ich
> daher nur da nehmen, wo ich absolut sicher bin, daß es funktioniert.

Im Moment habe ich noch genau 3 Stück im Code, die alle mit lcd_puts_p 
verwendet werden – und auch korrekt auf dem LCD landen.

W.S. schrieb:
> Ja, ich verZeifele auch beim Versuch deine main-Quelle zu lesen. Junge,
> SO wird das nix außer Spaghetti mit Sauerkraut. Die CPU rast ständig
> durch den switch-Block und zerrt auch ständig am LCD herum. So macht man
> das nicht, wenn man sich nicht mit Gewalt graue Haare zulegen will.

Genau diese Trennung habe ich doch:
DISPLAY_<mode>_INIT:
DISPLAY_<mode>_CHANGED:
DISPLAY_<mode>_WAITKEY:

<mode> ist hierbei NORM, MENU oder VAL.

_INIT initialisert/setzt die Statusvariablen passend für diesen Modus,
_CHANGED gibt das LCD neu aus,
_WAITKEY wartet auf eine Tasteneingabe.

Bei einem Moduswechsel wird _INIT, _CHANGED, _WAITKEY aufgerufen, und ab 
dann nur noch _WAITKEY bei jedem Schleifendurchlauf.

Sobald in _WAITKEY etwas geändert wird, wird 'display' auf _CHANGED 
gesetzt, so dass beim kommenden Schleifendurchlauf einmal die Werte neu 
aufs LCD geschrieben werden, und ab dann geht es wieder weiter mit nur 
noch _WAITKEY.

Bei NORM ist das noch etwas verwaschen, da gibt es kein _CHANGED, da 
der Wert nur in VAL gesetzt werden kann, und damit beim <mode>-Wechsel 
sowieso wieder ein _INIT-Durchlauf ansteht.

Die Tastenabfrage könnte ich noch auslagern und den einzelnen Funktionen 
als Argument mit übergeben, aber ob ich da jetzt ein

if( get_key_press ( 1<<KEY0 )) {
    *mp = menue_prev( *mp );
    return DISPLAY_MENU_CHANGED;
}
if( get_key_press ( 1<<KEY2 )) {
    *mp = menue_next( *mp );
    return DISPLAY_MENU_CHANGED;
}
if( get_key_short( 1<<KEY1 )){
    // TODO: menu item action
}
if( get_key_long( 1<<KEY1 )) return DISPLAY_NORM_INIT;
return DISPLAY_MENU_WAITKEY;

oder ein

switch (pressed_key) {
PRESS_KEY0:
    *mp = menue_prev( *mp );
    return DISPLAY_MENU_CHANGED;
break;
PRESS_KEY2:
    *mp = menue_next( *mp );
    return DISPLAY_MENU_CHANGED;
break;
PRESS_KEY1_SHORT:
    // TODO: menu item action
break;
PRESS_KEY1_LONG:
    return DISPLAY_NORM_INIT;
break;
default:
    return DISPLAY_MENU_WAITKEY;
break:
}

habe macht den Code erstmal nicht viel anders. Ich würde mir eine 
elseif-Orgie, die da eigentlich korrekterweise statt den sequentiellen 
ifs hinsollte,  sparen. Aber in der Übersichtlichkeit ist das in etwa 
gleich.

Die Variante mit 'display' als Return-Wert habe ich so absichtlich 
gewählt, da der Compiler hiermit mich nettweise direkt warnt, wenn ich 
in meiner State-Machine vergesse ein Ziel für den nächsten Durchlauf 
anzugeben.

Hast du dir eventuell den alten Code ganz oben angesehen?

Anbei nochmal der komplette Code als Zip + .lss

Grüße
Moritz

[W.S. (Gast)]

Moritz A. schrieb:
> Hast du dir eventuell den alten Code ganz oben angesehen?

Logo.

Aber ich fühle mich dennoch unverstanden. Hast du an meinem schnell 
skizzierten Beispiel gemerkt, daß dort die Tastenentprellung bereits 
enthalten ist? Daß IstNeTasteGedrückt() und LiefereTastenCode() sich 
überhaupt nicht um irgendwelches Entprellen kümmern müssen?

Ansonsten arbeite ich bei meínen Projekten völlig anders als du. Bei mir 
erzeugen Tastendrücke Events, die zunächst (nebst anderen) in eine 
Event-Warteschlange gelangen und von dort aus von der Grundschleife, die 
als Dispatcher fungiert, ins Menüsystem eingespeist werden, wo sie dann 
von den Programmteilen, die sich dafür interessieren, zur Kenntnis 
genmmen werden.

Moritz A. schrieb:
> Genau diese Trennung habe ich doch:
> DISPLAY_<mode>_INIT:
> DISPLAY_<mode>_CHANGED:
> DISPLAY_<mode>_WAITKEY:

Nee, was ich meine und oben skizziert habe, hast du eben nicht. Lies 
einfach nochmal und schau, wo man Funktionalitäten sinnvoll 
voneinander trennt. Deine Teilungen (Display(dies), Display(das), 
Display(jenes)) sind es nicht.

Ich versteh ja, wenn man jung ist und erstmal drauflosprogrammieren 
will, daß man dann noch nicht die Ruhe hat, sich ne sinnvolle 
Systemarchitektur auszudenken. Ich weiß jetzt auch wirklich nicht, ob 
ich dir nen Blick in die "Lernbetty" hier in den Sedimenten der 
Codesammlung anraten soll, denn angesichts deiner Antwort auf meinen 
Vorschlag befürchte ich, daß dich das nur erschlägt. Aber: es ist 
wirklich vom ganz generellen Ansatz her genau DIE Lösung - auch für 
andere Systeme als ARM7TDMI, wenn man mal von der alleruntersten 
hardwareabhängigen Ebene (serieller Treiber, Tastaturtreiber, ScrBlt und 
so) absieht:
- Ereignisse entstehen in den diversen Peripherien
- entstandene Ereignisse werden zwischengepuffert
- kein Programmteil wartet auf irgendeine Bedingung und vertrödelt damit 
Zeit
- gepufferte Ereignisse werden nicht wirklich im Grundsystem behandelt, 
sondern einem Menüsystem "zum Fressen gegeben"
- auch das Ereignis "Der Bildschirm sollte mal wieder aufgefrischt 
werden" ist nur ein Ereignis, was allerdings nicht Obliegenheit des 
Menüsystems ist, sondern des Bildschirm-hardwaretreibers.
- nichts ist prozedural programmiert, sondern ereignisgesteuert.
Naja, vielleicht guckst du trotzdem dir mal die Lernbetty an, mehr als 
daß du nix damit anfangen kannst, kann ja nicht passieren.

W.S.

[Moritz A. (moritz_a)]

W.S. schrieb:
> Aber ich fühle mich dennoch unverstanden. Hast du an meinem schnell
> skizzierten Beispiel gemerkt, daß dort die Tastenentprellung bereits
> enthalten ist?

Ich glaube den jetzt vorhandenen Code unter diesen Aspekten 
durchzudiskutieren geht am eigentlichen Problem vorbei, daher würde ich 
das (für den Moment und hier im Thread) erstmal gerne lassen. Es gibt 
auch noch deutlich schlimmeren Code, der 1a läuft ;)

> Naja, vielleicht guckst du trotzdem dir mal die Lernbetty an, mehr als
> daß du nix damit anfangen kannst, kann ja nicht passieren.
Ich habe mir Beitrag "Die Lernbetty: Die SwissBetty von Pollin als ARM-Evalboard" mal 
abgespeichert, um es unabhängig von diesem Projekt einmal näher 
anzusehen, und bei Gelegenheit durchzuarbeiten. Jetzt anzufangen einen 
nicht vorhandenen Code auf ein noch nicht ganz durchdrungenes Schema 
umzustellen macht das Debuggen für mich vermutlich nicht einfacher.

Leider ist ordentliches Programmieren etwas, was man weder in der 
Fachinformatiker-Ausbildung noch (bisher) im E-Technik-Studium wirklich 
lernt, daher bin ich an sich für Input an sich immer dankbar.

Grüße
Moritz

[Claude M. (stoner)]

Moritz A. schrieb:

> Hier sehe ich beim besten Willen keinen Unterschied im Zugriff.

Doch einen Unterschied gibt es schon. Du greifst mit val_unit[v] zu, im 
Beispiel wird mit x[2] zugegriffen (also mit konstantem Index). Somit 
wird im Beispiel x[2] durch den Compiler durch 'R' ersetzt und es wird 
nichts im Flash abgelegt. Darum geht es im Beispiel auch.

Wie auch immer, das war ja offenbar nicht das Problem - oder zumindest 
hat es mein Vorschlag nicht gelöst :-(

Wenn ich in den nächsten Tagen mal ein paar Minuten Zeit finde, so werde 
ich die Schlatung selbst mal aufbauen und versuchen das Ganze zu 
reproduzieren...

[Peter D. (peda)]

Vielleicht solltest Du erstmal einen Programmablaufplan erstellen.
Ich sehe da nicht durch, was das Programm machen soll.

Und die vielen LCD-Aufrufe machen es nicht nur unübersichtlich und 
langsam, sondern kosten auch Unmengen an Code.

Mal mal auf, was alles auf dem LCD dargestellt werden soll. Und dann 
überlege, wie man das mit einer einzigen Funktion (oder wenigen) 
erschlagen kann.

Und keine unsinnigen Delays (besser gar keine):

int main (void) {
...
    while(1) {
...
        _delay_ms(50);       <- weg damit, hat hier nichts verloren !!!

[Peter D. (peda)]

KEY0,1,2 sind keine sinnvollen Namen.
Du wirst ihnen ja eine bestimmte Funktion zugeordnet haben und sie nicht 
ständig anders verwenden.
Z.B. KEY_ENTER, KEY_UP, KEY_DOWN

[Karl H. (kbuchegg)]

Ich denke so ungefähr hab ich den Code verstanden.

Was soll es denn in Summe werden? Irgendwie kommt mir das, was ich da 
lese alles deutlich zu kompliziert aufgebaut vor.
Wenn ich diesen Code bekommen würde und vor dem Problem stehe, dass es 
da seltsame Effekte gibt, würde ich ehrlich gesagt mir überlegen ob ich 
das nicht alles neu schreibe. So wie ich das sehe, geht es um 3 
Variablen, die per Tastendruck in ihren Werten einstellbar sein sollen.
Diese komplizierte Einstellerei mit den einzelnen Stellen würde ich gar 
nicht machen. Die PeDa Routinen können Autorepeat und teilen mir auch 
mit, wenn der Autorepeat sich einklinkt. D.h. beim Werteverändern mach 
ich den normalen Key press auf ein Inkrement/Dekrement von +-1 und wenn 
sich der Autorepeat dazuschaltet, dann erhöhe ich dieses Delta auf +-10. 
Das funktioniert sehr gut (überhaupt wenn man den Autorepeat dann noch 
ein wenig schneller stellt) und man braucht dieses ganze "'An welcher 
Position steht gerade der Cursor und welche Zehnerpotenz muss daher 
addiert/subtrahiert werden" gar nicht.
Völlig anderer Ansatz -> daher neu schreiben.
Ist aber meine subjektive Meinung.

Zum eigentlichen Problem im Code:
Da ist auf Anhieb nichts zu sehen, von den klassischen Fehlern konnte 
ich keinen entdecken. Was nicht heißt, dass es nicht so einer sein 
könnte. Das Problem ist, dass dein Code so 'verstreut' ist und ich 
irgendwie ständig den Faden verliere, wenn ich den lese.

[Karl H. (kbuchegg)]

Aber was anderes sehe ich noch.

Ist das Absicht, dass deine ganzen 'case' in der Hauptschleife jeweils 
zum nächsten durchfallen? Das ist für eine Statemaschine ungewöhnlich! 
Dazu hast du ja diese 'display' Variable (wobei ich mich ja frage, warum 
die display heißt. Mit dem LCD hat die ja gar nichts zu tun), die in den 
einzelnen Funktionen neue Werte kriegt. Das ist aber ein wenig witzlos, 
wenn du dann diesen neuen Wert ignorierst und den jeweiligen case 
durchfallen lässt.

[Claude M. (stoner)]

Karl Heinz Buchegger schrieb:
> Ist das Absicht, dass deine ganzen 'case' in der Hauptschleife jeweils
> zum nächsten durchfallen? Das ist für eine Statemaschine ungewöhnlich!

Ja, ich denke, das will er tatsächlich so. Er macht das ja nicht 
erratisch sondern mit System.

Das war mir gleich als Erstes ins Auge gesprungen, weil ich das sehr 
unschön fand. Ich bin dann aber zum Schluss gekommen, das der Code so 
grundsätzlich funktioniert.

[Karl H. (kbuchegg)]

Claude M. schrieb:
> Karl Heinz Buchegger schrieb:
>> Ist das Absicht, dass deine ganzen 'case' in der Hauptschleife jeweils
>> zum nächsten durchfallen? Das ist für eine Statemaschine ungewöhnlich!
>
> Ja, ich denke, das will er tatsächlich so. Er macht das ja nicht
> erratisch sondern mit System.

Ist mir auch aufgefallen.
Ist trotzdem nicht schlau. Denn dadurch hat er sich 'Wissen' aus den 
Funktionen in den switch-case geholt.

Das hier

           case DISPLAY_MENU_INIT:
                display = display_menu_init( &menuepos );
            case DISPLAY_MENU_CHANGED:
                display = display_menu_changed( &menuepos );
            case DISPLAY_MENU_WAITKEY:
                display = display_menu_waitkey( &menuepos );
            break;

weiß 'irgendwie' magisch, dass die Funktion display_menu_changed IMMER 
als Returnwert DISPLAY_MENU_WAITKEY zurückgeben wird.
Tja. das war gestern so, das war heute so und wahrscheinlich wird es 
auch morgen so sein. Aber wenn dann übermogen die Funktion geändert 
wird, so dass das nicht mehr so ist, dann sucht man sich einen Wolf, 
warum das alles nicht mehr funktioniert.

> Das war mir gleich als Erstes ins Auge gesprungen, weil ich das sehr
> unschön fand. Ich bin dann aber zum Schluss gekommen, das der Code so
> grundsätzlich funktioniert.

Ja, funktionieren müsste es. Ich denke auch nicht, dass das jetzt 
irgendwas mit seinem Problem zu tun hat. Es ist mir nur wie dir beim 
Codestudium irgendewann mal ins Auge gesprungen.

[Moritz A. (moritz_a)]

Peter Dannegger schrieb:
> Vielleicht solltest Du erstmal einen Programmablaufplan erstellen.
> Ich sehe da nicht durch, was das Programm machen soll.

Das ganze soll später mal eine Dummy Load werden, mit im Idealfall 
einstellbarem Strom/Spannungsabfall/Leistung.

Im derzeitigen Stadium sieht man davon noch nicht viel, sondern es macht 
primär 2 Dinge:

In der unteren LCD-Zeile werden im Wechsel alle Werte aus val_cur 
ausgegeben, wobei 0xffff für "Wert nicht bekannt/verfügbar" steht. Dafür 
wird lastline() einmal pro Schleifendurchlauf aufgerufen.

In der oberen Zeile findet die eigentliche Navigation statt, im groben 
ist der Benutzerablauf so (-> zeigt den Folgestate an).

DISPLAY_NORM_INIT:
  Aktion: Aktuell gesetzten Wert in gesetzem Modus (A/V/W) ausgeben.
  Default: -> DISPLAY_NORM_WAITKEY

DISPLAY_NORM_WAITKEY:
  Aktion: Auf Tastendruck überprüfen
    KEY0: an/aus toggeln -> DISPLAY_NORM_WAITKEY
    KEY1 lang: Wert ändern -> DISPLAY_VAL_INIT
    KEY2: Menü -> DISPLAY_MENU_INIT
  Default: -> DISPLAY_NORM_WAITKEY

DISPLAY_MENU_INIT:
  Aktion: setze Aktiven Menüpunkt auf 0
  Default: -> DISPLAY_MENU_CHANGED

DISPLAY_MENU_CHANGED:
  Aktion: Gewählten Menüpunkt ausgeben
  Default: -> DISPLAY_MENU_WAITKEY

DISPLAY_MENU_WAITKEY:
  Aktion: Auf Tastendruck überprüfen
    KEY0/KEY2: aktiven Menüpunkt in/dekrementieren -> 
DISPLAY_MENU_CHANGED
    KEY1 kurz: Menüpunktaktion, noch nicht implementiert
    KEY1 lang: zurück in Standardansicht -> DISPLAY_NORM_INIT
  Default: -> DISPLAY_MENU_WAITKEY

DISPLAY_VAL_INIT:
  Aktion: LCD-Cursor an zum anzeigen der aktuellen Position
          Aktuelle Position auf 1 initalisieren (10er-Stelle)
  Default: -> DISPLAY_VAL_CHANGED

DISPLAY_VAL_CHANGED:
  Aktion: Aktuell gesetzten Wert in gesetzem Modus (A/V/W) ausgeben,
          Cursor auf aktive Stelle
  Default: -> DISPLAY_VAL_WAITKEY

DISPLAY_VAL_WAITKEY:
  Aktion: Auf Tastendruck überprüfen
    KEY0/KEY2: aktive Stelle in/dekrementieren -> DISPLAY_VAL_CHANGED
    KEY1 kurz: aktive Stelle weiterrücken:
                 falls Zahl zuende -> DISPLAY_NORM_INIT
                 sonst -> DISPLAY_VAL_CHANGED
    KEY1 lang: zurück in Standardansicht -> DISPLAY_NORM_INIT
  Default: -> DISPLAY_VAL_WAITKEY

> Und die vielen LCD-Aufrufe machen es nicht nur unübersichtlich und
> langsam, sondern kosten auch Unmengen an Code.

Ich hatte mich initial absichtlich dagegen entschieden, die gewünschte 
LCD-Anzeige in einer Variable abzulegen, um den Platz im RAM zu sparen.

Wo es allerdings hierdurch langsamer wird ist mir nicht ganz klar, das 
LCD wird nur neu geschrieben wenn sich die Anzeige geändert hat und 
ansonsten unangetastet gelassen.

Der "Normalzustand" des Programms ist, dass es sich in einer der 
_WAITKEY-States befindet und nichts anderes macht, als zu überprüfen, ob 
eine Taste gedrückt wurde.

Claude M. schrieb:
> Karl Heinz Buchegger schrieb:
>> Ist das Absicht, dass deine ganzen 'case' in der Hauptschleife jeweils
>> zum nächsten durchfallen? Das ist für eine Statemaschine ungewöhnlich!
>
> Ja, ich denke, das will er tatsächlich so. Er macht das ja nicht
> erratisch sondern mit System.

Genau, vielleicht wird es oben etwas klarer. Karl-Heinz hat natürlich 
recht, ich sollte lieber das durchrutschen weglassen und im nächsten 
Schleifendurchlauf die Aktion durchführen, so dass die Logik in den 
einzelnen States selber bleibt.

Karl Heinz Buchegger schrieb:
> Dazu hast du ja diese 'display' Variable (wobei ich mich ja frage, warum
> die display heißt. Mit dem LCD hat die ja gar nichts zu tun), die in den
> einzelnen Funktionen neue Werte kriegt.

Der Name ist "historisch", sinnvoller wäre wirklich, sie state oder so 
zu nennen, genau so wie meine Konstanten zum anspringen der States und 
die Funktionen dazu.

Grüße
Moritz

[W.S. (Gast)]

Moritz A. schrieb:
> im groben
> ist der Benutzerablauf so (-> zeigt den Folgestate an)....

Sorry, aber selbst bei deiner nochmaligen Darstellung wird mir nicht 
klar, was du EIGENTLICH damit bezweckst.

Moritz A. schrieb:
> Ich hatte mich initial absichtlich dagegen entschieden, die gewünschte
> LCD-Anzeige in einer Variable abzulegen, um den Platz im RAM zu sparen.

Das macht man ja auch nicht so. Stattdessen sollte es eine Funktion 
geben, die das Display neu setzt und diese erzeugt den anzuzeigenden 
Text jedesmal neu - und zwar gemäß dem momentanen Stand einer 
Zustandsvariable, die beispielsweise folgende "Werte" enthalten könnte: 
zeig_U, zeig_I, zeig_R, zeig_Garnix, zeig_USOLL, na und so weiter. Also 
nicht ne Variable, die den Anzeigewert enthält, sondern eine, die nur 
angibt, was grad anzuzeigen ist.

Moritz A. schrieb:
> Der "Normalzustand" des Programms ist, dass es sich in einer der
> _WAITKEY-States befindet und nichts anderes macht, als zu überprüfen, ob
> eine Taste gedrückt wurde.

Man kann so programmieren, aber es ist schon ein bissel oberdoof, es 
heutzutage derart anzugehen. Bedenke mal, daß der µC bei solcher 
Programmierweise durchweg seine Rechenleistung fast komplett vergeigt, 
indem er nix macht, als nur dazustehen und auf ne Taste zu warten. Mach 
es anders, ereignisorientiert. Das schafft dir die Freiheit, 
zwischendurch auch noch was anderes zu machen, z.B. nen kleinen 
Hintergrund-Monitor per UART zu betreiben, um ggf. per PC im µC nach dem 
rechten zu sehen.

W.S.

[Moritz A. (moritz_a)]

W.S. schrieb:
> Mach es anders, ereignisorientiert.

Anderswo hier im Wiki liest man wieder, dass man Taster unter keinen 
Umständen an Interrupts hängen darf, da es beim unvermeidlichen Prellen 
zu bösen Nebeneffekten führt.

Wie ich asynchron ein Ereignis ohne Interrupts aufnehmen soll, da bin 
ich ehrlich gesagt überfragt.

W.S. schrieb:
> Stattdessen sollte es eine Funktion
> geben, die das Display neu setzt und diese erzeugt den anzuzeigenden
> Text jedesmal neu - und zwar gemäß dem momentanen Stand einer
> Zustandsvariable, die beispielsweise folgende "Werte" enthalten könnte:

Also du meinst, statt dass ich wie bisher lcd_uint16_unit den 
anzuzeigenden Wert und die anzuzeigende Einheit übergebe, soll ich mir 
eine globale Zustandsvariable anlegen/jeder Funktion einen Pointer 
mitgeben und in diese Variable irgendwelche Konstanten für den 
anzuzeigenden Wert schreiben?

Aus meiner Sicht bringt das im Moment nur eine zusätzliche 
Fallunterscheidung mehr in lcd_uint16_unit mit sich, aber keinen direkt 
sichtbaren Vorteil.

W.S. schrieb:
> Sorry, aber selbst bei deiner nochmaligen Darstellung wird mir nicht
> klar, was du EIGENTLICH damit bezweckst.

Sollwert-Anzeige
   +--> Sollwert  setzen
   '·-> Untermenü mit Auswahl:
          +--> Const-I-Modus aktivieren
          +--> Const-U-Modus aktivieren
          +--> Const-P-Modus aktivieren
          '·-> Config (nicht genauer spezifiziert)

Viel mehr gibt die Programmbedienung bisher nicht her.

Im Moment sind die Aktionen natürlich noch nicht alle implementiert, da 
ich den Code erstmal mit leeren Aktionen lauffähig haben wollte.

So langsam weiß ich nicht mehr, was ich zum dritten "verstehe ich nicht" 
noch schreiben soll, ohne konkrete Nachfragen

Moritz

[Juergen G. (jup)]

Moritz A. schrieb:
> W.S. schrieb:
>> Mach es anders, ereignisorientiert.
>
> Anderswo hier im Wiki liest man wieder, dass man Taster unter keinen
> Umständen an Interrupts hängen darf

Das muss nichts miteinander zu tun haben.

W.S. meint mit ereignisorientiert nicht einen Interrupt, sondern jedes 
X-beliebige Ereignis das eintreten kann.

Zum Display:

Die Display Variablen koennen eine Art Bildschirmpuffer sein.
Also wenn Du ein 16x2 Text LCD verwendest ist es ein

char lcdLines[2][16];

eine Funktion
void updateDisplay()
{
   if(updateDisplayFlag == TRUE)
   {
      schreibe_den_Inhalt_von_lcdLines_aufsDisplay
      updateDisplayFlag = FALSE;
   }
}

sendet dann aktuell den neuen Inhalt ans Display

das updateDisplayFlag kann global sein und gesetzt werden wenn sich der 
Inhalt von lcdLines geaendert hat, zyclisch 10 mal pro Sekunde oder so 
oder beides.

updateDisplay rufst Du dann in Deiner main-Endlosschleife auf.

Damit hast Du den Anzeige Teil Deines Programmes unabhaengig vom Rest.
Die Anderen Funktionen des Programmes schreiben dann nur in lcdLines 
hinein.

[Moritz A. (moritz_a)]

W.S. schrieb:
> Moritz A. schrieb:
>> Ich hatte mich initial absichtlich dagegen entschieden, die gewünschte
>> LCD-Anzeige in einer Variable abzulegen, um den Platz im RAM zu sparen.
>
> Das macht man ja auch nicht so.

vs.

Juergen G. schrieb:
> Die Display Variablen koennen eine Art Bildschirmpuffer sein.
> Also wenn Du ein 16x2 Text LCD verwendest ist es ein
>
> char lcdLines[2][16];

Ja was denn nun?

[Juergen G. (jup)]

Zu den Ereignissen.

Ereignisse koenne zBsp. in einem Array abgelegt werden


uint8_t eventQueue[MAX_EREIGNISSE]

uint8_t eventQueueReadPosition;
uint8_t eventQueueWritePosition;

void dispatchEvent(void)
{
   if (eventQueueReadPosition != eventQueueWritePosition)
   {
        callEventHandler();
   }

}

callEventHandler()

kann dann wie jedes andere Teil des Programmes Events in der Queue 
platzieren.

und dispatchEvent()

wird auch in der Hauptschleife aufgerufen

[Juergen G. (jup)]

Moritz A. schrieb:
> Ja was denn nun?

Das haengt von Dir ab.

Bei mir hat bis jetzt der "Display Puffer" immer Platz im Ram gefunden.
Du musst bei einem Text Display eh immer den String zusammenbauen den Du 
anzeigen willst und dazu brauchst Du Variablen.
Fuer eine Zeile, nur den Teil einer Zeile oder das gesamte Display.

Bei mir machen das Funktionen die da zBsp.

dsp_Write_MenuText(const char* txt);
dsp_Write_StatusText(const char* txt);

Wenn Du den String innerhalb der Funktion zusammenbaust und ans Display 
schickst liegt die Variable auf dem Heap (also nur fuer die Laufzeit der 
Funktion)
Wenn Du den String in der Funktion zusammenbaust und in die Globale 
Variable schreibst liegt sie immer im RAM an der gleichen Stelle.

Fuer mich hat die Globale variante den Vorteil, das jede Funktion 
beliebig in den DisplayPuffer schreiben kann, es aber immer Zyklisch 
angezeigt wird.

Wenn Ereignisse schneller als 10/s auftreten und in die globalen 
Variablen schreiben, dann geht der vorherige Inhalt zwar verloren, aber 
er ist dann warscheinlich auch nicht mehr aktuell.

Schneller als 70 mal pro Sekunde kann kein Mensch sehen und mehr als 10 
refresh's pro Sekunde auf dem Display kann kein Mensch lesen.

[Juergen G. (jup)]

Ergaenzung zum letzten Post.

Das mit der DisplayPuffer Variablen darf man nicht auf alle Variablen 
veralgemeinern.

Also die EventQueue zBsp. kann man nicht so behandeln.

Das mit dem Schreiben in den DisplayPuffer geht nur weil eine ISR zBsp. 
nie direkt in diesen Puffer schreibt, das Schreiben wird nur von den 
entsprechenden Funktionen gemacht.
Diese Funktionen koennen durch einen IRQ zwar unterbrochen werden, 
machen aber nach der ISR da weiter wo sie unterbrochen wurden.

[Peter D. (peda)]

W.S. schrieb:
> Mach
> es anders, ereignisorientiert.

Das macht er bereits.
Die Funktionen get_key_press, get_key_short, get_key_long liefern 
nämlich Ereignisse.

[Claude M. (stoner)]

Ich habe Dinge gesehen, schlimme Dinge...

Ich wollte eigentlich nicht auch noch mit damit anfangen Deinen Code zu 
zerpflücken, sonden mich auf die Lösung Deines eigentlichen Problems 
konzentrieren. Aber es hat da Dinge drin, die es nicht nur Dir und uns 
schwer machen, das Problem zu lösen, sondern auch Ursache für Probleme 
sein können.

Ich hatte heute morgen früh noch unverhofft eine Stunde Zeit, um Deinen 
Code in mein Eclipse rein zu ziehen, Deine Schaltung aufzubauen und zu 
testen. Leider musste ich nach dem Reinziehen des Codes so viel Zeit in 
Bereinigungsarbeiten investieren, damit Dein Code überhaupt compiliert, 
dass ich für den Rest keine Zeit mehr hatte. Der Grund dafür ist, dass 
du die Header-Files völlig falsch verwendest:

Verwendest du in einer C-Datei Typen, Variabeln, Funktionen, Konstanten, 
etc., so musst du die entsprechenden Header-Files includen. Und zwar 
jedes einzelne! Auch wenn es in einfachen Fällen funktionieren mag, so 
ist es trotzdem falsch, ein Header-File, das man auch braucht, nur 
indirekt über ein anderes zu includen (bei Standard-Libraries wie 
<stdlib.h>, <string.h>, <avr/io.h>, <inttypes.h>, etc. kann man 
allenfalls diskutieren). Andernfalls ist es nicht mehr möglich, die 
Files unabhängig von einander und in beliebiger Reihenfolge zu 
compilieren (z.B. ohne dein Makefile), es leidet die 
Wiederverwendbarkeit des Codes und last but not least verlierst Du 
vollkommen den Überblick darüber, was von wo kommt bzw. was wo verwendet 
wird (wo kommt z.B. val_set her, das in menu.c verwendet aber nicht 
deklariert wird).

Um den Inhalt jedes Header-Files gehört ein #ifndef-#endif Block, damit 
das Header File in mehreren C-Files included werden kann (in valout.h 
und key.h fehlt ein solcher Block).

In die Header-Files gehören die Dinge (Typen, Variabeln, Funktionen, 
Konstanten, etc.), die auch ausserhalb des dazu gehörenden C-Files 
verwendet werden dürfen - und zwar nur die Deklaration, nicht aber die 
Definition. Globale Variabeln werden also im Header File als 'extern' 
deklariert und im entsprechenden C-File definiert. Bei Funktionen wird 
im Header nur der Prototyp deklariert, die Definition (Implementation) 
gehört wieder in das C-File. Werden Definitionen im Header gemacht, so 
werden die entsprechenden Datenstrukturen jedesmal angelegt wenn die 
Header-Datei included wird, was spätestens beim Linken zu Fehlern oder 
Warnungen führt. Falsch gemacht ist dies z.B. bei val_unit in main.h und 
bei draw_menue() in main.h.

Du darfst nur Header-Dateien includen. Das Includen von C-Dateien ist 
ein absolutes No-Go (z.B. #include "menu.c" in main.c ).

Wenn du das mit den Header-Dateien falsch machst, so führt das u.U. 
nicht unmittelbar zu Problemen, auf die Dauer aber schon. Der Compiler 
hilft dir zwar in der Regel mit Fehlermeldungen oder Warnungen. Zweitere 
werden aber oft übersehen oder liechtfertig ignoriert.

Übrigens: Compiliert der Code bei dir ohne eine einzige Warnung?

Ich bin noch über einen weteren Flash Fehler gestolpert: In valout.c 
gibt es ein Makro, das 'table_read(x,y)' als 'pgm_read_dword(x+y)' 
definiert. Es holt also Daten aus dem Flash. Aufgerufen wird das Makro 
aber mit einem Parameter (TEST), der auf Daten im RAM zeigt.

Daneben ist mir noch aufgefallen, dass du im Gegensatz zu Peter 
Dannegger den Counter des Timers (TCNT0) nur im Interrupt-Handler setzt 
und nicht auch beim Initialisieren. Das dürfte aber wohl keinen 
spürbaren Effekt haben.


Viele Grüsse
Claude

[Moritz A. (moritz_a)]

Claude M. schrieb:
> Um den Inhalt jedes Header-Files gehört ein #ifndef-#endif Block, damit
> das Header File in mehreren C-Files included werden kann (in valout.h
> und key.h fehlt ein solcher Block).

Wenn man Code von anderen unüberprüft verwendet ;)

> In die Header-Files gehören die Dinge (Typen, Variabeln, Funktionen,
> Konstanten, etc.), die auch ausserhalb des dazu gehörenden C-Files
> verwendet werden dürfen - und zwar nur die Deklaration, nicht aber die
> Definition. Globale Variabeln werden also im Header File als 'extern'
> deklariert und im entsprechenden C-File definiert.

Behoben

> Bei Funktionen wird
> im Header nur der Prototyp deklariert, die Definition (Implementation)
> gehört wieder in das C-File. Werden Definitionen im Header gemacht, so
> werden die entsprechenden Datenstrukturen jedesmal angelegt wenn die
> Header-Datei included wird, was spätestens beim Linken zu Fehlern oder
> Warnungen führt. Falsch gemacht ist dies z.B. bei val_unit in main.h und

Ich meine mal gelesen zu haben, dass es bei const durchaus richtig ist, 
dies bereits im Header-File zu definieren, da hier je nach Verwendung 
zur Compile-Zeit der Wert schon ersetzt wird, sofern der Index statisch 
ist. Ich habe es jetzt trotzdem auch mal als extern deklariert.

> Du darfst nur Header-Dateien includen. Das Includen von C-Dateien ist
> ein absolutes No-Go (z.B. #include "menu.c" in main.c ).

Da hast du recht, das war eine Nachlässigkeit, ich habe den Code erstmal 
nur für die Übersichtlichkeit ausgelagert. So richtig wiederverwendbar 
ist menu.c sowieso nicht, das es teilweise direkt auf globale Variablen 
aus main.h zugreift.

> Übrigens: Compiliert der Code bei dir ohne eine einzige Warnung?

Fast, die einzige war:

Compiling C: valout.c
avr-gcc -c -mmcu=atmega328p -I. -gdwarf-2 -DF_CPU=16000000UL -Os -funsigned-char -funsigned-bitfields -fpack-struct -fshort-enums -ffunction-sections -fdata-sections -Wall -Wextra -Wpedantic -Wstrict-prototypes -Wa,-adhlns=./valout.lst  -std=gnu99 -Wundef -MMD -MP -MF .dep/valout.o.d valout.c -o valout.o 
valout.c: In function ‘valout’:
valout.c:58:11: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
       val >= table_read(TEST,i-1);
           ^

> Ich bin noch über einen weteren Flash Fehler gestolpert: In valout.c
> gibt es ein Makro, das 'table_read(x,y)' als 'pgm_read_dword(x+y)'
> definiert. Es holt also Daten aus dem Flash. Aufgerufen wird das Makro
> aber mit einem Parameter (TEST), der auf Daten im RAM zeigt.

Ich sehe da ein PROGMEM:

#define table_type  const uint16_t PROGMEM
table_type TEST[] = {

> Daneben ist mir noch aufgefallen, dass du im Gegensatz zu Peter
> Dannegger den Counter des Timers (TCNT0) nur im Interrupt-Handler setzt
> und nicht auch beim Initialisieren. Das dürfte aber wohl keinen
> spürbaren Effekt haben.

Damit findet der erste Tastencheckaufruf zu spät statt, ja. Aber das 
gehört esowieso noch auf CTC umgestellt, was ich jetzt mal erledigt 
habe.

Ich hoffe, keinen deiner Punkte übersehen zu haben. Der Code kompiliert 
so bei mir, die aktuelle Version ist angehängt.

[Juergen G. (jup)]

Verwende mal den Parameter -Wall beim kompilieren, da ist der Kompiler 
gespraechiger und Du wirst sehen was Claude M. (stoner) meint.

[Moritz A. (moritz_a)]

avr-gcc … -Wall -Wextra -Wpedantic -Wstrict-prototypes …
          ^^^^^

Der Parameter steht doch mit im Aufruf.

[Juergen G. (jup)]

Ja, Schande ueber mich, hab in Deinem letzten Post den Scrollbalken 
ignoriert.

[Moritz A. (moritz_a)]

Kein Problem, das Makefile ist ja durchaus recht vollgepackt mit 
gcc-Parametern. Ich habe eigentlich versucht alles zu aktivieren was 
irgendwie sinnvoll ist bzw mich zwingt sauberer zu arbeiten.

[Juergen G. (jup)]

Mal ne Frage,

versuchst Du Dich gerade mit der Programmierung eines Menu so "from 
Scratch" vertraut zu machen, oder hast Du Dir mal bestehende Menu 
Systeme angesehen und versucht zu verstehen wie die Programmierer das 
machen?

[Claude M. (stoner)]

Moritz A. schrieb:
> Ich meine mal gelesen zu haben, dass es bei const durchaus richtig ist,
> dies bereits im Header-File zu definieren, da hier je nach Verwendung
> zur Compile-Zeit der Wert schon ersetzt wird, sofern der Index statisch
> ist. Ich habe es jetzt trotzdem auch mal als extern deklariert.

Ja, zumindest bei Konstanten mit primitiven Datentypen (char, int, ...) 
ist das richtig.

> Ich sehe da ein PROGMEM:

> #define table_type  const uint16_t PROGMEM
> table_type TEST[] = {

Ja, habe ich doch glatt das #define übersehen.

[Juergen G. (jup)]

Ich weiss nicht ob es didaktisch Sinn macht hier jetzt fertigen Code zu 
posten. Vielleicht hilft es Dir aber mal zu sehen wie meine Programme 
aufgebaut sind die ein Menu System verwenden.

Der Code ist bei weitem nicht perfekt, ich habe ihn vor einiger Zeit so 
auf die Schnelle zusammengeschrieben als ich meinen Belichter auf Ober- 
und Unterlicht umgeruestet habe.

Die Basis ist aber die selbe die Du verwendest.

Fleury LCD lib
Dannegger debounce

Das Menu System ist auch auf der Basis von einem Post hier im Forum.
Ich erinnere mich nicht mehr wer es war, aber ein Forenmitglied hat 
damals die State Machine aus dem AVR-Butterfly "ausgeloetet"

Schau mal drueber, vielleicht hilft's ja Dein Programm etwas besser zu 
strukturieren.

[Moritz A. (moritz_a)]

Juergen G. schrieb:
> Das Menu System ist auch auf der Basis von einem Post hier im Forum.
> Ich erinnere mich nicht mehr wer es war, aber ein Forenmitglied hat
> damals die State Machine aus dem AVR-Butterfly "ausgeloetet"

Ich habe mir den originalen Butterfly-Code mal zu Gemüte geführt, der 
ist deinem Code recht ähnlich, ja.

Und im Gegensatz zum Code aus Tinykon bietet er die notwendige 
Flexibilität, so dass ich mir diese State-Machine mit nachgeschaltetem 
Menü sparen kann. Ich bin daher gerade beim umstricken des Codes.

EDIT: Es war nicht Tinykon, sondern irgendwo anders hier aus dem Forum 
gefallen.

[Claude M. (stoner)]

In lcd_uint16_unit() rufst du valout( v, 6, 3, s ) auf. s hat dabei eine 
Grösse von 6. In valout() schreibst du dann 6 Zeichen in den Puffer und 
hängst ein \0 an. Dabei schreibst du in fremden Speicher.

Viele Grüsse
Claude

[Claude M. (stoner)]

Claude M. schrieb:
> In valout() schreibst du dann 6 Zeichen in den Puffer und
> hängst ein \0 an. Dabei schreibst du in fremden Speicher.

Das war so nicht ganz korrekt. Der innerhalb von valout() erzeugte 
String der Länge n+1 (inkl. '\0') dürfte in den Puffer 'buff' von 
valout() passen, aber dann füllst du den übergebenen Puffer 's' zuerst 
mit 6-n führenden Spaces auf und kopierst dann die n+1 Zeichen von 
'buff' rein. Somit hast du 6+1 Zeichen nach 's' geschrieben und in einen 
Speicherbereich geschieben in den du nicht darfst ('s' hat ja nur eine 
Grösse von 6 Zeichen).

Durch das Vergrössern des Puffers 's' in lcd_uint16_unit() auf 8 Bytes 
habe ich das Problem bei mir gestern Abend beheben können. Nach 
genauerem Studium von valout() bin ich der Meinung, dass das Problem so 
zwar behoben werden kann, dass valout() aber viel zu kompliziert und 
fehleranfällig ist und grundlegend überarbeitet werden sollte, sonst 
wirst du früher oder später wieder ähnliche Probleme haben...