Re: [edk2-devel] [PATCH 1/1] MdePkg: Use ANSI colors to indicate debug message severity

["Andrew Fish" <afish@apple.com>]

[-- Attachment #1: Type: text/plain, Size: 10800 bytes --]



> On Sep 1, 2022, at 4:52 PM, Rebecca Cran <quic_rcran@quicinc.com> wrote:
> 
> Thanks. I've just sent out a v2 that introduces a
> 
> PcdTerminalSupportsAnsiSequences FeatureFlag.
> 
> Let me know if you'd prefer a different name: I wondered about something like PcdConOutAnsiSeqSupport for example.
> 

Rebecca,

I think this feature flag is going to control the output of edk2 DEBUG. In an abstract sense DEBUG is not Terminal or ConOut. It could be mapped to the same device, but it does not have to be. 

So maybe PcdDebugAnsiSeqSupport or DebugLibAnsiSeqSupport? 

Thanks,

Andrew Fish

> -- 
> Rebecca Cran
> 
> On 8/29/22 17:29, Kinney, Michael D wrote:
>> Hi Rebecca,
>> 
>> I think this is a good idea to improve readability.
>> 
>> However, I think there is an assumption today that debug
>> message output is just ASCII text with no assumptions on
>> a terminal type and no assumption on screen width or screen
>> height and no support for cursor control.
>> 
>> This change would only make sense if the device the
>> serial messages are sent supports ANSI color codes.
>> 
>> I think we should make this a build time configuration
>> option to add the ANSI sequences.  I suggest a FeatureFlag
>> PCD with a default of disabled to preserve current behavior
>> and platforms can opt into ANSI color coded debug messages
>> in their DSC file.
>> 
>> Thanks,
>> 
>> Mike
>> 
>>> -----Original Message-----
>>> From: Rebecca Cran <rebecca@quicinc.com>
>>> Sent: Monday, August 29, 2022 3:12 PM
>>> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang
>>> <zhiguang.liu@intel.com>
>>> Cc: Rebecca Cran <rebecca@quicinc.com>
>>> Subject: [PATCH 1/1] MdePkg: Use ANSI colors to indicate debug message severity
>>> 
>>> There currently isn't a way to differentiate the different
>>> levels of DEBUG output: DEBUG_ERROR, DEBUG_WARN, DEBUG_INFO
>>> etc.
>>> 
>>> To improve this, wrap DEBUG_ERROR and DEBUG_WARN level
>>> messages in ANSI color code escape sequences. DEBUG_ERROR
>>> messages will be displayed in red text, and DEBUG_WARN
>>> in orange (the escape code is for the yellow color palette
>>> entry, but it gets displayed as orange).
>>> 
>>> Signed-off-by: Rebecca Cran <rebecca@quicinc.com>
>>> ---
>>>  MdePkg/Include/Library/PrintLib.h                |  39 ++++++
>>>  MdePkg/Library/BaseDebugLibSerialPort/DebugLib.c |   2 +
>>>  MdePkg/Library/BasePrintLib/PrintLib.c           | 128 ++++++++++++++++++++
>>>  MdePkg/Library/UefiDebugLibConOut/DebugLib.c     |   2 +
>>>  4 files changed, 171 insertions(+)
>>> 
>>> diff --git a/MdePkg/Include/Library/PrintLib.h b/MdePkg/Include/Library/PrintLib.h
>>> index 8d523cac528d..06d9761df897 100644
>>> --- a/MdePkg/Include/Library/PrintLib.h
>>> +++ b/MdePkg/Include/Library/PrintLib.h
>>> @@ -2,6 +2,7 @@
>>>    Provides services to print a formatted string to a buffer. All combinations of
>>>    Unicode and ASCII strings are supported.
>>> 
>>> +Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.<BR>
>>>  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>>>  SPDX-License-Identifier: BSD-2-Clause-Patent
>>> 
>>> @@ -931,4 +932,42 @@ SPrintLengthAsciiFormat (
>>>    IN  VA_LIST      Marker
>>>    );
>>> 
>>> +/**
>>> +  Wraps a message with ANSI color escape codes.
>>> +
>>> +  @param String     The string to wrap.
>>> +  @param StringLen  The size of the String buffer in characters.
>>> +  @param ErrorLevel The error level.
>>> +
>>> +  @retval RETURN_SUCCESS          The string was successfully updated.
>>> +  @retval RETURN_BUFFER_TOO_SMALL The buffer is too small.
>>> +
>>> +**/
>>> +RETURN_STATUS
>>> +EFIAPI
>>> +AsciiDebugGetColorString (
>>> +  IN OUT CHAR8  *String,
>>> +  IN UINTN      StringLen,
>>> +  IN UINTN      ErrorLevel
>>> +  );
>>> +
>>> +/**
>>> +  Wraps a message with ANSI color escape codes.
>>> +
>>> +  @param String     The string to wrap.
>>> +  @param StringLen  The size of the String buffer in Unicode characters.
>>> +  @param ErrorLevel The error level.
>>> +
>>> +  @retval RETURN_SUCCESS          The string was successfully updated.
>>> +  @retval RETURN_BUFFER_TOO_SMALL The buffer is too small.
>>> +
>>> +**/
>>> +RETURN_STATUS
>>> +EFIAPI
>>> +UnicodeDebugGetColorString (
>>> +  IN OUT CHAR16  *String,
>>> +  IN UINTN       StringLen,
>>> +  IN UINTN       ErrorLevel
>>> +  );
>>> +
>>>  #endif
>>> diff --git a/MdePkg/Library/BaseDebugLibSerialPort/DebugLib.c b/MdePkg/Library/BaseDebugLibSerialPort/DebugLib.c
>>> index bd5686947712..d599d16f5b7a 100644
>>> --- a/MdePkg/Library/BaseDebugLibSerialPort/DebugLib.c
>>> +++ b/MdePkg/Library/BaseDebugLibSerialPort/DebugLib.c
>>> @@ -125,6 +125,8 @@ DebugPrintMarker (
>>>      AsciiBSPrint (Buffer, sizeof (Buffer), Format, BaseListMarker);
>>>    }
>>> 
>>> +  AsciiDebugGetColorString (Buffer, MAX_DEBUG_MESSAGE_LENGTH, ErrorLevel);
>>> +
>>>    //
>>>    // Send the print string to a Serial Port
>>>    //
>>> diff --git a/MdePkg/Library/BasePrintLib/PrintLib.c b/MdePkg/Library/BasePrintLib/PrintLib.c
>>> index e6f4042bb90b..95135d45aff9 100644
>>> --- a/MdePkg/Library/BasePrintLib/PrintLib.c
>>> +++ b/MdePkg/Library/BasePrintLib/PrintLib.c
>>> @@ -1,12 +1,15 @@
>>>  /** @file
>>>    Base Print Library instance implementation.
>>> 
>>> +  Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.<BR>
>>>    Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>>>    Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
>>>    SPDX-License-Identifier: BSD-2-Clause-Patent
>>> 
>>>  **/
>>> 
>>> +#include <Library/BaseMemoryLib.h>
>>> +
>>>  #include "PrintLibInternal.h"
>>> 
>>>  //
>>> @@ -19,6 +22,19 @@ VA_LIST  gNullVaList;
>>> 
>>>  #define ASSERT_UNICODE_BUFFER(Buffer)  ASSERT ((((UINTN) (Buffer)) & 0x01) == 0)
>>> 
>>> +//
>>> +// Define the maximum debug and assert message length that this library supports
>>> +//
>>> +#define MAX_DEBUG_MESSAGE_LENGTH  0x100
>>> +
>>> +#define ASCII_RED_ESC_SEQ     "\033[31m"
>>> +#define ASCII_YELLOW_ESC_SEQ  "\033[33m"
>>> +#define ASCII_END_ESC_SEQ     "\033[0m"
>>> +
>>> +#define RED_ESC_SEQ     L"\033[31m"
>>> +#define YELLOW_ESC_SEQ  L"\033[33m"
>>> +#define END_ESC_SEQ     L"\033[0m"
>>> +
>>>  /**
>>>    Produces a Null-terminated Unicode string in an output buffer based on
>>>    a Null-terminated Unicode format string and a VA_LIST argument list.
>>> @@ -834,3 +850,115 @@ SPrintLengthAsciiFormat (
>>>  {
>>>    return BasePrintLibSPrintMarker (NULL, 0, OUTPUT_UNICODE | COUNT_ONLY_NO_PRINT, (CHAR8 *)FormatString, Marker, NULL);
>>>  }
>>> +
>>> +/**
>>> +  Wraps a message with ANSI color escape codes.
>>> +
>>> +  @param String     The string to wrap.
>>> +  @param StringLen  The size of the String buffer in characters.
>>> +  @param ErrorLevel The error level.
>>> +
>>> +  @retval RETURN_SUCCESS          The string was successfully updated.
>>> +  @retval RETURN_BUFFER_TOO_SMALL The buffer is too small.
>>> +
>>> +**/
>>> +RETURN_STATUS
>>> +EFIAPI
>>> +AsciiDebugGetColorString (
>>> +  IN OUT CHAR8  *String,
>>> +  IN UINTN      StringLen,
>>> +  IN UINTN      ErrorLevel
>>> +  )
>>> +{
>>> +  CHAR8  Buffer[MAX_DEBUG_MESSAGE_LENGTH];
>>> +  UINTN  ReqBufferLen;
>>> +
>>> +  ReqBufferLen = AsciiStrLen (String) +
>>> +                 AsciiStrLen (ASCII_RED_ESC_SEQ) +
>>> +                 AsciiStrLen (ASCII_END_ESC_SEQ) +
>>> +                 1;
>>> +
>>> +  if (StringLen < ReqBufferLen) {
>>> +    return RETURN_BUFFER_TOO_SMALL;
>>> +  }
>>> +
>>> +  ZeroMem (Buffer, sizeof (Buffer));
>>> +
>>> +  switch (ErrorLevel) {
>>> +    case DEBUG_WARN:
>>> +      AsciiStrCpyS (Buffer, MAX_DEBUG_MESSAGE_LENGTH, ASCII_YELLOW_ESC_SEQ);
>>> +      break;
>>> +    case DEBUG_ERROR:
>>> +      AsciiStrCpyS (Buffer, MAX_DEBUG_MESSAGE_LENGTH, ASCII_RED_ESC_SEQ);
>>> +      break;
>>> +  }
>>> +
>>> +  AsciiStrCatS (Buffer, MAX_DEBUG_MESSAGE_LENGTH, String);
>>> +
>>> +  switch (ErrorLevel) {
>>> +    case DEBUG_WARN:
>>> +    case DEBUG_ERROR:
>>> +      AsciiStrCatS (Buffer, MAX_DEBUG_MESSAGE_LENGTH, ASCII_END_ESC_SEQ);
>>> +      break;
>>> +  }
>>> +
>>> +  AsciiStrCpyS (String, StringLen, Buffer);
>>> +
>>> +  return RETURN_SUCCESS;
>>> +}
>>> +
>>> +/**
>>> +  Wraps a message with ANSI color escape codes.
>>> +
>>> +  @param String     The string to wrap.
>>> +  @param StringLen  The size of the String buffer in Unicode characters.
>>> +  @param ErrorLevel The error level.
>>> +
>>> +  @retval RETURN_SUCCESS          The string was successfully updated.
>>> +  @retval RETURN_BUFFER_TOO_SMALL The buffer is too small.
>>> +
>>> +**/
>>> +RETURN_STATUS
>>> +EFIAPI
>>> +UnicodeDebugGetColorString (
>>> +  IN OUT CHAR16  *String,
>>> +  IN UINTN       StringLen,
>>> +  IN UINTN       ErrorLevel
>>> +  )
>>> +{
>>> +  CHAR16  Buffer[MAX_DEBUG_MESSAGE_LENGTH];
>>> +  UINTN   ReqBufferLen;
>>> +
>>> +  ReqBufferLen = StrLen (String) +
>>> +                 StrLen (RED_ESC_SEQ) +
>>> +                 StrLen (END_ESC_SEQ) +
>>> +                 1;
>>> +
>>> +  if (StringLen < ReqBufferLen) {
>>> +    return RETURN_BUFFER_TOO_SMALL;
>>> +  }
>>> +
>>> +  ZeroMem (Buffer, sizeof (Buffer));
>>> +
>>> +  switch (ErrorLevel) {
>>> +    case DEBUG_WARN:
>>> +      StrCpyS (Buffer, MAX_DEBUG_MESSAGE_LENGTH, YELLOW_ESC_SEQ);
>>> +      break;
>>> +    case DEBUG_ERROR:
>>> +      StrCpyS (Buffer, MAX_DEBUG_MESSAGE_LENGTH, RED_ESC_SEQ);
>>> +      break;
>>> +  }
>>> +
>>> +  StrCatS (Buffer, MAX_DEBUG_MESSAGE_LENGTH, String);
>>> +
>>> +  switch (ErrorLevel) {
>>> +    case DEBUG_WARN:
>>> +    case DEBUG_ERROR:
>>> +      StrCatS (Buffer, MAX_DEBUG_MESSAGE_LENGTH, END_ESC_SEQ);
>>> +      break;
>>> +  }
>>> +
>>> +  StrCpyS (String, StringLen, Buffer);
>>> +
>>> +  return RETURN_SUCCESS;
>>> +}
>>> diff --git a/MdePkg/Library/UefiDebugLibConOut/DebugLib.c b/MdePkg/Library/UefiDebugLibConOut/DebugLib.c
>>> index 65c8dc2b4654..bc902b0e60c6 100644
>>> --- a/MdePkg/Library/UefiDebugLibConOut/DebugLib.c
>>> +++ b/MdePkg/Library/UefiDebugLibConOut/DebugLib.c
>>> @@ -108,6 +108,8 @@ DebugPrintMarker (
>>>        UnicodeBSPrintAsciiFormat (Buffer, sizeof (Buffer), Format, BaseListMarker);
>>>      }
>>> 
>>> +    UnicodeDebugGetColorString (Buffer, MAX_DEBUG_MESSAGE_LENGTH, ErrorLevel);
>>> +
>>>      //
>>>      // Send the print string to the Console Output device
>>>      //
>>> --
>>> 2.30.2
> 
> 
> 
> 


[-- Attachment #2: Type: text/html, Size: 20671 bytes --]