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; gao...@byosoft.com.cn Cc: G Edhaya Chandran <edhaya.chand...@arm.com>; carolyn.gjert...@amd.com; Samer El-Haj-Mahmoud <samer.el-haj-mahm...@arm.com>; eric....@intel.com; arvinx.c...@intel.com; supreeth.venkat...@amd.com; Sunny Wang <sunny.w...@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.chand...@arm.com>; carolyn.gjert...@amd.com; Samer El-Haj-Mahmoud <samer.el-haj-mahm...@arm.com>; eric....@intel.com; arvinx.c...@intel.com; supreeth.venkat...@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 <gao...@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. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#88542): https://edk2.groups.io/g/devel/message/88542 Mute This Topic: https://groups.io/mt/87274606/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
