[edk2,V2,3/4] Accept VT220 DEL and function keys for TTY terminal type

Message ID 1436223879-27956-4-git-send-email-roy.franz@linaro.org
State New
Headers show

Commit Message

Roy Franz July 6, 2015, 11:04 p.m.
Accept the VT220 escape code [3~ as backspace for TtyTerm terminals.  This is
sent by many Linux terminals by default.  Also accept VT220 function keys
F1-F12, and VT100 F1-F4 keys as these are commonly sent by Linux terminals.
The VT220 escape codes are longer, and variable length so a new state is added
to the state machine along with a variable to construct the multibyte escape
sequence.
There are currently no ambiguous escape sequence prefixes accepted, so the TTY
terminal accepts escape sequences for a variety of terminals.  The goal is to
'just work' with as many terminals as possible, rather than properly emulating
any specific terminal.  Backspace, Del, and F10 have been tested on xterm,
rxvt, tmux, and screen.
Note: The existing vt100 function key handling does not match the vt100
documentation that I found, so I added the TTY terminal handling
of VT100 F1-F4 (really PF1-PF4 on vt100) separately.  The vt100
has no F5-F10 keys, so I don't know what the current vt100 code
is based on.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.0
---
 MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h      |  1 +
 MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c | 95 +++++++++++++++++++-
 2 files changed, 95 insertions(+), 1 deletion(-)

Comments

Ard Biesheuvel July 7, 2015, 1:18 p.m. | #1
On 7 July 2015 at 01:04, Roy Franz <roy.franz@linaro.org> wrote:
> Accept the VT220 escape code [3~ as backspace for TtyTerm terminals.  This is
> sent by many Linux terminals by default.  Also accept VT220 function keys
> F1-F12, and VT100 F1-F4 keys as these are commonly sent by Linux terminals.
> The VT220 escape codes are longer, and variable length so a new state is added
> to the state machine along with a variable to construct the multibyte escape
> sequence.
> There are currently no ambiguous escape sequence prefixes accepted, so the TTY
> terminal accepts escape sequences for a variety of terminals.  The goal is to
> 'just work' with as many terminals as possible, rather than properly emulating
> any specific terminal.  Backspace, Del, and F10 have been tested on xterm,
> rxvt, tmux, and screen.
> Note: The existing vt100 function key handling does not match the vt100
> documentation that I found, so I added the TTY terminal handling
> of VT100 F1-F4 (really PF1-PF4 on vt100) separately.  The vt100
> has no F5-F10 keys, so I don't know what the current vt100 code
> is based on.
>
> Signed-off-by: Roy Franz <roy.franz@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.0

Same here ^^^
(and in patch 1/4)

> ---
>  MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h      |  1 +
>  MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c | 95 +++++++++++++++++++-
>  2 files changed, 95 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
> index 03542a4..4616ab3 100644
> --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
> @@ -118,6 +118,7 @@ typedef struct {
>  #define INPUT_STATE_LEFTOPENBRACKET       0x04
>  #define INPUT_STATE_O                     0x08
>  #define INPUT_STATE_2                     0x10
> +#define INPUT_STATE_LEFTOPENBRACKET_2     0x20
>
>  #define RESET_STATE_DEFAULT               0x00
>  #define RESET_STATE_ESC_R                 0x01
> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
> index 227df85..12e7f9f 100644
> --- a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
> @@ -1182,6 +1182,8 @@ UnicodeToEfiKey (
>    UINT16              UnicodeChar;
>    EFI_INPUT_KEY       Key;
>    BOOLEAN             SetDefaultResetState;
> +  static UINT16       TtyEscapeStr[3];
> +  static INTN         TtyEscapeIndex;
>

Is this guaranteed to be safe?

>    TimerStatus = gBS->CheckEvent (TerminalDevice->TwoSecondTimeOut);
>
> @@ -1223,7 +1225,8 @@ UnicodeToEfiKey (
>          continue;
>        }
>
> -      if (UnicodeChar == 'O' && TerminalDevice->TerminalType == VT100TYPE) {
> +      if (UnicodeChar == 'O' && (TerminalDevice->TerminalType == VT100TYPE ||
> +                                 TerminalDevice->TerminalType == TTYTERMTYPE)) {
>          TerminalDevice->InputState |= INPUT_STATE_O;
>          TerminalDevice->ResetState = RESET_STATE_DEFAULT;
>          continue;
> @@ -1371,6 +1374,22 @@ UnicodeToEfiKey (
>          default :
>            break;
>          }
> +      } else if (TerminalDevice->TerminalType == TTYTERMTYPE) {
> +        /* Also accept VT100 escape codes for F1-F4 for TTY term */
> +        switch (UnicodeChar) {
> +        case 'P':
> +          Key.ScanCode = SCAN_F1;
> +          break;
> +        case 'Q':
> +          Key.ScanCode = SCAN_F2;
> +          break;
> +        case 'R':
> +          Key.ScanCode = SCAN_F3;
> +          break;
> +        case 'S':
> +          Key.ScanCode = SCAN_F4;
> +          break;
> +        }
>        }
>
>        if (Key.ScanCode != SCAN_NULL) {
> @@ -1514,6 +1533,21 @@ UnicodeToEfiKey (
>          }
>        }
>
> +      /*
> +       * The VT220 escape codes that the TTY terminal accepts all have
> +       * numeric codes, and there are no ambiguous prefixes shared with
> +       * other terminal types.
> +       */
> +      if (TerminalDevice->TerminalType == TTYTERMTYPE &&
> +          Key.ScanCode == SCAN_NULL &&
> +          UnicodeChar >= '0' &&
> +          UnicodeChar <= '9') {
> +        TtyEscapeStr[0] = UnicodeChar;
> +        TtyEscapeIndex = 1;
> +        TerminalDevice->InputState |= INPUT_STATE_LEFTOPENBRACKET_2;
> +        continue;
> +      }
> +
>        if (Key.ScanCode != SCAN_NULL) {
>          Key.UnicodeChar = 0;
>          EfiKeyFiFoInsertOneKey (TerminalDevice, &Key);
> @@ -1527,6 +1561,65 @@ UnicodeToEfiKey (
>        break;
>
>
> +    case INPUT_STATE_ESC | INPUT_STATE_LEFTOPENBRACKET | INPUT_STATE_LEFTOPENBRACKET_2:
> +      /*
> +       * Here we handle the VT220 escape codes that we accept.  This
> +       * state is only used by the TTY terminal type.
> +       */
> +      Key.ScanCode = SCAN_NULL;
> +      if (TerminalDevice->TerminalType == TTYTERMTYPE) {
> +
> +        if (UnicodeChar == '~' && TtyEscapeIndex <= 2) {
> +          UINTN EscCode;
> +          TtyEscapeStr[TtyEscapeIndex] = 0; /* Terminate string */
> +          EscCode = StrDecimalToUintn(TtyEscapeStr);
> +          switch (EscCode) {
> +          case 3:
> +              Key.ScanCode = SCAN_DELETE;
> +              break;
> +          case 11:
> +          case 12:
> +          case 13:
> +          case 14:
> +          case 15:
> +            Key.ScanCode = SCAN_F1 + EscCode - 11;
> +            break;
> +          case 17:
> +          case 18:
> +          case 19:
> +          case 20:
> +          case 21:
> +            Key.ScanCode = SCAN_F6 + EscCode - 17;
> +            break;
> +          case 23:
> +          case 24:
> +            Key.ScanCode = SCAN_F11 + EscCode - 23;
> +            break;
> +          default:
> +            break;
> +          }
> +        } else if (TtyEscapeIndex == 1){
> +          /* 2 character escape code   */
> +          TtyEscapeStr[TtyEscapeIndex++] = UnicodeChar;
> +          continue;
> +        }
> +        else {
> +          DEBUG ((EFI_D_ERROR, "Unexpected state in escape2\n"));
> +        }
> +      }
> +      TerminalDevice->ResetState = RESET_STATE_DEFAULT;
> +
> +      if (Key.ScanCode != SCAN_NULL) {
> +        Key.UnicodeChar = 0;
> +        EfiKeyFiFoInsertOneKey (TerminalDevice, &Key);
> +        TerminalDevice->InputState = INPUT_STATE_DEFAULT;
> +        UnicodeToEfiKeyFlushState (TerminalDevice);
> +        continue;
> +      }
> +
> +      UnicodeToEfiKeyFlushState (TerminalDevice);
> +      break;
> +
>      default:
>        //
>        // Invalid state. This should never happen.

Changing state machine code like this makes me nervous, but as far as
I can tell, non-TtyTerm users are not affected by it, so there is
little risk of regression.

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
Roy Franz July 7, 2015, 8:07 p.m. | #2
On Tue, Jul 7, 2015 at 6:18 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 7 July 2015 at 01:04, Roy Franz <roy.franz@linaro.org> wrote:
>> Accept the VT220 escape code [3~ as backspace for TtyTerm terminals.  This is
>> sent by many Linux terminals by default.  Also accept VT220 function keys
>> F1-F12, and VT100 F1-F4 keys as these are commonly sent by Linux terminals.
>> The VT220 escape codes are longer, and variable length so a new state is added
>> to the state machine along with a variable to construct the multibyte escape
>> sequence.
>> There are currently no ambiguous escape sequence prefixes accepted, so the TTY
>> terminal accepts escape sequences for a variety of terminals.  The goal is to
>> 'just work' with as many terminals as possible, rather than properly emulating
>> any specific terminal.  Backspace, Del, and F10 have been tested on xterm,
>> rxvt, tmux, and screen.
>> Note: The existing vt100 function key handling does not match the vt100
>> documentation that I found, so I added the TTY terminal handling
>> of VT100 F1-F4 (really PF1-PF4 on vt100) separately.  The vt100
>> has no F5-F10 keys, so I don't know what the current vt100 code
>> is based on.
>>
>> Signed-off-by: Roy Franz <roy.franz@linaro.org>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>
> Same here ^^^
> (and in patch 1/4)

I'll fix this here and in the other patches.  Maybe I should re-read
my contributions.txt patch so I get this right :)
>
>> ---
>>  MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h      |  1 +
>>  MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c | 95 +++++++++++++++++++-
>>  2 files changed, 95 insertions(+), 1 deletion(-)
>>
>> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
>> index 03542a4..4616ab3 100644
>> --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
>> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
>> @@ -118,6 +118,7 @@ typedef struct {
>>  #define INPUT_STATE_LEFTOPENBRACKET       0x04
>>  #define INPUT_STATE_O                     0x08
>>  #define INPUT_STATE_2                     0x10
>> +#define INPUT_STATE_LEFTOPENBRACKET_2     0x20
>>
>>  #define RESET_STATE_DEFAULT               0x00
>>  #define RESET_STATE_ESC_R                 0x01
>> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
>> index 227df85..12e7f9f 100644
>> --- a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
>> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
>> @@ -1182,6 +1182,8 @@ UnicodeToEfiKey (
>>    UINT16              UnicodeChar;
>>    EFI_INPUT_KEY       Key;
>>    BOOLEAN             SetDefaultResetState;
>> +  static UINT16       TtyEscapeStr[3];
>> +  static INTN         TtyEscapeIndex;
>>
>
> Is this guaranteed to be safe?
I'm assuming you mean the length of 3? (for 2 characters + NUL termination)
I think it is.

I do the following when the INPUT_STATE_LEFTOPENBRACKET_2 is entered:
+        TtyEscapeStr[0] = UnicodeChar;
+        TtyEscapeIndex = 1;

When any additional escape sequence characters are processed any values
of TtyEscapeIndex other than 1 or 2 are errors.  If we got an escape sequence
that was too long, we will go back to the RESET_STATE_DEFAULT state,
and TtyEscape* will be re-initialized when the INPUT_STATE_LEFTOPENBRACKET_2
state is entered.

There is only 1 place where  INPUT_STATE_LEFTOPENBRACKET_2 is entered,
and both TtyEscape* variables are initialized there, and only valid
(ie Index of 1 or 2)
index values are processed once in INPUT_STATE_LEFTOPENBRACKET_2.

I can add some comments to this effect if you think it is worthwhile.


>
>>    TimerStatus = gBS->CheckEvent (TerminalDevice->TwoSecondTimeOut);
>>
>> @@ -1223,7 +1225,8 @@ UnicodeToEfiKey (
>>          continue;
>>        }
>>
>> -      if (UnicodeChar == 'O' && TerminalDevice->TerminalType == VT100TYPE) {
>> +      if (UnicodeChar == 'O' && (TerminalDevice->TerminalType == VT100TYPE ||
>> +                                 TerminalDevice->TerminalType == TTYTERMTYPE)) {
>>          TerminalDevice->InputState |= INPUT_STATE_O;
>>          TerminalDevice->ResetState = RESET_STATE_DEFAULT;
>>          continue;
>> @@ -1371,6 +1374,22 @@ UnicodeToEfiKey (
>>          default :
>>            break;
>>          }
>> +      } else if (TerminalDevice->TerminalType == TTYTERMTYPE) {
>> +        /* Also accept VT100 escape codes for F1-F4 for TTY term */
>> +        switch (UnicodeChar) {
>> +        case 'P':
>> +          Key.ScanCode = SCAN_F1;
>> +          break;
>> +        case 'Q':
>> +          Key.ScanCode = SCAN_F2;
>> +          break;
>> +        case 'R':
>> +          Key.ScanCode = SCAN_F3;
>> +          break;
>> +        case 'S':
>> +          Key.ScanCode = SCAN_F4;
>> +          break;
>> +        }
>>        }
>>
>>        if (Key.ScanCode != SCAN_NULL) {
>> @@ -1514,6 +1533,21 @@ UnicodeToEfiKey (
>>          }
>>        }
>>
>> +      /*
>> +       * The VT220 escape codes that the TTY terminal accepts all have
>> +       * numeric codes, and there are no ambiguous prefixes shared with
>> +       * other terminal types.
>> +       */
>> +      if (TerminalDevice->TerminalType == TTYTERMTYPE &&
>> +          Key.ScanCode == SCAN_NULL &&
>> +          UnicodeChar >= '0' &&
>> +          UnicodeChar <= '9') {
>> +        TtyEscapeStr[0] = UnicodeChar;
>> +        TtyEscapeIndex = 1;
>> +        TerminalDevice->InputState |= INPUT_STATE_LEFTOPENBRACKET_2;
>> +        continue;
>> +      }
>> +
>>        if (Key.ScanCode != SCAN_NULL) {
>>          Key.UnicodeChar = 0;
>>          EfiKeyFiFoInsertOneKey (TerminalDevice, &Key);
>> @@ -1527,6 +1561,65 @@ UnicodeToEfiKey (
>>        break;
>>
>>
>> +    case INPUT_STATE_ESC | INPUT_STATE_LEFTOPENBRACKET | INPUT_STATE_LEFTOPENBRACKET_2:
>> +      /*
>> +       * Here we handle the VT220 escape codes that we accept.  This
>> +       * state is only used by the TTY terminal type.
>> +       */
>> +      Key.ScanCode = SCAN_NULL;
>> +      if (TerminalDevice->TerminalType == TTYTERMTYPE) {
>> +
>> +        if (UnicodeChar == '~' && TtyEscapeIndex <= 2) {
>> +          UINTN EscCode;
>> +          TtyEscapeStr[TtyEscapeIndex] = 0; /* Terminate string */
>> +          EscCode = StrDecimalToUintn(TtyEscapeStr);
>> +          switch (EscCode) {
>> +          case 3:
>> +              Key.ScanCode = SCAN_DELETE;
>> +              break;
>> +          case 11:
>> +          case 12:
>> +          case 13:
>> +          case 14:
>> +          case 15:
>> +            Key.ScanCode = SCAN_F1 + EscCode - 11;
>> +            break;
>> +          case 17:
>> +          case 18:
>> +          case 19:
>> +          case 20:
>> +          case 21:
>> +            Key.ScanCode = SCAN_F6 + EscCode - 17;
>> +            break;
>> +          case 23:
>> +          case 24:
>> +            Key.ScanCode = SCAN_F11 + EscCode - 23;
>> +            break;
>> +          default:
>> +            break;
>> +          }
>> +        } else if (TtyEscapeIndex == 1){
>> +          /* 2 character escape code   */
>> +          TtyEscapeStr[TtyEscapeIndex++] = UnicodeChar;
>> +          continue;
>> +        }
>> +        else {
>> +          DEBUG ((EFI_D_ERROR, "Unexpected state in escape2\n"));
>> +        }
>> +      }
>> +      TerminalDevice->ResetState = RESET_STATE_DEFAULT;
>> +
>> +      if (Key.ScanCode != SCAN_NULL) {
>> +        Key.UnicodeChar = 0;
>> +        EfiKeyFiFoInsertOneKey (TerminalDevice, &Key);
>> +        TerminalDevice->InputState = INPUT_STATE_DEFAULT;
>> +        UnicodeToEfiKeyFlushState (TerminalDevice);
>> +        continue;
>> +      }
>> +
>> +      UnicodeToEfiKeyFlushState (TerminalDevice);
>> +      break;
>> +
>>      default:
>>        //
>>        // Invalid state. This should never happen.
>
> Changing state machine code like this makes me nervous, but as far as
> I can tell, non-TtyTerm users are not affected by it, so there is
> little risk of regression.
>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
Ard Biesheuvel July 7, 2015, 8:25 p.m. | #3
On 7 July 2015 at 22:07, Roy Franz <roy.franz@linaro.org> wrote:
> On Tue, Jul 7, 2015 at 6:18 AM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> On 7 July 2015 at 01:04, Roy Franz <roy.franz@linaro.org> wrote:
>>> Accept the VT220 escape code [3~ as backspace for TtyTerm terminals.  This is
>>> sent by many Linux terminals by default.  Also accept VT220 function keys
>>> F1-F12, and VT100 F1-F4 keys as these are commonly sent by Linux terminals.
>>> The VT220 escape codes are longer, and variable length so a new state is added
>>> to the state machine along with a variable to construct the multibyte escape
>>> sequence.
>>> There are currently no ambiguous escape sequence prefixes accepted, so the TTY
>>> terminal accepts escape sequences for a variety of terminals.  The goal is to
>>> 'just work' with as many terminals as possible, rather than properly emulating
>>> any specific terminal.  Backspace, Del, and F10 have been tested on xterm,
>>> rxvt, tmux, and screen.
>>> Note: The existing vt100 function key handling does not match the vt100
>>> documentation that I found, so I added the TTY terminal handling
>>> of VT100 F1-F4 (really PF1-PF4 on vt100) separately.  The vt100
>>> has no F5-F10 keys, so I don't know what the current vt100 code
>>> is based on.
>>>
>>> Signed-off-by: Roy Franz <roy.franz@linaro.org>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>
>> Same here ^^^
>> (and in patch 1/4)
>
> I'll fix this here and in the other patches.  Maybe I should re-read
> my contributions.txt patch so I get this right :)

:-)

>>
>>> ---
>>>  MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h      |  1 +
>>>  MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c | 95 +++++++++++++++++++-
>>>  2 files changed, 95 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
>>> index 03542a4..4616ab3 100644
>>> --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
>>> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
>>> @@ -118,6 +118,7 @@ typedef struct {
>>>  #define INPUT_STATE_LEFTOPENBRACKET       0x04
>>>  #define INPUT_STATE_O                     0x08
>>>  #define INPUT_STATE_2                     0x10
>>> +#define INPUT_STATE_LEFTOPENBRACKET_2     0x20
>>>
>>>  #define RESET_STATE_DEFAULT               0x00
>>>  #define RESET_STATE_ESC_R                 0x01
>>> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
>>> index 227df85..12e7f9f 100644
>>> --- a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
>>> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
>>> @@ -1182,6 +1182,8 @@ UnicodeToEfiKey (
>>>    UINT16              UnicodeChar;
>>>    EFI_INPUT_KEY       Key;
>>>    BOOLEAN             SetDefaultResetState;
>>> +  static UINT16       TtyEscapeStr[3];
>>> +  static INTN         TtyEscapeIndex;
>>>
>>
>> Is this guaranteed to be safe?
> I'm assuming you mean the length of 3? (for 2 characters + NUL termination)
> I think it is.
>

No, I mean the static. I know UEFI is single threaded, but it has an
event model that could potentially result in functions being called in
a reentrant fashion. I think this can only happen in functions that
signal events themselves, but I am not intimate enough with this stuff
to claim that this is 100%  safe.

> I do the following when the INPUT_STATE_LEFTOPENBRACKET_2 is entered:
> +        TtyEscapeStr[0] = UnicodeChar;
> +        TtyEscapeIndex = 1;
>
> When any additional escape sequence characters are processed any values
> of TtyEscapeIndex other than 1 or 2 are errors.  If we got an escape sequence
> that was too long, we will go back to the RESET_STATE_DEFAULT state,
> and TtyEscape* will be re-initialized when the INPUT_STATE_LEFTOPENBRACKET_2
> state is entered.
>
> There is only 1 place where  INPUT_STATE_LEFTOPENBRACKET_2 is entered,
> and both TtyEscape* variables are initialized there, and only valid
> (ie Index of 1 or 2)
> index values are processed once in INPUT_STATE_LEFTOPENBRACKET_2.
>
> I can add some comments to this effect if you think it is worthwhile.
>
>
>>
>>>    TimerStatus = gBS->CheckEvent (TerminalDevice->TwoSecondTimeOut);
>>>
>>> @@ -1223,7 +1225,8 @@ UnicodeToEfiKey (
>>>          continue;
>>>        }
>>>
>>> -      if (UnicodeChar == 'O' && TerminalDevice->TerminalType == VT100TYPE) {
>>> +      if (UnicodeChar == 'O' && (TerminalDevice->TerminalType == VT100TYPE ||
>>> +                                 TerminalDevice->TerminalType == TTYTERMTYPE)) {
>>>          TerminalDevice->InputState |= INPUT_STATE_O;
>>>          TerminalDevice->ResetState = RESET_STATE_DEFAULT;
>>>          continue;
>>> @@ -1371,6 +1374,22 @@ UnicodeToEfiKey (
>>>          default :
>>>            break;
>>>          }
>>> +      } else if (TerminalDevice->TerminalType == TTYTERMTYPE) {
>>> +        /* Also accept VT100 escape codes for F1-F4 for TTY term */
>>> +        switch (UnicodeChar) {
>>> +        case 'P':
>>> +          Key.ScanCode = SCAN_F1;
>>> +          break;
>>> +        case 'Q':
>>> +          Key.ScanCode = SCAN_F2;
>>> +          break;
>>> +        case 'R':
>>> +          Key.ScanCode = SCAN_F3;
>>> +          break;
>>> +        case 'S':
>>> +          Key.ScanCode = SCAN_F4;
>>> +          break;
>>> +        }
>>>        }
>>>
>>>        if (Key.ScanCode != SCAN_NULL) {
>>> @@ -1514,6 +1533,21 @@ UnicodeToEfiKey (
>>>          }
>>>        }
>>>
>>> +      /*
>>> +       * The VT220 escape codes that the TTY terminal accepts all have
>>> +       * numeric codes, and there are no ambiguous prefixes shared with
>>> +       * other terminal types.
>>> +       */
>>> +      if (TerminalDevice->TerminalType == TTYTERMTYPE &&
>>> +          Key.ScanCode == SCAN_NULL &&
>>> +          UnicodeChar >= '0' &&
>>> +          UnicodeChar <= '9') {
>>> +        TtyEscapeStr[0] = UnicodeChar;
>>> +        TtyEscapeIndex = 1;
>>> +        TerminalDevice->InputState |= INPUT_STATE_LEFTOPENBRACKET_2;
>>> +        continue;
>>> +      }
>>> +
>>>        if (Key.ScanCode != SCAN_NULL) {
>>>          Key.UnicodeChar = 0;
>>>          EfiKeyFiFoInsertOneKey (TerminalDevice, &Key);
>>> @@ -1527,6 +1561,65 @@ UnicodeToEfiKey (
>>>        break;
>>>
>>>
>>> +    case INPUT_STATE_ESC | INPUT_STATE_LEFTOPENBRACKET | INPUT_STATE_LEFTOPENBRACKET_2:
>>> +      /*
>>> +       * Here we handle the VT220 escape codes that we accept.  This
>>> +       * state is only used by the TTY terminal type.
>>> +       */
>>> +      Key.ScanCode = SCAN_NULL;
>>> +      if (TerminalDevice->TerminalType == TTYTERMTYPE) {
>>> +
>>> +        if (UnicodeChar == '~' && TtyEscapeIndex <= 2) {
>>> +          UINTN EscCode;
>>> +          TtyEscapeStr[TtyEscapeIndex] = 0; /* Terminate string */
>>> +          EscCode = StrDecimalToUintn(TtyEscapeStr);
>>> +          switch (EscCode) {
>>> +          case 3:
>>> +              Key.ScanCode = SCAN_DELETE;
>>> +              break;
>>> +          case 11:
>>> +          case 12:
>>> +          case 13:
>>> +          case 14:
>>> +          case 15:
>>> +            Key.ScanCode = SCAN_F1 + EscCode - 11;
>>> +            break;
>>> +          case 17:
>>> +          case 18:
>>> +          case 19:
>>> +          case 20:
>>> +          case 21:
>>> +            Key.ScanCode = SCAN_F6 + EscCode - 17;
>>> +            break;
>>> +          case 23:
>>> +          case 24:
>>> +            Key.ScanCode = SCAN_F11 + EscCode - 23;
>>> +            break;
>>> +          default:
>>> +            break;
>>> +          }
>>> +        } else if (TtyEscapeIndex == 1){
>>> +          /* 2 character escape code   */
>>> +          TtyEscapeStr[TtyEscapeIndex++] = UnicodeChar;
>>> +          continue;
>>> +        }
>>> +        else {
>>> +          DEBUG ((EFI_D_ERROR, "Unexpected state in escape2\n"));
>>> +        }
>>> +      }
>>> +      TerminalDevice->ResetState = RESET_STATE_DEFAULT;
>>> +
>>> +      if (Key.ScanCode != SCAN_NULL) {
>>> +        Key.UnicodeChar = 0;
>>> +        EfiKeyFiFoInsertOneKey (TerminalDevice, &Key);
>>> +        TerminalDevice->InputState = INPUT_STATE_DEFAULT;
>>> +        UnicodeToEfiKeyFlushState (TerminalDevice);
>>> +        continue;
>>> +      }
>>> +
>>> +      UnicodeToEfiKeyFlushState (TerminalDevice);
>>> +      break;
>>> +
>>>      default:
>>>        //
>>>        // Invalid state. This should never happen.
>>
>> Changing state machine code like this makes me nervous, but as far as
>> I can tell, non-TtyTerm users are not affected by it, so there is
>> little risk of regression.
>>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
Roy Franz July 7, 2015, 8:31 p.m. | #4
On Tue, Jul 7, 2015 at 1:25 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 7 July 2015 at 22:07, Roy Franz <roy.franz@linaro.org> wrote:
>> On Tue, Jul 7, 2015 at 6:18 AM, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org> wrote:
>>> On 7 July 2015 at 01:04, Roy Franz <roy.franz@linaro.org> wrote:
>>>> Accept the VT220 escape code [3~ as backspace for TtyTerm terminals.  This is
>>>> sent by many Linux terminals by default.  Also accept VT220 function keys
>>>> F1-F12, and VT100 F1-F4 keys as these are commonly sent by Linux terminals.
>>>> The VT220 escape codes are longer, and variable length so a new state is added
>>>> to the state machine along with a variable to construct the multibyte escape
>>>> sequence.
>>>> There are currently no ambiguous escape sequence prefixes accepted, so the TTY
>>>> terminal accepts escape sequences for a variety of terminals.  The goal is to
>>>> 'just work' with as many terminals as possible, rather than properly emulating
>>>> any specific terminal.  Backspace, Del, and F10 have been tested on xterm,
>>>> rxvt, tmux, and screen.
>>>> Note: The existing vt100 function key handling does not match the vt100
>>>> documentation that I found, so I added the TTY terminal handling
>>>> of VT100 F1-F4 (really PF1-PF4 on vt100) separately.  The vt100
>>>> has no F5-F10 keys, so I don't know what the current vt100 code
>>>> is based on.
>>>>
>>>> Signed-off-by: Roy Franz <roy.franz@linaro.org>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>
>>> Same here ^^^
>>> (and in patch 1/4)
>>
>> I'll fix this here and in the other patches.  Maybe I should re-read
>> my contributions.txt patch so I get this right :)
>
> :-)
>
>>>
>>>> ---
>>>>  MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h      |  1 +
>>>>  MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c | 95 +++++++++++++++++++-
>>>>  2 files changed, 95 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
>>>> index 03542a4..4616ab3 100644
>>>> --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
>>>> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
>>>> @@ -118,6 +118,7 @@ typedef struct {
>>>>  #define INPUT_STATE_LEFTOPENBRACKET       0x04
>>>>  #define INPUT_STATE_O                     0x08
>>>>  #define INPUT_STATE_2                     0x10
>>>> +#define INPUT_STATE_LEFTOPENBRACKET_2     0x20
>>>>
>>>>  #define RESET_STATE_DEFAULT               0x00
>>>>  #define RESET_STATE_ESC_R                 0x01
>>>> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
>>>> index 227df85..12e7f9f 100644
>>>> --- a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
>>>> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
>>>> @@ -1182,6 +1182,8 @@ UnicodeToEfiKey (
>>>>    UINT16              UnicodeChar;
>>>>    EFI_INPUT_KEY       Key;
>>>>    BOOLEAN             SetDefaultResetState;
>>>> +  static UINT16       TtyEscapeStr[3];
>>>> +  static INTN         TtyEscapeIndex;
>>>>
>>>
>>> Is this guaranteed to be safe?
>> I'm assuming you mean the length of 3? (for 2 characters + NUL termination)
>> I think it is.
>>
>
> No, I mean the static. I know UEFI is single threaded, but it has an
> event model that could potentially result in functions being called in
> a reentrant fashion. I think this can only happen in functions that
> signal events themselves, but I am not intimate enough with this stuff
> to claim that this is 100%  safe.
>
Hmm, that is a good point.    I think these should really go into the
TerminalDevice
structure, since these really are per-terminal state.  I'll move them
there, as that is
where the rest of the terminal state is tracked.

>> I do the following when the INPUT_STATE_LEFTOPENBRACKET_2 is entered:
>> +        TtyEscapeStr[0] = UnicodeChar;
>> +        TtyEscapeIndex = 1;
>>
>> When any additional escape sequence characters are processed any values
>> of TtyEscapeIndex other than 1 or 2 are errors.  If we got an escape sequence
>> that was too long, we will go back to the RESET_STATE_DEFAULT state,
>> and TtyEscape* will be re-initialized when the INPUT_STATE_LEFTOPENBRACKET_2
>> state is entered.
>>
>> There is only 1 place where  INPUT_STATE_LEFTOPENBRACKET_2 is entered,
>> and both TtyEscape* variables are initialized there, and only valid
>> (ie Index of 1 or 2)
>> index values are processed once in INPUT_STATE_LEFTOPENBRACKET_2.
>>
>> I can add some comments to this effect if you think it is worthwhile.
>>
>>
>>>
>>>>    TimerStatus = gBS->CheckEvent (TerminalDevice->TwoSecondTimeOut);
>>>>
>>>> @@ -1223,7 +1225,8 @@ UnicodeToEfiKey (
>>>>          continue;
>>>>        }
>>>>
>>>> -      if (UnicodeChar == 'O' && TerminalDevice->TerminalType == VT100TYPE) {
>>>> +      if (UnicodeChar == 'O' && (TerminalDevice->TerminalType == VT100TYPE ||
>>>> +                                 TerminalDevice->TerminalType == TTYTERMTYPE)) {
>>>>          TerminalDevice->InputState |= INPUT_STATE_O;
>>>>          TerminalDevice->ResetState = RESET_STATE_DEFAULT;
>>>>          continue;
>>>> @@ -1371,6 +1374,22 @@ UnicodeToEfiKey (
>>>>          default :
>>>>            break;
>>>>          }
>>>> +      } else if (TerminalDevice->TerminalType == TTYTERMTYPE) {
>>>> +        /* Also accept VT100 escape codes for F1-F4 for TTY term */
>>>> +        switch (UnicodeChar) {
>>>> +        case 'P':
>>>> +          Key.ScanCode = SCAN_F1;
>>>> +          break;
>>>> +        case 'Q':
>>>> +          Key.ScanCode = SCAN_F2;
>>>> +          break;
>>>> +        case 'R':
>>>> +          Key.ScanCode = SCAN_F3;
>>>> +          break;
>>>> +        case 'S':
>>>> +          Key.ScanCode = SCAN_F4;
>>>> +          break;
>>>> +        }
>>>>        }
>>>>
>>>>        if (Key.ScanCode != SCAN_NULL) {
>>>> @@ -1514,6 +1533,21 @@ UnicodeToEfiKey (
>>>>          }
>>>>        }
>>>>
>>>> +      /*
>>>> +       * The VT220 escape codes that the TTY terminal accepts all have
>>>> +       * numeric codes, and there are no ambiguous prefixes shared with
>>>> +       * other terminal types.
>>>> +       */
>>>> +      if (TerminalDevice->TerminalType == TTYTERMTYPE &&
>>>> +          Key.ScanCode == SCAN_NULL &&
>>>> +          UnicodeChar >= '0' &&
>>>> +          UnicodeChar <= '9') {
>>>> +        TtyEscapeStr[0] = UnicodeChar;
>>>> +        TtyEscapeIndex = 1;
>>>> +        TerminalDevice->InputState |= INPUT_STATE_LEFTOPENBRACKET_2;
>>>> +        continue;
>>>> +      }
>>>> +
>>>>        if (Key.ScanCode != SCAN_NULL) {
>>>>          Key.UnicodeChar = 0;
>>>>          EfiKeyFiFoInsertOneKey (TerminalDevice, &Key);
>>>> @@ -1527,6 +1561,65 @@ UnicodeToEfiKey (
>>>>        break;
>>>>
>>>>
>>>> +    case INPUT_STATE_ESC | INPUT_STATE_LEFTOPENBRACKET | INPUT_STATE_LEFTOPENBRACKET_2:
>>>> +      /*
>>>> +       * Here we handle the VT220 escape codes that we accept.  This
>>>> +       * state is only used by the TTY terminal type.
>>>> +       */
>>>> +      Key.ScanCode = SCAN_NULL;
>>>> +      if (TerminalDevice->TerminalType == TTYTERMTYPE) {
>>>> +
>>>> +        if (UnicodeChar == '~' && TtyEscapeIndex <= 2) {
>>>> +          UINTN EscCode;
>>>> +          TtyEscapeStr[TtyEscapeIndex] = 0; /* Terminate string */
>>>> +          EscCode = StrDecimalToUintn(TtyEscapeStr);
>>>> +          switch (EscCode) {
>>>> +          case 3:
>>>> +              Key.ScanCode = SCAN_DELETE;
>>>> +              break;
>>>> +          case 11:
>>>> +          case 12:
>>>> +          case 13:
>>>> +          case 14:
>>>> +          case 15:
>>>> +            Key.ScanCode = SCAN_F1 + EscCode - 11;
>>>> +            break;
>>>> +          case 17:
>>>> +          case 18:
>>>> +          case 19:
>>>> +          case 20:
>>>> +          case 21:
>>>> +            Key.ScanCode = SCAN_F6 + EscCode - 17;
>>>> +            break;
>>>> +          case 23:
>>>> +          case 24:
>>>> +            Key.ScanCode = SCAN_F11 + EscCode - 23;
>>>> +            break;
>>>> +          default:
>>>> +            break;
>>>> +          }
>>>> +        } else if (TtyEscapeIndex == 1){
>>>> +          /* 2 character escape code   */
>>>> +          TtyEscapeStr[TtyEscapeIndex++] = UnicodeChar;
>>>> +          continue;
>>>> +        }
>>>> +        else {
>>>> +          DEBUG ((EFI_D_ERROR, "Unexpected state in escape2\n"));
>>>> +        }
>>>> +      }
>>>> +      TerminalDevice->ResetState = RESET_STATE_DEFAULT;
>>>> +
>>>> +      if (Key.ScanCode != SCAN_NULL) {
>>>> +        Key.UnicodeChar = 0;
>>>> +        EfiKeyFiFoInsertOneKey (TerminalDevice, &Key);
>>>> +        TerminalDevice->InputState = INPUT_STATE_DEFAULT;
>>>> +        UnicodeToEfiKeyFlushState (TerminalDevice);
>>>> +        continue;
>>>> +      }
>>>> +
>>>> +      UnicodeToEfiKeyFlushState (TerminalDevice);
>>>> +      break;
>>>> +
>>>>      default:
>>>>        //
>>>>        // Invalid state. This should never happen.
>>>
>>> Changing state machine code like this makes me nervous, but as far as
>>> I can tell, non-TtyTerm users are not affected by it, so there is
>>> little risk of regression.
>>>
>>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
Roy Franz July 7, 2015, 9:12 p.m. | #5
On Tue, Jul 7, 2015 at 1:41 PM, Andrew Fish <afish@apple.com> wrote:
>
> On Jul 7, 2015, at 1:31 PM, Roy Franz <roy.franz@linaro.org> wrote:
>
> On Tue, Jul 7, 2015 at 1:25 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>
> On 7 July 2015 at 22:07, Roy Franz <roy.franz@linaro.org> wrote:
>
> On Tue, Jul 7, 2015 at 6:18 AM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>
> On 7 July 2015 at 01:04, Roy Franz <roy.franz@linaro.org> wrote:
>
> Accept the VT220 escape code [3~ as backspace for TtyTerm terminals.  This
> is
> sent by many Linux terminals by default.  Also accept VT220 function keys
> F1-F12, and VT100 F1-F4 keys as these are commonly sent by Linux terminals.
> The VT220 escape codes are longer, and variable length so a new state is
> added
> to the state machine along with a variable to construct the multibyte escape
> sequence.
> There are currently no ambiguous escape sequence prefixes accepted, so the
> TTY
> terminal accepts escape sequences for a variety of terminals.  The goal is
> to
> 'just work' with as many terminals as possible, rather than properly
> emulating
> any specific terminal.  Backspace, Del, and F10 have been tested on xterm,
> rxvt, tmux, and screen.
> Note: The existing vt100 function key handling does not match the vt100
> documentation that I found, so I added the TTY terminal handling
> of VT100 F1-F4 (really PF1-PF4 on vt100) separately.  The vt100
> has no F5-F10 keys, so I don't know what the current vt100 code
> is based on.
>
> Signed-off-by: Roy Franz <roy.franz@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.0
>
>
> Same here ^^^
> (and in patch 1/4)
>
>
> I'll fix this here and in the other patches.  Maybe I should re-read
> my contributions.txt patch so I get this right :)
>
>
> :-)
>
>
> ---
> MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h      |  1 +
> MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c | 95
> +++++++++++++++++++-
> 2 files changed, 95 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
> b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
> index 03542a4..4616ab3 100644
> --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
> @@ -118,6 +118,7 @@ typedef struct {
> #define INPUT_STATE_LEFTOPENBRACKET       0x04
> #define INPUT_STATE_O                     0x08
> #define INPUT_STATE_2                     0x10
> +#define INPUT_STATE_LEFTOPENBRACKET_2     0x20
>
> #define RESET_STATE_DEFAULT               0x00
> #define RESET_STATE_ESC_R                 0x01
> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
> b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
> index 227df85..12e7f9f 100644
> --- a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
> @@ -1182,6 +1182,8 @@ UnicodeToEfiKey (
>   UINT16              UnicodeChar;
>   EFI_INPUT_KEY       Key;
>   BOOLEAN             SetDefaultResetState;
> +  static UINT16       TtyEscapeStr[3];
> +  static INTN         TtyEscapeIndex;
>
>
> Is this guaranteed to be safe?
>
> I'm assuming you mean the length of 3? (for 2 characters + NUL termination)
> I think it is.
>
>
> No, I mean the static. I know UEFI is single threaded, but it has an
> event model that could potentially result in functions being called in
> a reentrant fashion. I think this can only happen in functions that
> signal events themselves, but I am not intimate enough with this stuff
> to claim that this is 100%  safe.
>
> Hmm, that is a good point.    I think these should really go into the
> TerminalDevice
>
>
> In general globals (or static variables in a function) are bad as there can
> be multiple instances of a protocol. The terminal driver could be connected
> on an arbitrary number of serial ports.
>
> structure, since these really are per-terminal state.  I'll move them
> there, as that is
> where the rest of the terminal state is tracked.
>
>
> This is the data structure that tracks the Protocol’s instance data. The
> generic form in EFI is to use the Containment Record (CR()) macro to convert
> the protocol instance back into the private data for the driver.
>
> https://svn.code.sf.net/p/edk2/code/trunk/edk2/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
>
> typedef struct {
>   UINTN                               Signature;
>   EFI_HANDLE                          Handle;
>   UINT8                               TerminalType;
>   EFI_SERIAL_IO_PROTOCOL              *SerialIo;
>   EFI_DEVICE_PATH_PROTOCOL            *DevicePath;
>   EFI_SIMPLE_TEXT_INPUT_PROTOCOL      SimpleInput;
>   EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL     SimpleTextOutput;
>   EFI_SIMPLE_TEXT_OUTPUT_MODE         SimpleTextOutputMode;
>   TERMINAL_CONSOLE_MODE_DATA          *TerminalConsoleModeData;
>   UINTN                               SerialInTimeOut;
>   RAW_DATA_FIFO                       *RawFiFo;
>   UNICODE_FIFO                        *UnicodeFiFo;
>   EFI_KEY_FIFO                        *EfiKeyFiFo;
>   EFI_UNICODE_STRING_TABLE            *ControllerNameTable;
>   EFI_EVENT                           TimerEvent;
>   EFI_EVENT                           TwoSecondTimeOut;
>   UINT32                              InputState;
>   UINT32                              ResetState;
>
>   //
>   // Esc could not be output to the screen by user,
>   // but the terminal driver need to output it to
>   // the terminal emulation software to send control sequence.
>   // This boolean is used by the terminal driver only
>   // to indicate whether the Esc could be sent or not.
>   //
>   BOOLEAN                             OutputEscChar;
>   EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL   SimpleInputEx;
>   LIST_ENTRY                          NotifyList;
> } TERMINAL_DEV;
> …

Agreed, the static was clearly wrong.

>
> #define TERMINAL_CON_IN_DEV_FROM_THIS(a)  CR (a, TERMINAL_DEV, SimpleInput,
> TERMINAL_DEV_SIGNATURE)
> #define TERMINAL_CON_OUT_DEV_FROM_THIS(a) CR (a, TERMINAL_DEV,
> SimpleTextOutput, TERMINAL_DEV_SIGNATURE)
> #define TERMINAL_CON_IN_EX_DEV_FROM_THIS(a)  CR (a, TERMINAL_DEV,
> SimpleInputEx, TERMINAL_DEV_SIGNATURE)

In UnicodeToEfiKey() we are passed a TerminalDevice structure already, so the
TERMINAL_CON_IN_DEV_FROM_THIS() has already been done for us.

I'll post an updated series that fixes this (and a few other more
minor changes) shortly.

Roy

>
> Thanks,
>
> Andrew Fish
>
> I do the following when the INPUT_STATE_LEFTOPENBRACKET_2 is entered:
> +        TtyEscapeStr[0] = UnicodeChar;
> +        TtyEscapeIndex = 1;
>
> When any additional escape sequence characters are processed any values
> of TtyEscapeIndex other than 1 or 2 are errors.  If we got an escape
> sequence
> that was too long, we will go back to the RESET_STATE_DEFAULT state,
> and TtyEscape* will be re-initialized when the INPUT_STATE_LEFTOPENBRACKET_2
> state is entered.
>
> There is only 1 place where  INPUT_STATE_LEFTOPENBRACKET_2 is entered,
> and both TtyEscape* variables are initialized there, and only valid
> (ie Index of 1 or 2)
> index values are processed once in INPUT_STATE_LEFTOPENBRACKET_2.
>
> I can add some comments to this effect if you think it is worthwhile.
>
>
>
>   TimerStatus = gBS->CheckEvent (TerminalDevice->TwoSecondTimeOut);
>
> @@ -1223,7 +1225,8 @@ UnicodeToEfiKey (
>         continue;
>       }
>
> -      if (UnicodeChar == 'O' && TerminalDevice->TerminalType == VT100TYPE)
> {
> +      if (UnicodeChar == 'O' && (TerminalDevice->TerminalType == VT100TYPE
> ||
> +                                 TerminalDevice->TerminalType ==
> TTYTERMTYPE)) {
>         TerminalDevice->InputState |= INPUT_STATE_O;
>         TerminalDevice->ResetState = RESET_STATE_DEFAULT;
>         continue;
> @@ -1371,6 +1374,22 @@ UnicodeToEfiKey (
>         default :
>           break;
>         }
> +      } else if (TerminalDevice->TerminalType == TTYTERMTYPE) {
> +        /* Also accept VT100 escape codes for F1-F4 for TTY term */
> +        switch (UnicodeChar) {
> +        case 'P':
> +          Key.ScanCode = SCAN_F1;
> +          break;
> +        case 'Q':
> +          Key.ScanCode = SCAN_F2;
> +          break;
> +        case 'R':
> +          Key.ScanCode = SCAN_F3;
> +          break;
> +        case 'S':
> +          Key.ScanCode = SCAN_F4;
> +          break;
> +        }
>       }
>
>       if (Key.ScanCode != SCAN_NULL) {
> @@ -1514,6 +1533,21 @@ UnicodeToEfiKey (
>         }
>       }
>
> +      /*
> +       * The VT220 escape codes that the TTY terminal accepts all have
> +       * numeric codes, and there are no ambiguous prefixes shared with
> +       * other terminal types.
> +       */
> +      if (TerminalDevice->TerminalType == TTYTERMTYPE &&
> +          Key.ScanCode == SCAN_NULL &&
> +          UnicodeChar >= '0' &&
> +          UnicodeChar <= '9') {
> +        TtyEscapeStr[0] = UnicodeChar;
> +        TtyEscapeIndex = 1;
> +        TerminalDevice->InputState |= INPUT_STATE_LEFTOPENBRACKET_2;
> +        continue;
> +      }
> +
>       if (Key.ScanCode != SCAN_NULL) {
>         Key.UnicodeChar = 0;
>         EfiKeyFiFoInsertOneKey (TerminalDevice, &Key);
> @@ -1527,6 +1561,65 @@ UnicodeToEfiKey (
>       break;
>
>
> +    case INPUT_STATE_ESC | INPUT_STATE_LEFTOPENBRACKET |
> INPUT_STATE_LEFTOPENBRACKET_2:
> +      /*
> +       * Here we handle the VT220 escape codes that we accept.  This
> +       * state is only used by the TTY terminal type.
> +       */
> +      Key.ScanCode = SCAN_NULL;
> +      if (TerminalDevice->TerminalType == TTYTERMTYPE) {
> +
> +        if (UnicodeChar == '~' && TtyEscapeIndex <= 2) {
> +          UINTN EscCode;
> +          TtyEscapeStr[TtyEscapeIndex] = 0; /* Terminate string */
> +          EscCode = StrDecimalToUintn(TtyEscapeStr);
> +          switch (EscCode) {
> +          case 3:
> +              Key.ScanCode = SCAN_DELETE;
> +              break;
> +          case 11:
> +          case 12:
> +          case 13:
> +          case 14:
> +          case 15:
> +            Key.ScanCode = SCAN_F1 + EscCode - 11;
> +            break;
> +          case 17:
> +          case 18:
> +          case 19:
> +          case 20:
> +          case 21:
> +            Key.ScanCode = SCAN_F6 + EscCode - 17;
> +            break;
> +          case 23:
> +          case 24:
> +            Key.ScanCode = SCAN_F11 + EscCode - 23;
> +            break;
> +          default:
> +            break;
> +          }
> +        } else if (TtyEscapeIndex == 1){
> +          /* 2 character escape code   */
> +          TtyEscapeStr[TtyEscapeIndex++] = UnicodeChar;
> +          continue;
> +        }
> +        else {
> +          DEBUG ((EFI_D_ERROR, "Unexpected state in escape2\n"));
> +        }
> +      }
> +      TerminalDevice->ResetState = RESET_STATE_DEFAULT;
> +
> +      if (Key.ScanCode != SCAN_NULL) {
> +        Key.UnicodeChar = 0;
> +        EfiKeyFiFoInsertOneKey (TerminalDevice, &Key);
> +        TerminalDevice->InputState = INPUT_STATE_DEFAULT;
> +        UnicodeToEfiKeyFlushState (TerminalDevice);
> +        continue;
> +      }
> +
> +      UnicodeToEfiKeyFlushState (TerminalDevice);
> +      break;
> +
>     default:
>       //
>       // Invalid state. This should never happen.
>
>
> Changing state machine code like this makes me nervous, but as far as
> I can tell, non-TtyTerm users are not affected by it, so there is
> little risk of regression.
>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
>
> ------------------------------------------------------------------------------
> Don't Limit Your Business. Reach for the Cloud.
> GigeNET's Cloud Solutions provide you with the tools and support that
> you need to offload your IT needs and focus on growing your business.
> Configured For All Businesses. Start Your Cloud Today.
> https://www.gigenetcloud.com/
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>
>
>
> ------------------------------------------------------------------------------
> Don't Limit Your Business. Reach for the Cloud.
> GigeNET's Cloud Solutions provide you with the tools and support that
> you need to offload your IT needs and focus on growing your business.
> Configured For All Businesses. Start Your Cloud Today.
> https://www.gigenetcloud.com/
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>

------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/

Patch hide | download patch | download mbox

diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
index 03542a4..4616ab3 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
@@ -118,6 +118,7 @@  typedef struct {
 #define INPUT_STATE_LEFTOPENBRACKET       0x04
 #define INPUT_STATE_O                     0x08
 #define INPUT_STATE_2                     0x10
+#define INPUT_STATE_LEFTOPENBRACKET_2     0x20
 
 #define RESET_STATE_DEFAULT               0x00
 #define RESET_STATE_ESC_R                 0x01
diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
index 227df85..12e7f9f 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
@@ -1182,6 +1182,8 @@  UnicodeToEfiKey (
   UINT16              UnicodeChar;
   EFI_INPUT_KEY       Key;
   BOOLEAN             SetDefaultResetState;
+  static UINT16       TtyEscapeStr[3];
+  static INTN         TtyEscapeIndex;
 
   TimerStatus = gBS->CheckEvent (TerminalDevice->TwoSecondTimeOut);
 
@@ -1223,7 +1225,8 @@  UnicodeToEfiKey (
         continue;
       }
 
-      if (UnicodeChar == 'O' && TerminalDevice->TerminalType == VT100TYPE) {
+      if (UnicodeChar == 'O' && (TerminalDevice->TerminalType == VT100TYPE ||
+                                 TerminalDevice->TerminalType == TTYTERMTYPE)) {
         TerminalDevice->InputState |= INPUT_STATE_O;
         TerminalDevice->ResetState = RESET_STATE_DEFAULT;
         continue;
@@ -1371,6 +1374,22 @@  UnicodeToEfiKey (
         default :
           break;
         }
+      } else if (TerminalDevice->TerminalType == TTYTERMTYPE) {
+        /* Also accept VT100 escape codes for F1-F4 for TTY term */
+        switch (UnicodeChar) {
+        case 'P':
+          Key.ScanCode = SCAN_F1;
+          break;
+        case 'Q':
+          Key.ScanCode = SCAN_F2;
+          break;
+        case 'R':
+          Key.ScanCode = SCAN_F3;
+          break;
+        case 'S':
+          Key.ScanCode = SCAN_F4;
+          break;
+        }
       }
 
       if (Key.ScanCode != SCAN_NULL) {
@@ -1514,6 +1533,21 @@  UnicodeToEfiKey (
         }
       }
 
+      /*
+       * The VT220 escape codes that the TTY terminal accepts all have
+       * numeric codes, and there are no ambiguous prefixes shared with
+       * other terminal types.
+       */
+      if (TerminalDevice->TerminalType == TTYTERMTYPE &&
+          Key.ScanCode == SCAN_NULL &&
+          UnicodeChar >= '0' &&
+          UnicodeChar <= '9') {
+        TtyEscapeStr[0] = UnicodeChar;
+        TtyEscapeIndex = 1;
+        TerminalDevice->InputState |= INPUT_STATE_LEFTOPENBRACKET_2;
+        continue;
+      }
+
       if (Key.ScanCode != SCAN_NULL) {
         Key.UnicodeChar = 0;
         EfiKeyFiFoInsertOneKey (TerminalDevice, &Key);
@@ -1527,6 +1561,65 @@  UnicodeToEfiKey (
       break;
 
 
+    case INPUT_STATE_ESC | INPUT_STATE_LEFTOPENBRACKET | INPUT_STATE_LEFTOPENBRACKET_2:
+      /*
+       * Here we handle the VT220 escape codes that we accept.  This
+       * state is only used by the TTY terminal type.
+       */
+      Key.ScanCode = SCAN_NULL;
+      if (TerminalDevice->TerminalType == TTYTERMTYPE) {
+
+        if (UnicodeChar == '~' && TtyEscapeIndex <= 2) {
+          UINTN EscCode;
+          TtyEscapeStr[TtyEscapeIndex] = 0; /* Terminate string */
+          EscCode = StrDecimalToUintn(TtyEscapeStr);
+          switch (EscCode) {
+          case 3:
+              Key.ScanCode = SCAN_DELETE;
+              break;
+          case 11:
+          case 12:
+          case 13:
+          case 14:
+          case 15:
+            Key.ScanCode = SCAN_F1 + EscCode - 11;
+            break;
+          case 17:
+          case 18:
+          case 19:
+          case 20:
+          case 21:
+            Key.ScanCode = SCAN_F6 + EscCode - 17;
+            break;
+          case 23:
+          case 24:
+            Key.ScanCode = SCAN_F11 + EscCode - 23;
+            break;
+          default:
+            break;
+          }
+        } else if (TtyEscapeIndex == 1){
+          /* 2 character escape code   */
+          TtyEscapeStr[TtyEscapeIndex++] = UnicodeChar;
+          continue;
+        }
+        else {
+          DEBUG ((EFI_D_ERROR, "Unexpected state in escape2\n"));
+        }
+      }
+      TerminalDevice->ResetState = RESET_STATE_DEFAULT;
+
+      if (Key.ScanCode != SCAN_NULL) {
+        Key.UnicodeChar = 0;
+        EfiKeyFiFoInsertOneKey (TerminalDevice, &Key);
+        TerminalDevice->InputState = INPUT_STATE_DEFAULT;
+        UnicodeToEfiKeyFlushState (TerminalDevice);
+        continue;
+      }
+
+      UnicodeToEfiKeyFlushState (TerminalDevice);
+      break;
+
     default:
       //
       // Invalid state. This should never happen.