C->LCD-> Im Menü scrollen

[Anja F. (Gast)]

Einen wunderschönen guten Abend,

ich möchte gerne durch ein Menü scrollen ( dieses besteht aus einer 
Struktur ).

Es funktioniert zwar, ich finde es jedoch nicht elegant gelöst. Hat 
jemand eine bessere Lösung wie man solch ein Menü "durch rollen" kann?!

/*!<-- TESTING BETA <--*/
uint8_t OpenMenueBETA( sLcdMenue_t *psMenue )
{
    uint8_t     uiActualMenuePos = 0;   
    uint8_t     uiSubFncRet = 0;
    
    do 
    {
        switch ( EncoderGetRotation() )
        {
            case ENC_ROTATION_LEFT:
            {
                if ( uiActualMenuePos >  eLCD_MENUE_START )
                {
                    uiActualMenuePos--;
                }
            }break;
            
            case ENC_ROTATION_RIGHT:
            {
                if ( uiActualMenuePos < (__eLCD_MAX_ENTRYS__-1) )
                {
                    uiActualMenuePos++;
                }
            }break; 
        }
        
        /*!<
        *
        *   psMenue[0..6] // Hier befinden sich die Namen für die Menues
        *   
        *
        *   Zeile[0]: "Menue Open"
        *   Zeile[1]: "->Func1"
        *
        *   ..scrollen..
        *
        *   Zeile[0]: "Func1"
        *   Zeile[1]: "->Func2"
        *
        *>*/        
        LcdFillString( psMenue[uiActualMenuePos].Message.sMain.pMainMessage , 0 , 0 );
        
        if ( uiActualMenuePos+1 < (__eLCD_MAX_ENTRYS__) )
        {
            lcd_gotoxy( 0 , 1 );
            lcd_puts( "->" );
            LcdFillString( psMenue[uiActualMenuePos+1].Message.sMain.pMainMessage , 2 , 1 );
        }
        else
        {
            LcdFillString( "" , 0 , 1 );
            uiActualMenuePos = 5 ;
        }
        
 
        if ( sLCDTime.uiMilli > 500 )
        {
            char *ui = { "\0\0" };
            ui[0] = uiActualMenuePos + '0';
            LcdFillString( ui , 14 , 0 );
            _delay_ms(100);
            sLCDTime.uiMilli = 0;
        }
        
        if ( unFlag.sFlag.bButtonEnter )
        {
            unFlag.sFlag.bButtonEnter = 0;
            if ( psMenue[uiActualMenuePos+1].pfnc != NULL )
            {
                psMenue[uiActualMenuePos+1].pfnc();
            }
        }
        
    }while ( uiSubFncRet != MENUE_EXIT );
    
    
    return 0;
}

[leo (Gast)]

Anja F. schrieb:
1.) Es ist sicher praktisch ein paar Eckdaten zur Hardware anzugeben.

2.) der Code ist (fuer mich) schwer zu lesen.

Warum ...

> uiActualMenuePos

... und nicht einfach "pos" (im Menuecode - was sonst), usw.?

Die ungarische Notation, gross/klein, lange reduntante Namen, usw. 
machen keinen Sinn und helfen nicht, den Code schnell zu erfassen - im 
Gegenteil.

leo

[Sprach Geleerter (Gast)]

Anja F. schrieb:
> Einen wunderschönen guten Abend,

Beim Versuch dir zu helfen bin ich leider schon an deinen
Englischkentnissen (die offensichtlich nicht sonderlich gut
sind) hängengeblieben.

Dein Verständnis vom englischen "actual" ist offensichtlich
"aktuell".

Die allgemeine englischsprachige Auffassung von "actual"
ist dagegen "tatsächlich".

Wer "aktuell" im Englischen meinen möchte der schreibt "current".

[Anja F. (Gast)]

leo schrieb:
> Anja F. schrieb:
> 1.) Es ist sicher praktisch ein paar Eckdaten zur Hardware anzugeben.
>
> 2.) der Code ist (fuer mich) schwer zu lesen.
>
> Warum ...
>
>> uiActualMenuePos
>
> ... und nicht einfach "pos" (im Menuecode - was sonst), usw.?
> leo

Mit den Namen habe ich es nicht so.. Manchmal schreibe ich da auch etwas 
sehr umgeschriebenes hin.. Das muss sich bessern, stimmt.

[mir (Gast)]

Sprach Geleerter schrieb:
> Beim Versuch dir zu helfen bin ich leider schon an deinen
> Englischkentnissen (die offensichtlich nicht sonderlich gut
> sind) hängengeblieben.

Konzentriere dich doch erst einmal auf die deutsche Interpunktion, bevor 
du Kleinlichkeiten äußerst.
"derzeitig" finde ich auch andere Variationen
https://www.dict.cc/?s=actual

[Frank M. (ukw) (Moderator)]

Anja F. schrieb:
>   char *ui = { "\0\0" };

Die Initialisierung mit "\0\0" halte ich für "unschön". Ich war erstmal 
erstaunt, dass hiermit tatsächlich drei Bytes reserviert und 
initialisiert werden, weil das erste '\0' ja den Initialisierungsstring 
schon terminiert.

Aber das scheint tatsächlich zu funktionieren, wie ich nach einigen 
Tests (bzgl. Stack) herausgefunden habe.

Allerdings geht dann das nicht in Ordnung:

>   ui[0] = uiActualMenuePos + '0';

Hier schreibst Du in eine Zeichenkonstante.

Unter Linux erzeugt dies reproduzierbar einen Segmentation fault:

void myfunc (void)
{
    char * ui = "\0\0";

    ui[0] = '0'; // hier knallts

    printf ("%02x %02x\n", ui[0], ui[1]);
}

Um sicherzustellen, dass Dein String ui tatsächlich terminiert und 
beschreibbar ist, schreibe besser:

    char ui[2];
    ui[0] = uiActualMenuePos + '0';
    ui[1] = '\0';

Beachte, dass dies nur für einstellige Werte funktioniert. Bei 
Menüpositionen größer 9 bekommst Du hier ein Problem.

[Frank M. (ukw) (Moderator)]

Anja F. schrieb:
>         if ( uiActualMenuePos+1 < (_eLCD_MAX_ENTRYS_) )
>         {
>             ...
>         }
>         else
>         {
>             LcdFillString( "" , 0 , 1 );
>             uiActualMenuePos = 5 ;
>         }

Im switch oberhalb dieses Blockes stellst Du sicher, dass 
uiActualMenuePos niemals über die Anzahl der Menüeinträge hinausgehen 
kann - also immer kleiner ist.

Was soll dann dieses else? Angst vor eigenem Programmierfehler? 
Eigentlich kann das weg. Wenn Du es doch behalten möchtest, dann 
schreibe
statt:

           uiActualMenuePos = 5 ;

besser

           uiActualMenuePos = __eLCD_MAX_ENTRYS__ - 1;

Apropos: Diese Konstante mit den beiden führenden Unterstrichen, kommt 
diese aus der verwendeten LCD-Bibliothek oder wurde diese von Dir 
definiert? Wenn letzteres, lass den Quatsch mit den führenden 
Unterstrichen, denn solche Konstanten sind für den Compiler und dessen 
Laufzeitumgebung reserviert. Nenne diese Konstante besser 
LCD_MAX_ENTRYS.

Noch eine Frage:
>         if ( uiActualMenuePos+1 < (_eLCD_MAX_ENTRYS_) )

Warum klammerst Du _eLCD_MAX_ENTRYS_ ?

Muss das so sein, weil sich da eventuell ein komplexer Ausdruck dahinter 
verbirgt? Wenn ja: Mach die Klammerung im #define, nicht hier.

Letzte Frage:
> if ( psMenue[uiActualMenuePos+1].pfnc != NULL )

Wieso +1 ? Lässt Du psMenu ab Index 1 loslaufen? Ist denn auch 
sichergestellt, dass die Größe des Arrays auch tatsächlich um 1 größer 
ist als tatsächlich benötigt?

In C laufen die Indizes über Arrays normalerweise von 0 bis N-1, wobei N 
die Größe des Arrays ist.

[PittyJ (Gast)]

leo schrieb:
> Anja F. schrieb:
> 1.) Es ist sicher praktisch ein paar Eckdaten zur Hardware anzugeben.
>
> 2.) der Code ist (fuer mich) schwer zu lesen.
>
> Warum ...
>
>> uiActualMenuePos
>
> ... und nicht einfach "pos" (im Menuecode - was sonst), usw.?
>
> Die ungarische Notation, gross/klein, lange reduntante Namen, usw.
> machen keinen Sinn und helfen nicht, den Code schnell zu erfassen - im
> Gegenteil.
>
> leo

Sehe ich genau umgekehrt.
Ich finde uiActualMenuePos besser als nur pos.
pos kann z.B. auch positiv bedeutet. Dann hätte man unbeknnten Code und 
muss noch um die Abkürzungen rätseln??
Ne Ne: uiActualMenuePos ist voll OK.

[DPA (Gast)]

Naja, das kann man schon so machen. Blockt halt alles. Kann jenachdem 
auch so gewünscht sein.

Oft find ich es auch schöner, keine blockierenden funktionen zu haben. 
Man kann statdessen eigentlich immer den Code auf asynchrone Modelle 
abändern, aber dann wird das memory management & hier noch das Stacking 
komplizierter, weil man dann nicht in ner Funktion mit Speicher auf dem 
Stack bleibt. Hier wäre es schön, wenn C co-routinen hätte.

Wie auch immer, um sowas asynchron zu machen, erstmal nur einen 
Eventloop erstellen:

int main(){
  // initialise everything
  input_init();
  ui_init();

  while(1){
    // Different short tasks
    input_check_for_input();
    foo_do_stuff();
    bar_do_stuff();
    ...
  }
}

Dann kann man mit event listenern / callbacks arbeiten. Beispiel:

typedef void (*input_handler_t)(void* private, enum input_what, int value);
void* input_handler_private;
input_handler_t input_handler;

void set_input_handler(input_handler_t new_input_handler, void* private){
  input_handler = new_input_handler;
  input_handler_private = input_handler;
}

void input_check_for_input(void){
  if(!input_handler)
    retrun;
  static int old_bla_encoder_state = false;
  if(old_bla_encoder_state != encoder_state)
    input_handler(input_handler_private, INPUT_ENCODER_ROTATED, encoder_state-old_bla_encoder_state);
}

Und das menu könnte man dann z.B. so machen:

struct ui {
  input_handler_t input_handler;
  void(*draw)(struct ui*);
};

enum { max_ui_layers = 16; }
int top;
struct ui* ui_stack[max_ui_layers];

static void ui_set(struct ui* new_ui){
  set_input_handler(new_ui->input_handler, new_ui);
  new_ui->draw(new_ui);
}

void ui_push(struct ui* new_ui){
  if(top >= max_ui_layers)
    fatal_ui_error();
  ui_stack[top++] = new_ui;
  ui_set(new_ui);
}

void ui_pop(void){
  if(top <= 1)
    fatal_ui_error();
  ui_set(ui_stack[--top-1]);
}

struct menu_ui {
  struct ui ui; // keep this the first entry
  const char* text;
  ...
};

struct menu_ui main_menu = {
  .ui.input_handler = menu_input_handler;
  .ui.draw = menu_draw;
};

void ui_init(){
  ui_push(&main_menu.ui);
}

void menu_draw(struct ui* pui){
  struct menu* menu = (struct menu*)pui; // Maybe use a container_of macro instead
  ...
  print_text(menu->text);
  ...
}

void menu_input_handler(void* private, enum what, int value){
  struct menu* menu = private; // maybe use a cast to ui and container_of instead

  if(what == INPUT_ENCODER_ROTATED){
    int option = (menu->option + value) % menu->option_count;
    if(option < 0)
      option += menu->option_count;
    menu->option = option;
    menu_draw(pui);
  }

  if(what == BUTTON_BLA && value){
    do_stuff(selection);
    ui_pop();
  }

  ...
}

Gibt sicher auch andere Methoden. Das wichtige ist, so ist der Input 
entkoppelt.  Nachteil ist, die push/pop aufrufe immer richtig zu machen 
ist praktisch unmachbar. Eventuell irgend ne referenz mitgeben, und was 
bauen was soviele pops machen kann, bis es bei dem ui element ankommt. 
Aber eben, das ist nur wie ich sowas machen würde, andere würden dass 
vermutlich anders machen.

[NichtWichtig (Gast)]

Die Position die durch den Encoder verändert wird sollte Gesamtanzahl 
Menuepunkte UND darstellbare Zeilen des Displays berücksichtigen.

Beispiel:
10 Menupunkte, 4 Zeilen darstellbar
uiActualMenuePos max = 10-4+1=7 = Zeile 7,8,9,10 werden angezeigt.



Blockiert  EncoderGetRotation() bis sich etwas tut oder liefert das Ding 
sofort Up/Down/Nix zurück?

Wenn Nix (default) zurück kommt brauchst Du nix am Display machen

if( Up || Down )
{
   // LCD updaten

}

und

   if ( psMenue[uiActualMenuePos+1].pfnc != NULL )
   {
       psMenue[uiActualMenuePos+1].pfnc();
   }

Abfrage auf != NULL ist überflüssig
Es reicht schlicht sowas.

   if ( psMenue[uiActualMenuePos+1].pfnc )
   {
       psMenue[uiActualMenuePos+1].pfnc();
   }

[W.S. (Gast)]

Anja F. schrieb:
> Es funktioniert zwar, ich finde es jedoch nicht elegant gelöst. Hat
> jemand eine bessere Lösung wie man solch ein Menü "durch rollen" kann?!

> /*!<-- TESTING BETA <--*/
> uint8_t OpenMenueBETA( sLcdMenue_t *psMenue )
> {....
>     do
>     {
>         switch ( EncoderGetRotation() )

Mich graust so etwas.

Also mal ganz vom Anfang her:
Du hast also nen µC, der irgendwas tun soll. Er hat ein Display, wo du 
diverse Dinge sehen kannst und unter diesen ist auch sowas wie ein 
"Menü". Aber dort (s.o.) gehst du rein prozedural vor wie der Elefant im 
Porzellanladen mit

do { switch (Encoder GET Rotation() ) .... } while...?

Was soll deiner Ansicht nach passieren mit all den anderen Dingen, die 
da angezeigt werden? Und was passiert, wenn deine Encoder-Get-Funktion 
mal bloß mit "hat keiner dran gedreht" zurück kommt? Du blockierst dich 
mit so einem Konstrukt an jeder Stelle selber.

Mach es anders. Zuerst trenne das Ermitteln einer Aktion an deinen 
Knöpfen ab vom Rest der Firmware und packe dessen Ergebnisse in Events, 
die du in einen Ringpuffer schreibst. Dann schreibe dein Menüsystem 
ereignisgestuert und nichtblockierend.

Also etwa so:
in main:
  für immer
  { if (GetEvent())
    Event = LiesEvent();
    if (Event.Typ == MenüEvent) Menue(Event);
    if (Event.Typ == Tralala) Tralala(Event);
    .. etc.
    ErledigeSonstigesZeugs();
  }

und
void Menue (TEvent Event)
{  if (Event.Was == Links) MacheLinks();
   if (Event.Was == Rechts) MacheRechts();
}
kurzum: nichtblockierend.

W.S.

[Thomas E. (thomase)]

Sprach Geleerter schrieb:
> Die allgemeine englischsprachige Auffassung von "actual"
> ist dagegen "tatsächlich".

Außer dir ist das allerdings jedem inkl. dem Compiler vollkommen egal.

[Nop (Gast)]

Thomas E. schrieb:

> Außer dir ist das allerdings jedem inkl. dem Compiler vollkommen egal.

Nö. Und spätestens wenn Du es mal mit Code zu tun bekommst, der nicht in 
Denglisch geschrieben ist, sondern im falschen Englisch einer anderen 
Sprache, wirst Du auch merken wieso.

[GL (Gast)]

Daumen hoch für ungarische Notation. Man kommt rein und weiß anhand des 
Namens direkt worum es sich handelt.

[S. R. (svenska)]

Frank M. schrieb:
> Nenne diese Konstante besser LCD_MAX_ENTRYS.

Ich empfehle ja "LCD_MAX_ENTRIES", dann wäre das auch korrekt.

Thomas E. schrieb:
> Außer dir ist das allerdings jedem inkl. dem Compiler vollkommen egal.

Nein. Ich sehe das Ergebnis täglich auf Arbeit, mit Code von Chinesen 
und Japanern. Sowohl Namen als auch Kommentare - sofern vorhanden, weil 
noch mehr englische Wörter - sind oft genug nicht klar oder 
missverständlich. Bei komplexen Systemen ist das absolut nicht egal.

Wenn es am Englisch scheitert, dann sollte man besser bei Deutsch 
bleiben. Mir ist ein fremdsprachlicher Vortrag (übersetzt/untertitelt) 
lieber als ein Vortrag, der durch die mangelhafte Sprachkenntnis des 
Vortragenden unbrauchbar wird.

GL schrieb:
> Daumen hoch für ungarische Notation. Man kommt rein und weiß
> anhand des Namens direkt worum es sich handelt.

Daumen runter für ungarische Notation. Man kommt rein und glaubt anhand 
des Namens direkt zu wissen, um was es sich handelt. Dann prüft man zwei 
Tage später nach und stellt fest, dass das in einer früheren Version des 
Codes tatsächlich mal so war.

[Eric B. (beric)]

S. R. schrieb:
> Dann prüft man zwei
> Tage später nach und stellt fest, dass das in einer früheren Version des
> Codes tatsächlich mal so war.

Das hätte dann aber beim Codereview angeprangert werden sollen und 
anschliessend korrigiert. ;-)
Die ungarische Notation hilft eigentlich nur für Variablen mit skalaren 
Werten; sobald Structs, Arrays, Pointer, usw ins Spiel kommen wird's 
lustig:

strName --> ist das ein STRing oder ein STRuct?
psAdresse --> Pointer auf String oder Struct?
acNummer --> Array von Char, also ein String? Aber warum heißt die 
Variable dann nicht sNummer oder strNummer?

[G. O. (aminox86)]

Anja F. schrieb:
> ich möchte gerne durch ein Menü scrollen ( dieses besteht aus einer
> Struktur ).

https://www.mikrocontroller.net/articles/Tinykon

[S. R. (svenska)]

Eric B. schrieb:
> Das hätte dann aber beim Codereview angeprangert werden
> sollen und anschliessend korrigiert. ;-)

Wenn ich mir unsere Patches so anschaue, dann sind die in möglichst 
minimalinvasiv, um Konflikte zu verhindern. Eine Veränderung, die 
möglicherweise sämtliche Dateien im Projekt gleichzeitig anfasst, wäre 
ein eigenes Refactoring-Projekt über ein Jahr. Sowas geht nicht spontan 
durchs Review. ;-)

[A. S. (Gast)]

Eric B. schrieb:
> Die ungarische Notation hilft eigentlich nur für Variablen mit skalaren
> Werten;

Nein, sie hilft gar nicht. Bzw. Maximal in so Fällen, wo schmutzige 
Tricks mit Überlauf nur bis 256 funktionieren. Aber z.b. hier: ob ich 
pos mit ++ oder INCpos() erhöhen sollte, kann ich nicht am Typ ablesen.

Der Typ ist zwar Milliardär geworden und Weltraumtourist, hat uns 
Windows beschert und vieles andere, aber das war ein Flop. Sowohl in 
seiner Version als auch in der heute üblichen absurden wo der Typ und 
nicht der Zweck kodiert wird.

[A. S. (Gast)]

Frank M. schrieb:
> Um sicherzustellen, dass Dein String ui tatsächlich terminiert und
> beschreibbar ist, schreibe besser:    char ui[2];
>     ui[0] = uiActualMenuePos + '0';
>     ui[1] = '\0';

Das ist nicht nötig, der Ursprungspost kann leicht korrigiert werden:

Anja F. schrieb:
> char *ui = { "\0\0" };


einfach ui[] statt *ui.

Aber statt der Operation am offenen Array einfach sofort auf größeren 
Buffer und sprintf gehen. Alles andere ist hacken.