Re: [edk2-devel] [PATCH V3 3/3] MdeModulePkg/BM_UI: Add the new terminal types to related menu

["Dandan Bi" <dandan.bi@intel.com>]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Gao, Zhichao
> Sent: Wednesday, September 18, 2019 1:34 PM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Ni, Ray <ray.ni@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [edk2-devel] [PATCH V3 3/3] MdeModulePkg/BM_UI: Add the new
> terminal types to related menu
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2186
> 
> Add the new introduced terminal types to related setup menu to change the
> terminal type from setup. Most platforms would have its own configure
> setup menu and they need to change it to support these.
> The new introduced terminal types are Linux, XtermR6, VT400 and SCO.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
>  .../BootMaintenanceManager.h                  | 12 ++++---
>  .../BootMaintenanceManagerStrings.uni         | 10 +++++-
>  .../ConsoleOption.c                           | 35 ++++++-------------
>  .../BootMaintenanceManagerUiLib/Data.c        | 16 ++++++---
>  4 files changed, 39 insertions(+), 34 deletions(-)
> 
> diff --git
> a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanc
> eManager.h
> b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanc
> eManager.h
> index ea3cdce794..67847d8bf3 100644
> ---
> a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanc
> eManager.h
> +++
> b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanc
> eMa
> +++ nager.h
> @@ -1,7 +1,7 @@
>  /** @file
>  Header file for boot maintenance module.
> 
> -Copyright (c) 2004 - 2018, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2004 - 2019, Intel Corporation. All rights reserved.<BR>
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -92,7 +92,11 @@ typedef enum _TYPE_OF_TERMINAL {
>    TerminalTypeVt100,
>    TerminalTypeVt100Plus,
>    TerminalTypeVtUtf8,
> -  TerminalTypeTtyTerm
> +  TerminalTypeTtyTerm,
> +  TerminalTypeLinux,
> +  TerminalTypeXtermR6,
> +  TerminalTypeVt400,
> +  TerminalTypeSCO
>  } TYPE_OF_TERMINAL;
> 
>  //
> @@ -1301,12 +1305,12 @@ extern BM_MENU_OPTION
> ConsoleOutMenu;
>  extern BM_MENU_OPTION             ConsoleErrMenu;
>  extern BM_MENU_OPTION             DriverMenu;
>  extern BM_MENU_OPTION             TerminalMenu;
> -extern UINT16                     TerminalType[5];
> +extern UINT16                     TerminalType[9];
>  extern COM_ATTR                   BaudRateList[19];
>  extern COM_ATTR                   DataBitsList[4];
>  extern COM_ATTR                   ParityList[5];
>  extern COM_ATTR                   StopBitsList[3];
> -extern EFI_GUID                   TerminalTypeGuid[5];
> +extern EFI_GUID                   TerminalTypeGuid[9];
>  extern EFI_DEVICE_PATH_PROTOCOL   EndDevicePath[];
>  extern UINT16                     mFlowControlType[2];
>  extern UINT32                     mFlowControlValue[2];
> diff --git
> a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanc
> eManagerStrings.uni
> b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanc
> eManagerStrings.uni
> index 2e67d27bd0..3d47473e6c 100644
> ---
> a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanc
> eManagerStrings.uni
> +++
> b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanc
> eMa
> +++ nagerStrings.uni
> @@ -1,7 +1,7 @@
>  ///** @file
>  //  String definitions for Boot Maintenance Utility.
>  //
> -//  Copyright (c) 2004 - 2018, Intel Corporation. All rights reserved.<BR>
> +//  Copyright (c) 2004 - 2019, Intel Corporation. All rights
> +reserved.<BR>
>  //  SPDX-License-Identifier: BSD-2-Clause-Patent  //  //**/ @@ -233,6
> +233,14 @@
>                                         #language fr-FR  "VT_UTF8"
>  #string STR_COM_TYPE_4                 #language en-US  "TTY_TERM"
>                                         #language fr-FR  "TTY_TERM"
> +#string STR_COM_TYPE_5                 #language en-US  "LINUX"
> +                                       #language fr-FR  "LINUX"
> +#string STR_COM_TYPE_6                 #language en-US  "XTERM_R6"
> +                                       #language fr-FR  "XTERM_R6"
> +#string STR_COM_TYPE_7                 #language en-US  "VT_400"
> +                                       #language fr-FR  "VT_400"
> +#string STR_COM_TYPE_8                 #language en-US  "SCO"
> +                                       #language fr-FR  "SCO"
>  #string STR_RESET                      #language en-US  "Reset System"
>                                         #language fr-FR  "Reset System"
>  #string STR_FORM_GOTO_MAIN             #language en-US  "Go Back To Main
> Page"
> diff --git
> a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/ConsoleOption.c
> b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/ConsoleOption.c
> index 7a53b58771..b0641c5ee9 100644
> ---
> a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/ConsoleOption.c
> +++
> b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/ConsoleOption.c
> @@ -897,6 +897,7 @@ IsTerminalDevicePath (
>    VENDOR_DEVICE_PATH        *Vendor;
>    UART_DEVICE_PATH          *Uart;
>    ACPI_HID_DEVICE_PATH      *Acpi;
> +  UINTN                     Index;
> 
>    IsTerminal = FALSE;
> 
> @@ -929,37 +930,21 @@ IsTerminalDevicePath (
>    }
> 
>    //
> -  // There are four kinds of Terminal types
> +  // There are 9 kinds of Terminal types
>    // check to see whether this devicepath
>    // is one of that type
>    //
> -  if (CompareGuid (&Vendor->Guid, &TerminalTypeGuid[0])) {
> -    *Termi      = TerminalTypePcAnsi;
> -    IsTerminal  = TRUE;
> -  } else {
> -    if (CompareGuid (&Vendor->Guid, &TerminalTypeGuid[1])) {
> -      *Termi      = TerminalTypeVt100;
> -      IsTerminal  = TRUE;
> -    } else {
> -      if (CompareGuid (&Vendor->Guid, &TerminalTypeGuid[2])) {
> -        *Termi      = TerminalTypeVt100Plus;
> -        IsTerminal  = TRUE;
> -      } else {
> -        if (CompareGuid (&Vendor->Guid, &TerminalTypeGuid[3])) {
> -          *Termi      = TerminalTypeVtUtf8;
> -          IsTerminal  = TRUE;
> -        } else {
> -          if (CompareGuid (&Vendor->Guid, &TerminalTypeGuid[4])) {
> -            *Termi      = TerminalTypeTtyTerm;
> -            IsTerminal  = TRUE;
> -          } else {
> -            IsTerminal = FALSE;
> -          }
> -        }
> -      }
> +  for (Index = 0; Index < ARRAY_SIZE (TerminalTypeGuid); Index++) {
> +    if (CompareGuid (&Vendor->Guid, &TerminalTypeGuid[Index])) {
> +      *Termi = Index;
> +      IsTerminal = TRUE;

 Hi Zhichao,

Thanks for simplifying this code logic. 
One comment here, I think you miss the "break" statement here, or the Index will increase to 9 and then "if (Index == ARRAY_SIZE (TerminalTypeGuid))" will be always true and "IsTerminal" variable is always FALSE, this is not the expected behavior. Please double check the code logic.

Thanks,
Dandan

>      }
>    }
> 
> +  if (Index == ARRAY_SIZE (TerminalTypeGuid)) {
> +    IsTerminal = FALSE;
> +  }
> +
>    if (!IsTerminal) {
>      return FALSE;
>    }
> diff --git a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/Data.c
> b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/Data.c
> index b9d86b09d8..21c4ef3d55 100644
> --- a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/Data.c
> +++ b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/Data.c
> @@ -1,7 +1,7 @@
>  /** @file
>  Define some data used for Boot Maint
> 
> -Copyright (c) 2004 - 2015, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2004 - 2019, Intel Corporation. All rights reserved.<BR>
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -16,12 +16,16 @@ EFI_IFR_GUID_LABEL  *mEndLabel = NULL;  ///  ///
> Terminal type string token storage  ///
> -UINT16              TerminalType[] = {
> +UINT16              TerminalType[9] = {
>    STRING_TOKEN(STR_COM_TYPE_0),
>    STRING_TOKEN(STR_COM_TYPE_1),
>    STRING_TOKEN(STR_COM_TYPE_2),
>    STRING_TOKEN(STR_COM_TYPE_3),
>    STRING_TOKEN(STR_COM_TYPE_4),
> +  STRING_TOKEN(STR_COM_TYPE_5),
> +  STRING_TOKEN(STR_COM_TYPE_6),
> +  STRING_TOKEN(STR_COM_TYPE_7),
> +  STRING_TOKEN(STR_COM_TYPE_8),
>  };
> 
>  ///
> @@ -248,10 +252,14 @@ COM_ATTR            StopBitsList[3] = {
>  ///
>  /// Guid for messaging path, used in Serial port setting.
>  ///
> -EFI_GUID            TerminalTypeGuid[] = {
> +EFI_GUID            TerminalTypeGuid[9] = {
>    DEVICE_PATH_MESSAGING_PC_ANSI,
>    DEVICE_PATH_MESSAGING_VT_100,
>    DEVICE_PATH_MESSAGING_VT_100_PLUS,
>    DEVICE_PATH_MESSAGING_VT_UTF8,
> -  EFI_TTY_TERM_GUID
> +  EFI_TTY_TERM_GUID,
> +  EDKII_LINUX_TERM_GUID,
> +  EDKII_XTERM_R6_GUID,
> +  EDKII_VT400_GUID,
> +  EDKII_SCO_TERM_GUID
>  };
> --
> 2.21.0.windows.1
> 
> 
>