[PATCH v2 1/3] menu: fix the logic checking whether ESC key is pressed

[PATCH v2 1/3] menu: fix the logic checking whether ESC key is pressed

[Weijie Gao]

It's observed that the bootmenu on a serial console sometimes
incorrectly quitted with superfluous characters filled to command
line input:

>  *** U-Boot Boot Menu ***
>
>      1. Startup system (Default)
>      2. Upgrade firmware
>      3. Upgrade ATF BL2
>      4. Upgrade ATF FIP
>      5. Load image
>      0. U-Boot console
>
>
>  Press UP/DOWN to move, ENTER to select, ESC to quit
>MT7988> [B

Analysis shows it was caused by the wrong logic of bootmenu_loop:

At first the bootmenu_loop received the first ESC char correctly.

However, during the second call to bootmenu_loop, there's no data
in the UART Rx FIFO. Due to the low baudrate, the second char of
the down array key sequence hasn't be fully received.

But bootmenu_loop just did a mdelay(10), and then treated it as a
single ESC key press event. It didn't even try tstc() again after
the 10ms timeout.

This patch fixes this issue by letting bootmenu_loop check tstc()
twice.

Tested-By: E Shattow <lucent@gmail.com>
Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
---
 common/menu.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/common/menu.c b/common/menu.c
index 8cc9bf06d9c..48ab7f0f398 100644
--- a/common/menu.c
+++ b/common/menu.c
@@ -525,14 +525,15 @@ enum bootmenu_key bootmenu_loop(struct bootmenu_data *menu,
 				struct cli_ch_state *cch)
 {
 	enum bootmenu_key key;
-	int c;
+	int c, errchar = 0;
 
 	c = cli_ch_process(cch, 0);
 	if (!c) {
 		while (!c && !tstc()) {
 			schedule();
 			mdelay(10);
-			c = cli_ch_process(cch, -ETIMEDOUT);
+			c = cli_ch_process(cch, errchar);
+			errchar = -ETIMEDOUT;
 		}
 		if (!c) {
 			c = getchar();
-- 
2.45.2

[PATCH v2 2/3] menu: add support to check if menu needs to be reprinted

[Weijie Gao]

This patch adds a new callback named need_reprint for menu.
The need_reprint will be called before printing the menu. If the
callback exists and returns FALSE, menu printing will be canceled.

This is very useful if the menu was not changed. It can save time
for serial-based menu to handle more input data.

Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
---
 boot/pxe_utils.c |  2 +-
 cmd/bootmenu.c   |  2 +-
 cmd/eficonfig.c  |  2 +-
 common/menu.c    | 11 +++++++++++
 include/menu.h   |  1 +
 5 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
index d6a4b2cb859..3ae17553c6d 100644
--- a/boot/pxe_utils.c
+++ b/boot/pxe_utils.c
@@ -1474,7 +1474,7 @@ static struct menu *pxe_menu_to_menu(struct pxe_menu *cfg)
 	 * Create a menu and add items for all the labels.
 	 */
 	m = menu_create(cfg->title, DIV_ROUND_UP(cfg->timeout, 10),
-			cfg->prompt, NULL, label_print, NULL, NULL);
+			cfg->prompt, NULL, label_print, NULL, NULL, NULL);
 	if (!m)
 		return NULL;
 
diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
index 977a04b7d76..c99605f3398 100644
--- a/cmd/bootmenu.c
+++ b/cmd/bootmenu.c
@@ -506,7 +506,7 @@ static enum bootmenu_ret bootmenu_show(int delay)
 
 	menu = menu_create(NULL, bootmenu->delay, 1, menu_display_statusline,
 			   bootmenu_print_entry, bootmenu_choice_entry,
-			   bootmenu);
+			   NULL, bootmenu);
 	if (!menu) {
 		bootmenu_destroy(bootmenu);
 		return BOOTMENU_RET_FAIL;
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
index bea09e4ecc7..498653b778d 100644
--- a/cmd/eficonfig.c
+++ b/cmd/eficonfig.c
@@ -443,7 +443,7 @@ efi_status_t eficonfig_process_common(struct efimenu *efi_menu,
 		efi_menu->menu_desc = menu_desc;
 
 	menu = menu_create(NULL, 0, 1, display_statusline, item_data_print,
-			   item_choice, efi_menu);
+			   item_choice, NULL, efi_menu);
 	if (!menu)
 		return EFI_INVALID_PARAMETER;
 
diff --git a/common/menu.c b/common/menu.c
index 48ab7f0f398..5a2126aa01a 100644
--- a/common/menu.c
+++ b/common/menu.c
@@ -43,6 +43,7 @@ struct menu {
 	void (*display_statusline)(struct menu *);
 	void (*item_data_print)(void *);
 	char *(*item_choice)(void *);
+	bool (*need_reprint)(void *);
 	void *item_choice_data;
 	struct list_head items;
 	int item_cnt;
@@ -117,6 +118,11 @@ static inline void *menu_item_destroy(struct menu *m,
  */
 static inline void menu_display(struct menu *m)
 {
+	if (m->need_reprint) {
+		if (!m->need_reprint(m->item_choice_data))
+			return;
+	}
+
 	if (m->title) {
 		puts(m->title);
 		putc('\n');
@@ -362,6 +368,9 @@ int menu_item_add(struct menu *m, char *item_key, void *item_data)
  * item. Returns a key string corresponding to the chosen item or NULL if
  * no item has been selected.
  *
+ * need_reprint - If not NULL, will be called before printing the menu.
+ * Returning FALSE means the menu does not need reprint.
+ *
  * item_choice_data - Will be passed as the argument to the item_choice function
  *
  * Returns a pointer to the menu if successful, or NULL if there is
@@ -371,6 +380,7 @@ struct menu *menu_create(char *title, int timeout, int prompt,
 				void (*display_statusline)(struct menu *),
 				void (*item_data_print)(void *),
 				char *(*item_choice)(void *),
+				bool (*need_reprint)(void *),
 				void *item_choice_data)
 {
 	struct menu *m;
@@ -386,6 +396,7 @@ struct menu *menu_create(char *title, int timeout, int prompt,
 	m->display_statusline = display_statusline;
 	m->item_data_print = item_data_print;
 	m->item_choice = item_choice;
+	m->need_reprint = need_reprint;
 	m->item_choice_data = item_choice_data;
 	m->item_cnt = 0;
 
diff --git a/include/menu.h b/include/menu.h
index 6571c39b143..79643af272b 100644
--- a/include/menu.h
+++ b/include/menu.h
@@ -13,6 +13,7 @@ struct menu *menu_create(char *title, int timeout, int prompt,
 				void (*display_statusline)(struct menu *),
 				void (*item_data_print)(void *),
 				char *(*item_choice)(void *),
+				bool (*need_reprint)(void *),
 				void *item_choice_data);
 int menu_default_set(struct menu *m, char *item_key);
 int menu_get_choice(struct menu *m, void **choice);
-- 
2.45.2

[PATCH v2 3/3] bootmenu: add reprint check

[Weijie Gao]

Record the last active menu item and check if it equals to the
current selected item before reprint.

Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
---
 cmd/bootmenu.c | 16 +++++++++++++++-
 include/menu.h |  1 +
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
index c99605f3398..ffa63a4628d 100644
--- a/cmd/bootmenu.c
+++ b/cmd/bootmenu.c
@@ -103,11 +103,13 @@ static char *bootmenu_choice_entry(void *data)
 
 		switch (key) {
 		case BKEY_UP:
+			menu->last_active = menu->active;
 			if (menu->active > 0)
 				--menu->active;
 			/* no menu key selected, regenerate menu */
 			return NULL;
 		case BKEY_DOWN:
+			menu->last_active = menu->active;
 			if (menu->active < menu->count - 1)
 				++menu->active;
 			/* no menu key selected, regenerate menu */
@@ -133,6 +135,17 @@ static char *bootmenu_choice_entry(void *data)
 	return NULL;
 }
 
+static bool bootmenu_need_reprint(void *data)
+{
+	struct bootmenu_data *menu = data;
+	bool need_reprint;
+
+	need_reprint = menu->last_active != menu->active;
+	menu->last_active = menu->active;
+
+	return need_reprint;
+}
+
 static void bootmenu_destroy(struct bootmenu_data *menu)
 {
 	struct bootmenu_entry *iter = menu->first;
@@ -332,6 +345,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
 
 	menu->delay = delay;
 	menu->active = 0;
+	menu->last_active = -1;
 	menu->first = NULL;
 
 	default_str = env_get("bootmenu_default");
@@ -506,7 +520,7 @@ static enum bootmenu_ret bootmenu_show(int delay)
 
 	menu = menu_create(NULL, bootmenu->delay, 1, menu_display_statusline,
 			   bootmenu_print_entry, bootmenu_choice_entry,
-			   NULL, bootmenu);
+			   bootmenu_need_reprint, bootmenu);
 	if (!menu) {
 		bootmenu_destroy(bootmenu);
 		return BOOTMENU_RET_FAIL;
diff --git a/include/menu.h b/include/menu.h
index 79643af272b..6cede89b950 100644
--- a/include/menu.h
+++ b/include/menu.h
@@ -40,6 +40,7 @@ int menu_show(int bootdelay);
 struct bootmenu_data {
 	int delay;			/* delay for autoboot */
 	int active;			/* active menu entry */
+	int last_active;		/* last active menu entry */
 	int count;			/* total count of menu entries */
 	struct bootmenu_entry *first;	/* first menu entry */
 };
-- 
2.45.2

Re: [PATCH v2 1/3] menu: fix the logic checking whether ESC key is pressed

[Simon Glass]

On Tue, 29 Oct 2024 at 10:47, Weijie Gao <weijie.gao@mediatek.com> wrote:
>
> It's observed that the bootmenu on a serial console sometimes
> incorrectly quitted with superfluous characters filled to command
> line input:
>
> >  *** U-Boot Boot Menu ***
> >
> >      1. Startup system (Default)
> >      2. Upgrade firmware
> >      3. Upgrade ATF BL2
> >      4. Upgrade ATF FIP
> >      5. Load image
> >      0. U-Boot console
> >
> >
> >  Press UP/DOWN to move, ENTER to select, ESC to quit
> >MT7988> [B
>
> Analysis shows it was caused by the wrong logic of bootmenu_loop:
>
> At first the bootmenu_loop received the first ESC char correctly.
>
> However, during the second call to bootmenu_loop, there's no data
> in the UART Rx FIFO. Due to the low baudrate, the second char of
> the down array key sequence hasn't be fully received.
>
> But bootmenu_loop just did a mdelay(10), and then treated it as a
> single ESC key press event. It didn't even try tstc() again after
> the 10ms timeout.
>
> This patch fixes this issue by letting bootmenu_loop check tstc()
> twice.
>
> Tested-By: E Shattow <lucent@gmail.com>
> Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
> ---
>  common/menu.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/common/menu.c b/common/menu.c
> index 8cc9bf06d9c..48ab7f0f398 100644
> --- a/common/menu.c
> +++ b/common/menu.c
> @@ -525,14 +525,15 @@ enum bootmenu_key bootmenu_loop(struct bootmenu_data *menu,
>                                 struct cli_ch_state *cch)
>  {
>         enum bootmenu_key key;
> -       int c;
> +       int c, errchar = 0;
>
>         c = cli_ch_process(cch, 0);
>         if (!c) {
>                 while (!c && !tstc()) {
>                         schedule();
>                         mdelay(10);
> -                       c = cli_ch_process(cch, -ETIMEDOUT);
> +                       c = cli_ch_process(cch, errchar);
> +                       errchar = -ETIMEDOUT;
>                 }
>                 if (!c) {
>                         c = getchar();
> --
> 2.45.2
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 2/3] menu: add support to check if menu needs to be reprinted

[Daniel Golle]

On Tue, Oct 29, 2024 at 05:47:16PM +0800, Weijie Gao wrote:
> This patch adds a new callback named need_reprint for menu.
> The need_reprint will be called before printing the menu. If the
> callback exists and returns FALSE, menu printing will be canceled.
> 
> This is very useful if the menu was not changed. It can save time
> for serial-based menu to handle more input data.
> 
> Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>

Reviewed-by: Daniel Golle <daniel@makrotopia.org>
Tested-by: Daniel Golle <daniel@makrotopia.org>

> ---
>  boot/pxe_utils.c |  2 +-
>  cmd/bootmenu.c   |  2 +-
>  cmd/eficonfig.c  |  2 +-
>  common/menu.c    | 11 +++++++++++
>  include/menu.h   |  1 +
>  5 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
> index d6a4b2cb859..3ae17553c6d 100644
> --- a/boot/pxe_utils.c
> +++ b/boot/pxe_utils.c
> @@ -1474,7 +1474,7 @@ static struct menu *pxe_menu_to_menu(struct pxe_menu *cfg)
>  	 * Create a menu and add items for all the labels.
>  	 */
>  	m = menu_create(cfg->title, DIV_ROUND_UP(cfg->timeout, 10),
> -			cfg->prompt, NULL, label_print, NULL, NULL);
> +			cfg->prompt, NULL, label_print, NULL, NULL, NULL);
>  	if (!m)
>  		return NULL;
>  
> diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
> index 977a04b7d76..c99605f3398 100644
> --- a/cmd/bootmenu.c
> +++ b/cmd/bootmenu.c
> @@ -506,7 +506,7 @@ static enum bootmenu_ret bootmenu_show(int delay)
>  
>  	menu = menu_create(NULL, bootmenu->delay, 1, menu_display_statusline,
>  			   bootmenu_print_entry, bootmenu_choice_entry,
> -			   bootmenu);
> +			   NULL, bootmenu);
>  	if (!menu) {
>  		bootmenu_destroy(bootmenu);
>  		return BOOTMENU_RET_FAIL;
> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> index bea09e4ecc7..498653b778d 100644
> --- a/cmd/eficonfig.c
> +++ b/cmd/eficonfig.c
> @@ -443,7 +443,7 @@ efi_status_t eficonfig_process_common(struct efimenu *efi_menu,
>  		efi_menu->menu_desc = menu_desc;
>  
>  	menu = menu_create(NULL, 0, 1, display_statusline, item_data_print,
> -			   item_choice, efi_menu);
> +			   item_choice, NULL, efi_menu);
>  	if (!menu)
>  		return EFI_INVALID_PARAMETER;
>  
> diff --git a/common/menu.c b/common/menu.c
> index 48ab7f0f398..5a2126aa01a 100644
> --- a/common/menu.c
> +++ b/common/menu.c
> @@ -43,6 +43,7 @@ struct menu {
>  	void (*display_statusline)(struct menu *);
>  	void (*item_data_print)(void *);
>  	char *(*item_choice)(void *);
> +	bool (*need_reprint)(void *);
>  	void *item_choice_data;
>  	struct list_head items;
>  	int item_cnt;
> @@ -117,6 +118,11 @@ static inline void *menu_item_destroy(struct menu *m,
>   */
>  static inline void menu_display(struct menu *m)
>  {
> +	if (m->need_reprint) {
> +		if (!m->need_reprint(m->item_choice_data))
> +			return;
> +	}
> +
>  	if (m->title) {
>  		puts(m->title);
>  		putc('\n');
> @@ -362,6 +368,9 @@ int menu_item_add(struct menu *m, char *item_key, void *item_data)
>   * item. Returns a key string corresponding to the chosen item or NULL if
>   * no item has been selected.
>   *
> + * need_reprint - If not NULL, will be called before printing the menu.
> + * Returning FALSE means the menu does not need reprint.
> + *
>   * item_choice_data - Will be passed as the argument to the item_choice function
>   *
>   * Returns a pointer to the menu if successful, or NULL if there is
> @@ -371,6 +380,7 @@ struct menu *menu_create(char *title, int timeout, int prompt,
>  				void (*display_statusline)(struct menu *),
>  				void (*item_data_print)(void *),
>  				char *(*item_choice)(void *),
> +				bool (*need_reprint)(void *),
>  				void *item_choice_data)
>  {
>  	struct menu *m;
> @@ -386,6 +396,7 @@ struct menu *menu_create(char *title, int timeout, int prompt,
>  	m->display_statusline = display_statusline;
>  	m->item_data_print = item_data_print;
>  	m->item_choice = item_choice;
> +	m->need_reprint = need_reprint;
>  	m->item_choice_data = item_choice_data;
>  	m->item_cnt = 0;
>  
> diff --git a/include/menu.h b/include/menu.h
> index 6571c39b143..79643af272b 100644
> --- a/include/menu.h
> +++ b/include/menu.h
> @@ -13,6 +13,7 @@ struct menu *menu_create(char *title, int timeout, int prompt,
>  				void (*display_statusline)(struct menu *),
>  				void (*item_data_print)(void *),
>  				char *(*item_choice)(void *),
> +				bool (*need_reprint)(void *),
>  				void *item_choice_data);
>  int menu_default_set(struct menu *m, char *item_key);
>  int menu_get_choice(struct menu *m, void **choice);
> -- 
> 2.45.2
> 

Re: [PATCH v2 3/3] bootmenu: add reprint check

[Daniel Golle]

On Tue, Oct 29, 2024 at 05:47:22PM +0800, Weijie Gao wrote:
> Record the last active menu item and check if it equals to the
> current selected item before reprint.
> 
> Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>

Reviewed-by: Daniel Golle <daniel@makrotopia.org>
Tested-by: Daniel Golle <daniel@makrotopia.org>

> ---
>  cmd/bootmenu.c | 16 +++++++++++++++-
>  include/menu.h |  1 +
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
> index c99605f3398..ffa63a4628d 100644
> --- a/cmd/bootmenu.c
> +++ b/cmd/bootmenu.c
> @@ -103,11 +103,13 @@ static char *bootmenu_choice_entry(void *data)
>  
>  		switch (key) {
>  		case BKEY_UP:
> +			menu->last_active = menu->active;
>  			if (menu->active > 0)
>  				--menu->active;
>  			/* no menu key selected, regenerate menu */
>  			return NULL;
>  		case BKEY_DOWN:
> +			menu->last_active = menu->active;
>  			if (menu->active < menu->count - 1)
>  				++menu->active;
>  			/* no menu key selected, regenerate menu */
> @@ -133,6 +135,17 @@ static char *bootmenu_choice_entry(void *data)
>  	return NULL;
>  }
>  
> +static bool bootmenu_need_reprint(void *data)
> +{
> +	struct bootmenu_data *menu = data;
> +	bool need_reprint;
> +
> +	need_reprint = menu->last_active != menu->active;
> +	menu->last_active = menu->active;
> +
> +	return need_reprint;
> +}
> +
>  static void bootmenu_destroy(struct bootmenu_data *menu)
>  {
>  	struct bootmenu_entry *iter = menu->first;
> @@ -332,6 +345,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
>  
>  	menu->delay = delay;
>  	menu->active = 0;
> +	menu->last_active = -1;
>  	menu->first = NULL;
>  
>  	default_str = env_get("bootmenu_default");
> @@ -506,7 +520,7 @@ static enum bootmenu_ret bootmenu_show(int delay)
>  
>  	menu = menu_create(NULL, bootmenu->delay, 1, menu_display_statusline,
>  			   bootmenu_print_entry, bootmenu_choice_entry,
> -			   NULL, bootmenu);
> +			   bootmenu_need_reprint, bootmenu);
>  	if (!menu) {
>  		bootmenu_destroy(bootmenu);
>  		return BOOTMENU_RET_FAIL;
> diff --git a/include/menu.h b/include/menu.h
> index 79643af272b..6cede89b950 100644
> --- a/include/menu.h
> +++ b/include/menu.h
> @@ -40,6 +40,7 @@ int menu_show(int bootdelay);
>  struct bootmenu_data {
>  	int delay;			/* delay for autoboot */
>  	int active;			/* active menu entry */
> +	int last_active;		/* last active menu entry */
>  	int count;			/* total count of menu entries */
>  	struct bootmenu_entry *first;	/* first menu entry */
>  };
> -- 
> 2.45.2
> 

Re: [PATCH v2 1/3] menu: fix the logic checking whether ESC key is pressed

[Daniel Golle]

On Tue, Oct 29, 2024 at 05:47:10PM +0800, Weijie Gao wrote:
> It's observed that the bootmenu on a serial console sometimes
> incorrectly quitted with superfluous characters filled to command
> line input:
> 
> >  *** U-Boot Boot Menu ***
> >
> >      1. Startup system (Default)
> >      2. Upgrade firmware
> >      3. Upgrade ATF BL2
> >      4. Upgrade ATF FIP
> >      5. Load image
> >      0. U-Boot console
> >
> >
> >  Press UP/DOWN to move, ENTER to select, ESC to quit
> >MT7988> [B
> 
> Analysis shows it was caused by the wrong logic of bootmenu_loop:
> 
> At first the bootmenu_loop received the first ESC char correctly.
> 
> However, during the second call to bootmenu_loop, there's no data
> in the UART Rx FIFO. Due to the low baudrate, the second char of
> the down array key sequence hasn't be fully received.
> 
> But bootmenu_loop just did a mdelay(10), and then treated it as a
> single ESC key press event. It didn't even try tstc() again after
> the 10ms timeout.
> 
> This patch fixes this issue by letting bootmenu_loop check tstc()
> twice.
> 
> Tested-By: E Shattow <lucent@gmail.com>
> Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>

Reviewed-by: Daniel Golle <daniel@makrotopia.org>
Tested-by: Daniel Golle <daniel@makrotopia.org>

Together with 2/3 and 3/3 this now avoids dropping into the U-Boot shell
as long as the menu is short enough. In case of menus with 8 entries of
more I still manage to drop into shell when holding the arrow down key
before the last item has been reached.
That being said, it's already a very big improvement that this now only
happens when holding down a button traversing over a menu with a high
number of items.


> ---
>  common/menu.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/common/menu.c b/common/menu.c
> index 8cc9bf06d9c..48ab7f0f398 100644
> --- a/common/menu.c
> +++ b/common/menu.c
> @@ -525,14 +525,15 @@ enum bootmenu_key bootmenu_loop(struct bootmenu_data *menu,
>  				struct cli_ch_state *cch)
>  {
>  	enum bootmenu_key key;
> -	int c;
> +	int c, errchar = 0;
>  
>  	c = cli_ch_process(cch, 0);
>  	if (!c) {
>  		while (!c && !tstc()) {
>  			schedule();
>  			mdelay(10);
> -			c = cli_ch_process(cch, -ETIMEDOUT);
> +			c = cli_ch_process(cch, errchar);
> +			errchar = -ETIMEDOUT;
>  		}
>  		if (!c) {
>  			c = getchar();
> -- 
> 2.45.2
> 

Re: [PATCH v2 1/3] menu: fix the logic checking whether ESC key is pressed

[Weijie Gao]

Hi Daniel,

On Thu, 2024-10-31 at 19:16 +0000, Daniel Golle wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Tue, Oct 29, 2024 at 05:47:10PM +0800, Weijie Gao wrote:
> > It's observed that the bootmenu on a serial console sometimes
> > incorrectly quitted with superfluous characters filled to command
> > line input:
> > 
> > >  *** U-Boot Boot Menu ***
> > > 
> > >      1. Startup system (Default)
> > >      2. Upgrade firmware
> > >      3. Upgrade ATF BL2
> > >      4. Upgrade ATF FIP
> > >      5. Load image
> > >      0. U-Boot console
> > > 
> > > 
> > >  Press UP/DOWN to move, ENTER to select, ESC to quit
> > > MT7988> [B
> > 
> > Analysis shows it was caused by the wrong logic of bootmenu_loop:
> > 
> > At first the bootmenu_loop received the first ESC char correctly.
> > 
> > However, during the second call to bootmenu_loop, there's no data
> > in the UART Rx FIFO. Due to the low baudrate, the second char of
> > the down array key sequence hasn't be fully received.
> > 
> > But bootmenu_loop just did a mdelay(10), and then treated it as a
> > single ESC key press event. It didn't even try tstc() again after
> > the 10ms timeout.
> > 
> > This patch fixes this issue by letting bootmenu_loop check tstc()
> > twice.
> > 
> > Tested-By: E Shattow <lucent@gmail.com>
> > Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
> 
> Reviewed-by: Daniel Golle <daniel@makrotopia.org>
> Tested-by: Daniel Golle <daniel@makrotopia.org>
> 
> Together with 2/3 and 3/3 this now avoids dropping into the U-Boot
> shell
> as long as the menu is short enough. In case of menus with 8 entries
> of
> more I still manage to drop into shell when holding the arrow down
> key
> before the last item has been reached.
> That being said, it's already a very big improvement that this now
> only
> happens when holding down a button traversing over a menu with a high
> number of items.

I didn't encounter with this.
Did you try enabling CONFIG_SERIAL_RX_BUFFER?

> 
> 
> > ---
> >  common/menu.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/common/menu.c b/common/menu.c
> > index 8cc9bf06d9c..48ab7f0f398 100644
> > --- a/common/menu.c
> > +++ b/common/menu.c
> > @@ -525,14 +525,15 @@ enum bootmenu_key bootmenu_loop(struct
> > bootmenu_data *menu,
> >                               struct cli_ch_state *cch)
> >  {
> >       enum bootmenu_key key;
> > -     int c;
> > +     int c, errchar = 0;
> > 
> >       c = cli_ch_process(cch, 0);
> >       if (!c) {
> >               while (!c && !tstc()) {
> >                       schedule();
> >                       mdelay(10);
> > -                     c = cli_ch_process(cch, -ETIMEDOUT);
> > +                     c = cli_ch_process(cch, errchar);
> > +                     errchar = -ETIMEDOUT;
> >               }
> >               if (!c) {
> >                       c = getchar();
> > --
> > 2.45.2
> > 

Re: [PATCH v2 1/3] menu: fix the logic checking whether ESC key is pressed

[Daniel Golle]

On Fri, Nov 01, 2024 at 03:20:11PM +0800, Weijie Gao wrote:
> Hi Daniel,
> 
> On Thu, 2024-10-31 at 19:16 +0000, Daniel Golle wrote:
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> > 
> > 
> > On Tue, Oct 29, 2024 at 05:47:10PM +0800, Weijie Gao wrote:
> > > It's observed that the bootmenu on a serial console sometimes
> > > incorrectly quitted with superfluous characters filled to command
> > > line input:
> > > 
> > > >  *** U-Boot Boot Menu ***
> > > > 
> > > >      1. Startup system (Default)
> > > >      2. Upgrade firmware
> > > >      3. Upgrade ATF BL2
> > > >      4. Upgrade ATF FIP
> > > >      5. Load image
> > > >      0. U-Boot console
> > > > 
> > > > 
> > > >  Press UP/DOWN to move, ENTER to select, ESC to quit
> > > > MT7988> [B
> > > 
> > > Analysis shows it was caused by the wrong logic of bootmenu_loop:
> > > 
> > > At first the bootmenu_loop received the first ESC char correctly.
> > > 
> > > However, during the second call to bootmenu_loop, there's no data
> > > in the UART Rx FIFO. Due to the low baudrate, the second char of
> > > the down array key sequence hasn't be fully received.
> > > 
> > > But bootmenu_loop just did a mdelay(10), and then treated it as a
> > > single ESC key press event. It didn't even try tstc() again after
> > > the 10ms timeout.
> > > 
> > > This patch fixes this issue by letting bootmenu_loop check tstc()
> > > twice.
> > > 
> > > Tested-By: E Shattow <lucent@gmail.com>
> > > Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
> > 
> > Reviewed-by: Daniel Golle <daniel@makrotopia.org>
> > Tested-by: Daniel Golle <daniel@makrotopia.org>
> > 
> > Together with 2/3 and 3/3 this now avoids dropping into the U-Boot
> > shell
> > as long as the menu is short enough. In case of menus with 8 entries
> > of
> > more I still manage to drop into shell when holding the arrow down
> > key
> > before the last item has been reached.
> > That being said, it's already a very big improvement that this now
> > only
> > happens when holding down a button traversing over a menu with a high
> > number of items.
> 
> I didn't encounter with this.
> Did you try enabling CONFIG_SERIAL_RX_BUFFER?

Combined with CONFIG_SERIAL_RX_BUFFER it does resolve the issue and I no
longer manage to get dropped into console.

Thank you a lot for taking care of this!

Re: [PATCH v2 1/3] menu: fix the logic checking whether ESC key is pressed

[Tom Rini]

On Tue, 29 Oct 2024 17:47:10 +0800, Weijie Gao wrote:

> It's observed that the bootmenu on a serial console sometimes
> incorrectly quitted with superfluous characters filled to command
> line input:
> 
> >  *** U-Boot Boot Menu ***
> >
> >      1. Startup system (Default)
> >      2. Upgrade firmware
> >      3. Upgrade ATF BL2
> >      4. Upgrade ATF FIP
> >      5. Load image
> >      0. U-Boot console
> >
> >
> >  Press UP/DOWN to move, ENTER to select, ESC to quit
> >MT7988> [B
> 
> [...]

Applied to u-boot/master, thanks!

-- 
Tom