Re: [edk2-devel] [PATCH] uefi-sct/SctPkg:Enhance BBTestReadKeyStrokeExFunctionAutoTestCheckpoint1()

["Sunny Wang" <Sunny.Wang@arm.com>]

Sorry for the late review.

The change looks fine. However, I'm confused about the comment " Log the error here and return key states are not supported when high order bit of KeyToggleState is 0 or KeyState not touched". It looks like returning EFI_UNSUPPORTED won't log any error/failure in the result log. Did I overlook anything here?

Furthermore, it looks like UEFI spec implies that the ReadKeyStrokeEx should clear KeyState (set the value to zero) if there is no key data. There is also a code change for doing this in edk2. https://edk2.groups.io/g/devel/message/20311
Therefore, if I didn't overlook anything here, we should separately deal with the "(Key.KeyState.KeyShiftState == 0xFFFFFFFF) || (Key.KeyState.KeyToggleState == 0xFF)" as a failure rather than directly returning EFI_UNSUPPORTED.

Barton and Edhaya, what do you guys think?

Best Regards,
Sunny
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gao Jie via groups.io
Sent: 24 November 2021 03:16
To: devel@edk2.groups.io
Cc: G Edhaya Chandran <Edhaya.Chandran@arm.com>; Carolyn.Gjertsen@amd.com; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; eric.jin@intel.com; arvinx.chen@intel.com; Supreeth.Venkatesh@amd.com
Subject: [edk2-devel] [PATCH] uefi-sct/SctPkg:Enhance BBTestReadKeyStrokeExFunctionAutoTestCheckpoint1()

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2386

Enhance BBTestReadKeyStrokeExFunctionAutoTestCheckpoint1()
to handle ReadKeyStrokeEx implementation which returns EFI_NOT_READY
but without touching KeyToggleState.

Signed-off-by: Barton Gao <gaojie@byosoft.com.cn>
---
 .../BlackBoxTest/SimpleTextInputExBBTestFunction.c    | 11 ++++++++++-
 .../BlackBoxTest/SimpleTextInputExBBTestFunction.c    | 11 ++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/SimpleTextInputEx/BlackBoxTest/SimpleTextInputExBBTestFunction.c b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/SimpleTextInputEx/BlackBoxTest/SimpleTextInputExBBTestFunction.c
index 0398bc26..c1669959 100644
--- a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/SimpleTextInputEx/BlackBoxTest/SimpleTextInputExBBTestFunction.c
+++ b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/SimpleTextInputEx/BlackBoxTest/SimpleTextInputExBBTestFunction.c
@@ -1160,6 +1160,12 @@ BBTestReadKeyStrokeExFunctionAutoTestCheckpoint1 (
   ValidState[5] = EFI_TOGGLE_STATE_VALID | EFI_KEY_STATE_EXPOSED | EFI_NUM_LOCK_ACTIVE | EFI_CAPS_LOCK_ACTIVE;
   ValidState[6] = EFI_TOGGLE_STATE_VALID | EFI_KEY_STATE_EXPOSED | EFI_SCROLL_LOCK_ACTIVE |EFI_NUM_LOCK_ACTIVE | EFI_CAPS_LOCK_ACTIVE;

+  //
+  // Set all bits to one (invalid values) for both KeyShiftState and KeyToggleState
+  //
+  Key.KeyState.KeyShiftState = 0xFFFFFFFF;
+  Key.KeyState.KeyToggleState = 0xFF;
+
   //
   //Read next keystroke from the input device
   //
@@ -1171,7 +1177,10 @@ BBTestReadKeyStrokeExFunctionAutoTestCheckpoint1 (
     return Status;
   }

-  if ((Key.KeyState.KeyToggleState & EFI_TOGGLE_STATE_VALID) == 0) {
+  if (((Key.KeyState.KeyToggleState & EFI_TOGGLE_STATE_VALID) == 0) || (Key.KeyState.KeyShiftState == 0xFFFFFFFF) || (Key.KeyState.KeyToggleState == 0xFF)) {
+    //
+    // Log the error here and return key states are not supported when high order bit of KeyToggleState is 0 or KeyState not touched
+    //
     return EFI_UNSUPPORTED;
   }

diff --git a/uefi-sct/SctPkg/TestCase/UEFI/IHV/Protocol/SimpleTextInputEx/BlackBoxTest/SimpleTextInputExBBTestFunction.c b/uefi-sct/SctPkg/TestCase/UEFI/IHV/Protocol/SimpleTextInputEx/BlackBoxTest/SimpleTextInputExBBTestFunction.c
index adbf3dcf..e20ed345 100644
--- a/uefi-sct/SctPkg/TestCase/UEFI/IHV/Protocol/SimpleTextInputEx/BlackBoxTest/SimpleTextInputExBBTestFunction.c
+++ b/uefi-sct/SctPkg/TestCase/UEFI/IHV/Protocol/SimpleTextInputEx/BlackBoxTest/SimpleTextInputExBBTestFunction.c
@@ -1160,6 +1160,12 @@ BBTestReadKeyStrokeExFunctionAutoTestCheckpoint1 (
   ValidState[5] = EFI_TOGGLE_STATE_VALID | EFI_KEY_STATE_EXPOSED | EFI_NUM_LOCK_ACTIVE | EFI_CAPS_LOCK_ACTIVE;
   ValidState[6] = EFI_TOGGLE_STATE_VALID | EFI_KEY_STATE_EXPOSED | EFI_SCROLL_LOCK_ACTIVE |EFI_NUM_LOCK_ACTIVE | EFI_CAPS_LOCK_ACTIVE;

+  //
+  // Set all bits to one (invalid values) for both KeyShiftState and KeyToggleState
+  //
+  Key.KeyState.KeyShiftState = 0xFFFFFFFF;
+  Key.KeyState.KeyToggleState = 0xFF;
+
   //
   //Read next keystroke from the input device
   //
@@ -1171,7 +1177,10 @@ BBTestReadKeyStrokeExFunctionAutoTestCheckpoint1 (
     return Status;
   }

-  if ((Key.KeyState.KeyToggleState & EFI_TOGGLE_STATE_VALID) == 0) {
+  if (((Key.KeyState.KeyToggleState & EFI_TOGGLE_STATE_VALID) == 0) || (Key.KeyState.KeyShiftState == 0xFFFFFFFF) || (Key.KeyState.KeyToggleState == 0xFF)) {
+    //
+    // Log the error here and return key states are not supported when high order bit of KeyToggleState is 0 or KeyState not touched
+    //
     return EFI_UNSUPPORTED;
   }

--
2.31.0.windows.1







IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.