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

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

[Gao Jie]

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

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

[G Edhaya Chandran]

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

Reviewed-by: G Edhaya Chandran <edhaya.chandran@arm.com>

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

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

[Sunny Wang]

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.

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

[Sunny Wang]

Hi Barton,

As our discussion in today's edk2-test bug triage meeting, my second point " UEFI spec implies that the ReadKeyStrokeEx should clear KeyState (set the value to zero) if there is no key data" is not clear enough. Sorry about that.

To be clear, UEFI spec says the following:
 -  UEFI drivers which implement the  EFI_SIMPLE_TEXT_INPUT_EX protocol are required to return KeyData.Key  and  KeyData.KeyState values. These drivers must always return the most current state of KeyData.KeyState.KeyShiftState and  KeyData.KeyState.KeyToggleState.
However, when user doesn't press any key, there will be no most current state to return. For solving thie problems, @Ruiyu Ni (In Cc) has patch series https://edk2.groups.io/g/devel/message/20311 and commit message below:
 - Today's implementation only return key state when there is a key.But when user doesn't press any key, the key state cannot be returned.The patch changes the ReadKeyStrokeEx() to always return the key state even there is no key pressed.

Therefore, this is just preferred implementation rather than a requirement in spec.

Best Regards,
Sunny
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sunny Wang via groups.io
Sent: 03 March 2022 17:28
To: devel@edk2.groups.io; gaojie@byosoft.com.cn
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; Sunny Wang <Sunny.Wang@arm.com>
Subject: Re: [edk2-devel] [PATCH] uefi-sct/SctPkg:Enhance BBTestReadKeyStrokeExFunctionAutoTestCheckpoint1()

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.





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.

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

[Gao Jie]

Hi Sunny,

Thanks for clarification. I will update the patch to make test fail when
KeyState is not touched.

Thanks
Barton

-----邮件原件-----
发件人: Sunny Wang <Sunny.Wang@arm.com> 
发送时间: 2022年4月7日 22:51
收件人: devel@edk2.groups.io; Sunny Wang <Sunny.Wang@arm.com>;
gaojie@byosoft.com.cn
抄送: 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; Ruiyu Ni <ruiyu.ni@intel.
com>; Sunny Wang <Sunny.Wang@arm.com>
主题: RE: [edk2-devel] [PATCH] uefi-sct/SctPkg:Enhance
BBTestReadKeyStrokeExFunctionAutoTestCheckpoint1()

Hi Barton,

As our discussion in today's edk2-test bug triage meeting, my second point "
UEFI spec implies that the ReadKeyStrokeEx should clear KeyState (set the
value to zero) if there is no key data" is not clear enough. Sorry about
that.

To be clear, UEFI spec says the following:
 -  UEFI drivers which implement the  EFI_SIMPLE_TEXT_INPUT_EX protocol are
required to return KeyData.Key  and  KeyData.KeyState values. These drivers
must always return the most current state of KeyData.KeyState.KeyShiftState
and  KeyData.KeyState.KeyToggleState.
However, when user doesn't press any key, there will be no most current
state to return. For solving thie problems, @Ruiyu Ni (In Cc) has patch
series https://edk2.groups.io/g/devel/message/20311 and commit message
below:
 - Today's implementation only return key state when there is a key.But when
user doesn't press any key, the key state cannot be returned.The patch
changes the ReadKeyStrokeEx() to always return the key state even there is
no key pressed.

Therefore, this is just preferred implementation rather than a requirement
in spec.

Best Regards,
Sunny
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sunny Wang
via groups.io
Sent: 03 March 2022 17:28
To: devel@edk2.groups.io; gaojie@byosoft.com.cn
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; Sunny Wang
<Sunny.Wang@arm.com>
Subject: Re: [edk2-devel] [PATCH] uefi-sct/SctPkg:Enhance
BBTestReadKeyStrokeExFunctionAutoTestCheckpoint1()

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.





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.

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

[G Edhaya Chandran]

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

The patch is up streamed by the below commit:
https://github.com/tianocore/edk2-test/commit/032822757792c5d4d0bfed1fd8524e69ef4f2d17


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116838): https://edk2.groups.io/g/devel/message/116838
Mute This Topic: https://groups.io/mt/87274606/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



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