From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 45868823ED for ; Fri, 23 Dec 2016 03:53:20 -0800 (PST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga101.fm.intel.com with ESMTP; 23 Dec 2016 03:53:19 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,393,1477983600"; d="scan'208";a="1075587266" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga001.jf.intel.com with ESMTP; 23 Dec 2016 03:53:18 -0800 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 23 Dec 2016 03:53:18 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.59]) by shsmsx102.ccr.corp.intel.com ([169.254.2.88]) with mapi id 14.03.0248.002; Fri, 23 Dec 2016 19:53:14 +0800 From: "Ni, Ruiyu" To: "Zeng, Star" , "edk2-devel@lists.01.org" CC: "Kinney, Michael D" , "Tian, Feng" Thread-Topic: [PATCH] MdeModulePkg ConSplitterDxe: Support toggle state sync Thread-Index: AQHSXFlo671p3LsUfkqOikFIzZFy4KEVS65g Date: Fri, 23 Dec 2016 11:53:14 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5B8627D9@SHSMSX104.ccr.corp.intel.com> References: <1482414197-32744-1-git-send-email-star.zeng@intel.com> In-Reply-To: <1482414197-32744-1-git-send-email-star.zeng@intel.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiOTEyN2NlYmUtNjVjNC00ZDNlLWI1YWMtNTc3Mjc1NGNlYzE3IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IlpGMmhhOFhEVFwvV0owSHFQemxiZEVBV3JlbFp1clMxWUxBUFhkTDh1RUhrPSJ9 x-ctpclassification: CTP_IC x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] MdeModulePkg ConSplitterDxe: Support toggle state sync X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 23 Dec 2016 11:53:20 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Star, 1. ConSplitterTextInPrivateReadKeyStroke(): How about using below loop to e= liminate the "Index--"? for (Index =3D 0; Index < Private->CurrentNumberOfConsoles;) { Status =3D Private->TextInList[Index]->ReadKeyStroke ( Private->TextInList[Index], &CurrentKey ); if (!EFI_ERROR (Status)) { // // Skip partial keystrokes until read out the non-partial one // if ((CurrentKey.ScanCode !=3D CHAR_NULL) || (CurrentKey.UnicodeChar != =3D SCAN_NULL)) { *Key =3D CurrentKey; return Status; } } else { Index++; } } 2. Similar logic exists in ConSplitterTextInReadKeyStrokeEx() 3. How about remove ToggleStateSyncHookSetState() and inline the code? It m= ight make the code more readable. 4. How about directly set Private->VirtualKeyStateExported to TRUE in ConSp= litterTextInSetState()? It might make the code more readable. 5. How about remove ToggleStateSyncToNewConInDev() and inline the code? Use= Private instead of mConIn. It might make the code more readable.=20 6. I agree ToggleStateSyncKeyNotify() has to use mConIn. Seems like a gap o= f UEFI spec that doesn't have VOID * Context as the 2nd parameter of the ho= t key notification function. Regards, Ray >-----Original Message----- >From: Zeng, Star >Sent: Thursday, December 22, 2016 9:43 PM >To: edk2-devel@lists.01.org >Cc: Zeng, Star ; Ni, Ruiyu ; Kinn= ey, Michael D ; >Tian, Feng >Subject: [PATCH] MdeModulePkg ConSplitterDxe: Support toggle state sync > >Register key notify for toggle state (CapsLock, NumLock and ScrollLock) >sync between multiple keyboards. >The implementation for this feature requires keyboard driver supports >EFI_KEY_STATE_EXPOSED, and turns on physical TextInEx partial key >report for toggle state sync. >The virtual TextInEx will report the partial key after it is required >by calling SetState(X | KEY_STATE_VALID_EXPOSED) explicitly. > >Cc: Ruiyu Ni >Cc: Michael Kinney >Cc: Feng Tian >Contributed-under: TianoCore Contribution Agreement 1.0 >Signed-off-by: Star Zeng >--- > .../Universal/Console/ConSplitterDxe/ConSplitter.c | 208 ++++++++++++++++= ++++- > .../Universal/Console/ConSplitterDxe/ConSplitter.h | 5 +- > 2 files changed, 206 insertions(+), 7 deletions(-) > >diff --git a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c >b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c >index 493bcbafdf39..203ad7b06e20 100644 >--- a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c >+++ b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c >@@ -67,6 +67,8 @@ GLOBAL_REMOVE_IF_UNREFERENCED TEXT_IN_SPLITTER_PRIVATE_D= ATA mConIn =3D { > (LIST_ENTRY *) NULL, > (LIST_ENTRY *) NULL > }, >+ 0, >+ FALSE, > > { > ConSplitterSimplePointerReset, >@@ -301,6 +303,157 @@ EFI_DRIVER_BINDING_PROTOCOL gConSplitterAb= solutePointerDriverBinding =3D > }; > > /** >+ Sync current toggle state to new console input device. >+ >+ @param TextInEx Simple Text Input Ex Input protocol pointer. >+ >+**/ >+VOID >+ToggleStateSyncToNewConInDev ( >+ IN EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL *TextInEx >+ ) >+{ >+ // >+ // Sync current toggle state to this new console input device. >+ // >+ TextInEx->SetState (TextInEx, &mConIn.PhysicalKeyToggleState); >+} >+ >+/** >+ Key notify for toggle state sync. >+ >+ @param KeyData A pointer to a buffer that is filled in with >+ the keystroke information for the key that was >+ pressed. >+ >+ @retval EFI_SUCCESS Toggle state sync successfully. >+ >+**/ >+EFI_STATUS >+EFIAPI >+ToggleStateSyncKeyNotify ( >+ IN EFI_KEY_DATA *KeyData >+ ) >+{ >+ UINTN Index; >+ >+ if (((KeyData->KeyState.KeyToggleState & KEY_STATE_VALID_EXPOSED) =3D= =3D KEY_STATE_VALID_EXPOSED) && >+ (KeyData->KeyState.KeyToggleState !=3D mConIn.PhysicalKeyToggleStat= e)) { >+ // >+ // There is toggle state change, sync to other console input devices. >+ // >+ for (Index =3D 0; Index < mConIn.CurrentNumberOfExConsoles; Index++) = { >+ mConIn.TextInExList[Index]->SetState ( >+ mConIn.TextInExList[Index], >+ &KeyData->KeyState.KeyToggleState >+ ); >+ } >+ mConIn.PhysicalKeyToggleState =3D KeyData->KeyState.KeyToggleState; >+ DEBUG ((EFI_D_INFO, "Current toggle state is 0x%02x\n", mConIn.Physic= alKeyToggleState)); >+ } >+ >+ return EFI_SUCCESS; >+} >+ >+/** >+ Initialization for toggle state sync. >+ >+ @param Private Text In Splitter pointer. >+ >+**/ >+VOID >+ToggleStateSyncInitialization ( >+ IN TEXT_IN_SPLITTER_PRIVATE_DATA *Private >+ ) >+{ >+ EFI_KEY_DATA KeyData; >+ VOID *NotifyHandle; >+ >+ // >+ // Initialize PhysicalKeyToggleState that will be synced to new console >+ // input device to turn on physical TextInEx partial key report for >+ // toggle state sync. >+ // >+ Private->PhysicalKeyToggleState =3D KEY_STATE_VALID_EXPOSED; >+ >+ // >+ // Initialize VirtualKeyStateExported to let the virtual TextInEx not r= eport >+ // the partial key even though the physical TextInEx turns on the parti= al >+ // key report. The virtual TextInEx will report the partial key after i= t is >+ // required by calling SetState(X | KEY_STATE_VALID_EXPOSED) explicitly= . >+ // >+ Private->VirtualKeyStateExported =3D FALSE; >+ >+ // >+ // Register key notify for toggle state sync. >+ // >+ KeyData.Key.ScanCode =3D SCAN_NULL; >+ KeyData.Key.UnicodeChar =3D CHAR_NULL; >+ KeyData.KeyState.KeyShiftState =3D 0; >+ KeyData.KeyState.KeyToggleState =3D 0; >+ Private->TextInEx.RegisterKeyNotify ( >+ &Private->TextInEx, >+ &KeyData, >+ ToggleStateSyncKeyNotify, >+ &NotifyHandle >+ ); >+} >+ >+/** >+ Reinitialization for toggle state sync. >+ >+ @param Private Text In Splitter pointer. >+ >+**/ >+VOID >+ToggleStateSyncReInitialization ( >+ IN TEXT_IN_SPLITTER_PRIVATE_DATA *Private >+ ) >+{ >+ UINTN Index; >+ >+ // >+ // Reinitialize PhysicalKeyToggleState that will be synced to new conso= le >+ // input device to turn on physical TextInEx partial key report for >+ // toggle state sync. >+ // >+ Private->PhysicalKeyToggleState =3D KEY_STATE_VALID_EXPOSED; >+ >+ // >+ // Reinitialize VirtualKeyStateExported to let the virtual TextInEx not= report >+ // the partial key even though the physical TextInEx turns on the parti= al >+ // key report. The virtual TextInEx will report the partial key after i= t is >+ // required by calling SetState(X | KEY_STATE_VALID_EXPOSED) explicitly= . >+ // >+ Private->VirtualKeyStateExported =3D FALSE; >+ >+ for (Index =3D 0; Index < Private->CurrentNumberOfExConsoles; Index++) = { >+ Private->TextInExList[Index]->SetState ( >+ Private->TextInExList[Index], >+ &Private->PhysicalKeyToggleState >+ ); >+ } >+} >+ >+/** >+ Toggle state sync hook SetState. >+ >+ @param KeyToggleState Pointer to key toggle state. >+ >+**/ >+VOID >+ToggleStateSyncHookSetState ( >+ IN OUT EFI_KEY_TOGGLE_STATE *KeyToggleState >+ ) >+{ >+ // >+ // Always turn on physical TextInEx partial key report for >+ // toggle state sync. >+ // >+ *KeyToggleState |=3D EFI_KEY_STATE_EXPOSED; >+} >+ >+/** > The Entry Point for module ConSplitter. The user code starts with this = function. > > Installs driver module protocols and. Creates virtual device handles fo= r ConIn, >@@ -538,6 +691,8 @@ ConSplitterTextInConstructor ( > > InitializeListHead (&ConInPrivate->NotifyList); > >+ ToggleStateSyncInitialization (ConInPrivate); >+ > ConInPrivate->AbsolutePointer.Mode =3D &ConInPrivate->AbsolutePointerMo= de; > // > // Allocate buffer for Absolute Pointer device >@@ -1890,6 +2045,8 @@ ConSplitterTextInExAddDevice ( > Private->TextInExList[Private->CurrentNumberOfExConsoles] =3D TextInEx; > Private->CurrentNumberOfExConsoles++; > >+ ToggleStateSyncToNewConInDev (TextInEx); >+ > // > // Extra CheckEvent added to reduce the double CheckEvent(). > // >@@ -3321,6 +3478,10 @@ ConSplitterTextInReset ( > } > } > >+ if (!EFI_ERROR (ReturnStatus)) { >+ ToggleStateSyncReInitialization (Private); >+ } >+ > return ReturnStatus; > } > >@@ -3363,8 +3524,18 @@ ConSplitterTextInPrivateReadKeyStroke ( > &CurrentKey > ); > if (!EFI_ERROR (Status)) { >- *Key =3D CurrentKey; >- return Status; >+ // >+ // If it is partial keystroke, skip it. >+ // >+ if ((CurrentKey.ScanCode =3D=3D CHAR_NULL) && (CurrentKey.UnicodeCh= ar =3D=3D SCAN_NULL)) { >+ // >+ // Try to read key from this physical console input device again. >+ // >+ Index--; >+ } else { >+ *Key =3D CurrentKey; >+ return Status; >+ } > } > } > >@@ -3542,6 +3713,10 @@ ConSplitterTextInResetEx ( > } > } > >+ if (!EFI_ERROR (ReturnStatus)) { >+ ToggleStateSyncReInitialization (Private); >+ } >+ > return ReturnStatus; > > } >@@ -3607,8 +3782,22 @@ ConSplitterTextInReadKeyStrokeEx ( > &CurrentKeyData > ); > if (!EFI_ERROR (Status)) { >- CopyMem (KeyData, &CurrentKeyData, sizeof (CurrentKeyData)); >- return Status; >+ // >+ // If it is partial keystroke, check if virtual KeyState has been r= equired to be exposed. >+ // >+ if ((CurrentKeyData.Key.ScanCode =3D=3D CHAR_NULL) && (CurrentKeyDa= ta.Key.UnicodeChar =3D=3D SCAN_NULL)) { >+ if (Private->VirtualKeyStateExported) { >+ CopyMem (KeyData, &CurrentKeyData, sizeof (CurrentKeyData)); >+ return Status; >+ } >+ // >+ // Try to read key from this physical console input device again. >+ // >+ Index--; >+ } else { >+ CopyMem (KeyData, &CurrentKeyData, sizeof (CurrentKeyData)); >+ return Status; >+ } > } > } > >@@ -3641,6 +3830,7 @@ ConSplitterTextInSetState ( > TEXT_IN_SPLITTER_PRIVATE_DATA *Private; > EFI_STATUS Status; > UINTN Index; >+ EFI_KEY_TOGGLE_STATE PhysicalKeyToggleState; > > if (KeyToggleState =3D=3D NULL) { > return EFI_INVALID_PARAMETER; >@@ -3648,6 +3838,9 @@ ConSplitterTextInSetState ( > > Private =3D TEXT_IN_EX_SPLITTER_PRIVATE_DATA_FROM_THIS (This); > >+ PhysicalKeyToggleState =3D *KeyToggleState; >+ ToggleStateSyncHookSetState (&PhysicalKeyToggleState); >+ > // > // if no physical console input device exists, return EFI_SUCCESS; > // otherwise return the status of setting state of physical console inp= ut device >@@ -3655,13 +3848,16 @@ ConSplitterTextInSetState ( > for (Index =3D 0; Index < Private->CurrentNumberOfExConsoles; Index++) = { > Status =3D Private->TextInExList[Index]->SetState ( > Private->TextInExList[Index]= , >- KeyToggleState >+ &PhysicalKeyToggleState > ); > if (EFI_ERROR (Status)) { > return Status; > } > } > >+ Private->PhysicalKeyToggleState =3D PhysicalKeyToggleState; >+ Private->VirtualKeyStateExported =3D (((*KeyToggleState) & EFI_KEY_STAT= E_EXPOSED) !=3D 0); >+ > return EFI_SUCCESS; > > } >@@ -3765,7 +3961,7 @@ ConSplitterTextInRegisterKeyNotify ( > } > } > >- InsertTailList (&mConIn.NotifyList, &NewNotify->NotifyEntry); >+ InsertTailList (&Private->NotifyList, &NewNotify->NotifyEntry); > > *NotifyHandle =3D NewNotify; > >diff --git a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.h >b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.h >index e32abbaea133..99ac5b55d302 100644 >--- a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.h >+++ b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.h >@@ -1,7 +1,7 @@ > /** @file > Private data structures for the Console Splitter driver > >-Copyright (c) 2006 - 2012, Intel Corporation. All rights reserved.
>+Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.
> This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD= License > which accompanies this distribution. The full text of the license may be= found at >@@ -88,6 +88,7 @@ typedef struct { > UINTN Rows; > } TEXT_OUT_SPLITTER_QUERY_DATA; > >+#define KEY_STATE_VALID_EXPOSED (EFI_TOGGLE_STATE_VALID | EFI_KEY_STATE_E= XPOSED) > > #define TEXT_IN_EX_SPLITTER_NOTIFY_SIGNATURE SIGNATURE_32 ('T', 'i', '= S', 'n') > >@@ -128,6 +129,8 @@ typedef struct { > EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL **TextInExList; > UINTN TextInExListCount; > LIST_ENTRY NotifyList; >+ EFI_KEY_TOGGLE_STATE PhysicalKeyToggleState; >+ BOOLEAN VirtualKeyStateExported; > > > EFI_SIMPLE_POINTER_PROTOCOL SimplePointer; >-- >2.7.0.windows.1