Menu for display - Githubissues

Wurstnase commented 8 years ago

Hi,

I'm starting to make a new nice menu for the new nice display. My programming skills are growing and growing but I have some issues with the current situation.

I want to have a kind of state machine for the menu. This menu should display e.g. %flow and %speed. For this I need something like PSTR("Flow: %lu"), flow.

Current code looks like this:

typedef void (FUNC)(void);
typedef FUNC *func;

typedef struct
{
  const char text[30];
  func menu_function;
  uint8_t next;
}
MENU_t;

typedef enum
{
  MAIN_MENU = 0x00,
    START_MENU,
      START,
      PAUSE,
      RESUME,
      STOP,
    NEW_MENU,
      SUB_MENU,
  NO_MENU
}
state_menu_t;

typedef struct
{
  MENU_t line_1;
  MENU_t line_2;
  MENU_t line_3;
  MENU_t line_4;
}
DISPLAY_MENU_t;

const DISPLAY_MENU_t PROGMEM state_menu_table[] = {
  {{"Hauptmenu", 0, MAIN_MENU}, {"Start", 0, START_MENU}, {"Flow: %lu", 0, START_MENU}, {"Speed", 0, NO_MENU}}
};

void display_menu(state_menu_t state) {
  display_set_cursor(0, 0);
  sendf_P(display_writechar, (state_menu_table[state].line_1.text));
  display_set_cursor(1, 10);
  sendf_P(display_writechar, (state_menu_table[state].line_2.text));
  display_set_cursor(2, 10);
  sendf_P(display_writechar, (state_menu_table[state].line_3.text));
  display_set_cursor(3, 10);
  sendf_P(display_writechar, (state_menu_table[state].line_4.text));
}

void display_menu_clock() {
  display_menu(MAIN_MENU);
}

How I can pass now the flow?

phord commented 8 years ago

Add an "arg" pointer parameter to each struct, maybe?

typedef struct
{
  const char text[30];
  uint32_t * arg;
  func menu_function;
  uint8_t next;
}
MENU_t;

// ...

uint32_t flow=100, speed=100;
const DISPLAY_MENU_t PROGMEM state_menu_table[] = {
  {{"Hauptmenu", NULL, 0, MAIN_MENU}, {"Start", Hauptmenu", NULL, 0, START_MENU}, 
    {"Flow: %lu", &flow, 0, START_MENU}, {"Speed: %lu", &speed, 0, NO_MENU}}
};

void display_menu(state_menu_t state) {
  display_set_cursor(0, 0);
  uint32_t arg = state_menu_table[state].line_1.arg : *state_menu_table[state].line_1.arg : 0 ;
  sendf_P(display_writechar, state_menu_table[state].line_1.text, arg);

The extra work to decide whether it is safe to dereference 'arg' is annoying here, but it would be less annoying if you do this in a loop. Alternatively you could point 'arg' to some unused variable instead of using NULL to exclude it.

I'm not sure if uint32_t is the correct size or not. It seems wasteful.

Other advice: Replace line_1, line_2, line_3, line_4 with line[4] instead. Then use a loop to iterate over them.

int i;
for (i=0; i<4; i++) {
  display_set_cursor(i, 10*(!i));
  uint32_t arg = state_menu_table[state].line[i].arg : *state_menu_table[state].line[i].arg : 0 ;
  sendf_P(display_writechar, state_menu_table[state].line[i].text, arg);
}

Wurstnase commented 8 years ago

Thanks, pointers is one thing I need to internalize. Read some of it, and sometimes I use them correctly. But most time it's more luck ;)

I guess you mean uint32_t arg = x ? *x : 0?

phord commented 8 years ago

Right.

On Fri, Aug 26, 2016, 2:35 PM Wurstnase notifications@github.com wrote:

Thanks, pointers is one thing I need to internalize. Read some of it, and sometimes I use them correctly. But most time it's more luck ;)

I guess you mean uint32_t arg = x ? *x : 0?

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/Traumflug/Teacup_Firmware/issues/238#issuecomment-242815975, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHkBFvhVQDJ84aPSKFUduQVhJW8dBydks5qjzG-gaJpZM4JuFIa .

Wurstnase commented 8 years ago

I really need to clean up my code for a push. This is my last attempt:

typedef void (*func)(void);

typedef struct
{
  func display_line;
  func menu_function;
  uint8_t next;
}
MENU_t;

typedef enum
{
  MAIN_MENU = 0x00,
    START_MENU,
      START,
      PAUSE,
      RESUME,
      STOP,
    COUNTER_MENU,
      RESET_COUNTER,
  NO_MENU
}
state_menu_t;

typedef struct
{
  MENU_t line[4];
}
DISPLAY_MENU_t;

void display_start(void) {
  sendf_P(display_writechar, PSTR("Start"));
}

void display_flow(void) {
  uint16_t var = (next_target.target.f_multiplier * 25 + 4) / 64;
  sendf_P(display_writechar, PSTR("Speed: %u%%  ") ,var);
}

void display_none(void) {

}

const DISPLAY_MENU_t PROGMEM state_menu_table[] = {
  {
    {
      {display_none, 0, START_MENU}, 
      {display_flow, 0, START_MENU}, 
      {display_start, 0, START_MENU},
      {display_none, 0, NO_MENU}
    }
  }
};

void display_menu(state_menu_t state) {
  uint8_t i;
  for (i = 0; i < 4; i++) {
    display_set_cursor(i, 0);
    func line;
    line = pgm_read_ptr(&state_menu_table[state].line[i].display_line);
    line();
  }
}

void display_menu_clock() {
  display_menu(MAIN_MENU);
}

Annoying thing is the:

func line;
line = pgm...;
line();

And there is the point to the pointer... When I try (pgm...)(); then I get:

 error: called object is not a function or function pointer
     pgm_read_ptr(&state_menu_table[state].line[i].display_line)();

What is the correct syntax for this?

Wurstnase commented 8 years ago

Correct syntax is: ((func)(pgm_read_ptr(&state_menu_table[state].line[i].display_line)))();

triffid commented 8 years ago

May need to cast it, line = (void (*)(void)) pgm... or so perhaps On 29 Aug 2016 20:21, "Wurstnase" notifications@github.com wrote:

I really need to clean up my code for a push. This is my last attempt:

typedef void (*func)(void);

typedef struct { func display_line; func menu_function; uint8_t next; } MENU_t;

typedef enum { MAIN_MENU = 0x00, START_MENU, START, PAUSE, RESUME, STOP, COUNTER_MENU, RESET_COUNTER, NO_MENU } state_menu_t;

typedef struct { MENU_t line[4]; } DISPLAY_MENU_t;

void display_start(void) { sendf_P(display_writechar, PSTR("Start")); }

void display_flow(void) { uint16_t var = (startpoint.f_multiplier * 25) / 64; sersendf_P(PSTR("%u\n"), var); sendf_P(display_writechar, PSTR("Speed: %u%% ") ,var); }

void display_none(void) {

}

const DISPLAY_MENU_t PROGMEM state_menu_table[] = { // { "Mainmenu", { { {display_none, 0, START_MENU}, {display_flow, 0, START_MENU}, {display_start, 0, START_MENU}, {display_none, 0, NO_MENU} } } };

void display_menu(state_menu_t state) { uint8_t i; for (i = 0; i < 3; i++) { display_set_cursor(i, 0); func line; line = pgm_read_ptr(&state_menu_table[state].line[i].display_line); line(); } }

void display_menu_clock() { display_menu(MAIN_MENU); }

Annoying thing is the:

func line; line = pgm...; line();

And there is the point to the pointer... When I try (pgm...)(); then I get:

error: called object is not a function or function pointer pgm_read_ptr(&state_menu_table[state].line[i].display_line)();

What is the correct syntax for this?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Traumflug/Teacup_Firmware/issues/238#issuecomment-243108658, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKBGvCY07tukjI88cZDIgkdQG3SqNv8ks5qks7bgaJpZM4JuFIa .

Wurstnase commented 8 years ago

May need to cast it, line = (void (*)(void)) pgm... or so perhaps

@triffid Yes, you are right.

Traumflug commented 8 years ago

Seeing the trouble here, seeing how many people struggle to get Configtool running, capitulate on compiling with a makefile, seeing that mess of suitable displays available, I recently developed a strong tendency to move all this into one of these small scale PCs like RaspPi or BeagleBone. That'd be an SD card image with an Android OS, completed with all the stuff one needs for printing: Teacup sources, Git, Configtool, Python, Pronterface, Slicer, all that stuff. There's a package manager, so getting such stuff in is as easy as 'apt-get install ...'.

Given that a RaspPi Zero is just $5 and is apparently an offering to stay, it's IMHO unlikely printer controllers can compete with their neccessarily limited display/handling capabilities long term. This embedded PC can be accessible via VNC (remote screen) and/or a RaspPi display (means: Teacup doesn't have to mess with this display) and/or the PC's print queue(!) and mounted inside the printer.

Now, I don't want to keep you away from writing menu code, of course :-)

Traumflug commented 8 years ago

mounted inside the printer.

... next to the actual printer controller, of course, which is dedicated to just driving the printer, nothing else.

Wurstnase commented 8 years ago

The display should do not that much.

Show maybe the position (not that big important)
Show/manipulate the temperatures
Show flow%/speed% and manipulation (My printer is in my cellar and I can't take my PC down to controll flow and speed)
Stop a print

All-in-all there are some interesting things which will improve it. The downside is setting the display up. Absolutely true! I've reading a lot of trouble (guides) on reprap.org.

Wurstnase commented 8 years ago

I've uploaded some code. This code is based not on experimental. The base is fix_ssd1306. https://github.com/Traumflug/Teacup_Firmware/tree/keys_and_display_menu

However anyone will decide. This code is currently just a piece of code I need for my work and want to share with all.

Wurstnase commented 8 years ago

While doing some work with the menu, I found some interesting stuff about PROGMEM. We could use __flash instead and have more control with the compiler and no more any pgmread* and so on.

Don't? Do? Should be some work, but in the end we get nicer and more readable code. And one external lib less.

Traumflug commented 8 years ago

A pointer to what you have found would be nice.

Wurstnase commented 8 years ago

Only in German: https://www.mikrocontroller.net/articles/AVR-GCC-Tutorial#Flash_mit_flash_und_Embedded-C

Wurstnase commented 8 years ago

Some code is near the same:

#define XSTR(X) ({static const __flash char __s[] = X; &__s[0];})
display_writestr_F(XSTR("Active"));
vs.
# define PSTR(s) (__extension__({static const char __c[] PROGMEM = (s); &__c[0];}))
display_writestr_P(PSTR("Active"));

others:

void sendf_F(void (*writechar)(uint8_t), const __flash char *format_F, ...) {
  va_list args;
  va_start(args, format_F);

  uint16_t i = 0;
  uint8_t c = 1, j = 0;
  while ((c = format_F[i++])) {

vs.

void sendf_P(void (*writechar)(uint8_t), PGM_P format_P, ...) {
  va_list args;
  va_start(args, format_P);

  uint16_t i = 0;
  uint8_t c = 1, j = 0;
  while ((c = pgm_read_byte(&format_P[i++]))) {

Traumflug commented 8 years ago

Ah, I see, gcc has started to support distinct address spaces. Very nice. Some English documentation is here: https://gcc.gnu.org/onlinedocs/gcc/Named-Address-Spaces.html

Limitations include:

No support for very small AVRs. To hack these, one still has to remember how this PROGMEM thing works.
There are several spaces. Support for 64 kB per space, only. Accordingly one has to know where the compiler puts the data. Alternative: one uses the 24-bit space __memx, which likely grows the binary.

Other than that it looks good. Finally no longer reading the wrong memory type due to a forgotten pgm_read_byte() :-)

If you feel like trying it, well, go ahead! I'd be interested in finding out what this brings in terms of code size and/or execution speed, too.

triffid commented 8 years ago

Ooh nice find, what's the minimum GCC version needed to use this? On 9 Sep 2016 05:46, "Traumflug" notifications@github.com wrote:

Ah, I see, gcc has started to support distinct address spaces. Very nice. Some English documentation is here: https://gcc.gnu.org/ onlinedocs/gcc/Named-Address-Spaces.html

Limitations include:

No support for very small AVRs. To hack these, one still has to remember how this PROGMEM thing works.

There are several spaces. Support for 64 kB per space, only. Accordingly one has to know where the compiler puts the data. Alternative: one uses the 24-bit space __memx, which likely grows the binary.

Other than that it looks good. Finally no longer reading the wrong memory type due to a forgotten pgm_read_byte() :-)

If you feel like trying it, well, go ahead! I'd be interested in finding out what this brings in terms of code size and/or execution speed, too.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Traumflug/Teacup_Firmware/issues/238#issuecomment-245751842, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKBGrownm8SrA6fMY_BZOALIJBeoXWEks5qoII5gaJpZM4JuFIa .

Wurstnase commented 8 years ago

@triffid Afaik 4.7

Wurstnase commented 8 years ago

I made a quick test with memx. I removed serial_writestr_P and changed serial_writestr to `void serial_writestr(const memx char * data_F)`

Less code, but 48bytes more.

Wurstnase commented 8 years ago

If you feel like trying it, well, go ahead! I'd be interested in finding out what this brings in terms of code size and/or execution speed, too.

Was less work I thought. Most time I need to fiddle out that avr/pgmspace.h includes avr/io.h which then was missing.

All-in-all 20 bytes bigger binary. Speed needs now a check. I would guess it should be slightly faster, because some code can be optimized by the compiler now.

Wurstnase commented 7 years ago

I test the progmem_to_flash again with latest experimental and GCC 6.1.1

  #include "config/board.gen7-v1.4.h"
  #include "config/printer.mendel.h"

experimental:

ATmega sizes               '168   '328(P)   '644(P)     '1280
Program:  22816 bytes      160%       75%       36%       18%
   Data:   1376 bytes      135%       68%       34%       17%
 EEPROM:     32 bytes        4%        2%        2%        1%

progmem_to_flash:

ATmega sizes               '168   '328(P)   '644(P)     '1280
Program:  22762 bytes      159%       75%       36%       18%
   Data:   1376 bytes      135%       68%       34%       17%
 EEPROM:     32 bytes        4%        2%        2%        1%

54 bytes saved :)

Wurstnase commented 7 years ago

I just compared the current experimental with make regressiontest and this is the result:

config	experimental	progmem_to_flash	diff
config.regtest-gen7-avr.h	22612	22496	116
config.regtest-ramps.h	23382	23270	112
config.regtest-nanoheart.h	20064	19970	94
config.regtest-teensy2.h	19380	19292	88
config.regtest-gen3.h	20766	20656	110
config.h.Profiling	18262	18174	88
config.regtest-display.h	23516	23418	98
config.regtest-acceleration-reprap.h	20712	20710	2
config.regtest-acceleration-temporal.h	20054	20044	10
config.regtest-no-endstops.h	21530	21494	36
config.regtest-no-lookahead.h	20168	20154	14

Wurstnase commented 7 years ago

This __flash stuff helps only with active -flto.

config	Program	Data
config.regtest-gen7-avr.h	-118	0
config.regtest-ramps.h	-114	0
config.regtest-nanoheart.h	-100	0
config.regtest-teensy2.h	-90	0
config.regtest-gen3.h	-112	0
config.h.Profiling	-90	0
config.regtest-display.h	-104	0
config.regtest-acceleration-reprap.h	-2	0
config.regtest-acceleration-temporal.h	-10	0
config.regtest-gen7-arm.h	16	0
config.regtest-no-endstops.h	-42	0
config.regtest-no-lookahead.h	-14	0
config.regtest-cnc-shield-stm32.h	0	0

Traumflug / Teacup_Firmware

Menu for display #238