Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STALL at GET DESCRIPTOR(CONFIGURATION) #4

Closed
tmk opened this issue Dec 18, 2020 · 6 comments
Closed

STALL at GET DESCRIPTOR(CONFIGURATION) #4

tmk opened this issue Dec 18, 2020 · 6 comments
Labels

Comments

@tmk
Copy link
Owner

tmk commented Dec 18, 2020

With some keyboards(Logitech G213, NuType F1) getConfDescr() fails to get Configuration Descriptor with STALL error(05).

BM Init
Addr:01
NC:01
HID_PROTOCOL_KEYBOARD
[gC]Cl:003B
crIN:05
crIN:05
crIN:05
crIN:05
crIN:05

pUsb->getConfDescr(bAddress, 0, i, &confDescrParserA);

USB_Host_Shield_2.0/Usb.cpp

Lines 801 to 803 in 5c303ed

uint8_t USB::getConfDescr(uint8_t addr, uint8_t ep, uint16_t nbytes, uint8_t conf, uint8_t* dataptr) {
return ( ctrlReq(addr, ep, bmREQ_GET_DESCR, USB_REQUEST_GET_DESCRIPTOR, conf, USB_DESCRIPTOR_CONFIGURATION, 0x0000, nbytes, nbytes, dataptr, NULL));
}

USB_Host_Shield_2.0/Usb.cpp

Lines 807 to 822 in 5c303ed

uint8_t USB::getConfDescr(uint8_t addr, uint8_t ep, uint8_t conf, USBReadParser *p) {
const uint8_t bufSize = 64;
uint8_t buf[bufSize];
USB_CONFIGURATION_DESCRIPTOR *ucd = reinterpret_cast<USB_CONFIGURATION_DESCRIPTOR *>(buf);
uint8_t ret = getConfDescr(addr, ep, 9, conf, buf);
if(ret)
return ret;
uint16_t total = ucd->wTotalLength;
//USBTRACE2("\r\ntotal conf.size:", total);
return ( ctrlReq(addr, ep, bmREQ_GET_DESCR, USB_REQUEST_GET_DESCRIPTOR, conf, USB_DESCRIPTOR_CONFIGURATION, 0x0000, total, bufSize, buf, p));
}

Configuration Descriptor is read twice in getConfDescr().

1. nbytes = 9

uint8_t ret = getConfDescr(addr, ep, 9, conf, buf);

ctrlReq(addr, ep, bmREQ_GET_DESCR, USB_REQUEST_GET_DESCRIPTOR, conf, USB_DESCRIPTOR_CONFIGURATION, 0x0000, 9, 9, buf, NULL) can get first 9 bytes of Configuration descriptor successfully.

2. total = 59(descriptor length), bufSize = 64

return ( ctrlReq(addr, ep, bmREQ_GET_DESCR, USB_REQUEST_GET_DESCRIPTOR, conf, USB_DESCRIPTOR_CONFIGURATION, 0x0000, total, bufSize, buf, p));

ctrlReq(addr, ep, bmREQ_GET_DESCR, USB_REQUEST_GET_DESCRIPTOR, conf, USB_DESCRIPTOR_CONFIGURATION, 0x0000, 59, 64, buf, p) fails with STALL when it reads whole of Configuration descriptor.
Buffer size is 64. Total length of Configuration descriptor is 59 in case of Logitech G213.

Cause

This part is related to this STALL.
In ctrlReq() InTransfer() returns STALL(0x05) when it reads whole of Configuration descriptor from the keyboards.

In case of Logitech G213 total length of Configuration descriptor is 59(0x3b) and bMaxPacketSize0 is 64.

STALL may be caused by out of bounds read on the descriptor in InTransfer()

USB_Host_Shield_2.0/Usb.cpp

Lines 170 to 173 in 5c303ed

uint16_t read = nbytes;
//uint16_t read = (left<nbytes) ? left : nbytes;
rcode = InTransfer(pep, nak_limit, &read, dataptr);

Line commented out read = (left<nbytes) ? left : nbytes seems to work well, but not sure why this has not been used.

ctrlReq(... total, nbytes, ...) looks doing right.

  1. Read first 9 bytes of Configuration descriptor: ctrlReq(... 9, 9, ...) calls Intransfer(, read = 9, ) and it works well.
  2. Read whole Confguration desriptor: ctrlReq(... 59, 64, ...) calls InTransfer(, read = 64, ) which is expected to read 59 bytes. This can issue excess IN transaction unnecessarily and reads the descriptor beyond the end in case that it is given incorrect 'max packet size'(8).

Root Cause: Correct 'max packet size' of the control endpoint0 is not given when InTransfer() is called.

Termination conditions of InTransfer() are:

USB_Host_Shield_2.0/Usb.cpp

Lines 283 to 286 in 5c303ed

/* The transfer is complete under two conditions: */
/* 1. The device sent a short packet (L.T. maxPacketSize) */
/* 2. 'nbytes' have been transferred. */
if((pktsize < maxpktsize) || (*nbytesptr >= nbytes)) // have we transferred 'nbytes' bytes?

The first one is not met because 'max packet size' is incorrect(8 instead of 64). Also the second one also doesn't work because it is given hard-coded 64 instead of real descriptor length 59.

How InTransfer() fails

With Logitech G213 total length of Configuration descriptor is 59(0x3b) and bMaxPacketSize0 is 64.

When bMaxPacketSize0 is 64 only one IN packet is needed to read whole of the descriptor(59).

Terminal condition:
if((pktsize < maxpktsize) || (*nbytesptr >= nbytes))

  1. Short packet: Size of the last packet is smaller than bMaxPacketSize0
  2. Already read: Total read size is bigger than requested size

1. maxpktsize is incorrect 8 and nbytes is hard-coded 64

Even after reading whole of the descriptor with IN-1 it doesn't stop because if (59 < 8 || 59 >= 64) doessn't match. And it starts unnecessary second IN transaction and ends with STALL error.

IN-1: 59 bytes of the descriptor(Short packet: 59<64)
IN-2: Some keyboards may response 0-length packet(ZLP)?  but others response with STALL error.

2. maxpktsize is correct 64

It stops after receiving IN-1 with matching like if (59 < 64 || 59 >= 64).
It doesn't issue second IN transfer and returns successfully.

3. nbytes is correct total descriptor size(59)

I stops after receiving IN-1 with matching like if (59 < 8 || 59 >= 59).
It doesn't issue second IN transfer and returns successfully.

4. maxpktsize is correct 64 and nbytes is correct total descriptor size(59)

I stops after receiving IN-1 with matching like if (59 < 64 || 59 >= 59).
It doesn't issue second IN transfer and returns successfully.

@tmk
Copy link
Owner Author

tmk commented Dec 18, 2020

Fix for total size of desriptor(size of requested data)

This gives InTransfer() correct total size of descriptor and seems to solve the problem.

https://geekhack.org/index.php?topic=69169.msg2992750#msg2992750

diff --git a/Usb.cpp b/Usb.cpp
index d8a51cb..2e334c0 100644
--- a/Usb.cpp
+++ b/Usb.cpp
@@ -170,8 +170,8 @@ uint8_t USB::ctrlReq(uint8_t addr, uint8_t ep, uint8_t bmReqType, uint8_t bReque
 #if defined(ESP8266) || defined(ESP32)
                         yield(); // needed in order to reset the watchdog timer on the ESP8266
 #endif
-                                uint16_t read = nbytes;
-                                //uint16_t read = (left<nbytes) ? left : nbytes;
+                                //uint16_t read = nbytes;
+                                uint16_t read = (left<nbytes) ? left : nbytes;
 
                                 rcode = InTransfer(pep, nak_limit, &read, dataptr);
                                 if (rcode) USBTRACE2("crIN:", rcode);

Fix for 'max packet size'

Root cause is that correct 'max packet size' of endpoint 0 is not given when getConfDescr() is called in Init() of hidboot.h.

This stores correct 'max packet size' and makes it availble to InTransfer().

diff --git a/hidboot.h b/hidboot.h
index 4076400..9e98ab2 100644
--- a/hidboot.h
+++ b/hidboot.h
@@ -375,6 +375,8 @@ uint8_t HIDBoot<BOOT_PROTOCOL>::Init(uint8_t parent, uint8_t port, bool lowspeed
 
         p->lowspeed = lowspeed;
 
+        pUsb->setEpInfoEntry(bAddress, 1, epInfo);
+
         if(len)
                 rcode = pUsb->getDevDescr(bAddress, 0, len, (uint8_t*)buf);

@tmk
Copy link
Owner Author

tmk commented Dec 18, 2020

Logitech G213 USB Descriptor

Total length of Configuration descriptor is 59(0x3b) and bMaxPacketSize0 is 64.

Device Descriptor:
  bLength                18
  bDescriptorType         1
  bcdUSB               2.00
  bDeviceClass            0 
  bDeviceSubClass         0 
  bDeviceProtocol         0 
  bMaxPacketSize0        64
  idVendor           0x046d Logitech, Inc.
  idProduct          0xc336 
  bcdDevice            9.00
  iManufacturer           1 Logitech
  iProduct                2 Gaming Keyboard G213
  iSerial                 3 056F38743330
  bNumConfigurations      1
  Configuration Descriptor:
    bLength                 9
    bDescriptorType         2
    wTotalLength       0x003b
    bNumInterfaces          2
    bConfigurationValue     1
    iConfiguration          4 U109.00_B0006
    bmAttributes         0xa0
      (Bus Powered)
      Remote Wakeup
    MaxPower              500mA

keyleds/keyleds#37 (comment)

NuType F1 USB Descriptor

Total length of Configuration descriptor is 59(0x3b) and bMaxPacketSize0 is 64.

    ---------------------- Device Descriptor ----------------------
bLength                  : 0x12 (18 bytes)
bDescriptorType          : 0x01 (Device Descriptor)
bcdUSB                   : 0x200 (USB Version 2.00)
bDeviceClass             : 0x00 (defined by the interface descriptors)
bDeviceSubClass          : 0x00
bDeviceProtocol          : 0x00
bMaxPacketSize0          : 0x40 (64 bytes)
idVendor                 : 0x05AC (Apple)
idProduct                : 0x0256
bcdDevice                : 0x0110
iManufacturer            : 0x01 (String Descriptor 1)
 Language 0x0409         : "SONiX"
iProduct                 : 0x02 (String Descriptor 2)
 Language 0x0409         : "USB DEVICE"
iSerialNumber            : 0x00 (No String Descriptor)
bNumConfigurations       : 0x01 (1 Configuration)
    ------------------ Configuration Descriptor -------------------
bLength                  : 0x09 (9 bytes)
bDescriptorType          : 0x02 (Configuration Descriptor)
wTotalLength             : 0x003B (59 bytes)
bNumInterfaces           : 0x02 (2 Interfaces)
bConfigurationValue      : 0x01 (Configuration 1)
iConfiguration           : 0x00 (No String Descriptor)
bmAttributes             : 0xA0
 D7: Reserved, set 1     : 0x01
 D6: Self Powered        : 0x00 (no)
 D5: Remote Wakeup       : 0x01 (yes)
 D4..0: Reserved, set 0  : 0x00
MaxPower                 : 0x32 (100 mA)

https://pastebin.com/fTWMypTS
https://geekhack.org/index.php?topic=69169.msg2893824#msg2893824

tmk added a commit that referenced this issue Dec 24, 2020
This makes correct bMaxPacketSize0 available before getConfDescr().
Without this InTransfer() can request data beyond the end of data
and response with STALL error.
#4
tmk added a commit that referenced this issue Dec 24, 2020
Without this fix InTransfer() can read beyond requested length and
fails with STALL. This can happen when correct value is not set to
maxPktSize.
#4
tmk added a commit to tmk/tmk_keyboard that referenced this issue Dec 24, 2020
Fix for STALL at GET DESCRIPTOR(CONFIGURATION)
tmk/USB_Host_Shield_2.0#4
@tmk
Copy link
Owner Author

tmk commented Dec 24, 2020

Fixed at:
15a323c
2e4b49c

@tmk
Copy link
Owner Author

tmk commented Feb 10, 2021

Logitech G413 have this issue as well.
felis#601 (comment)

tmk added a commit to tmk/tmk_core that referenced this issue Feb 10, 2021
Fix for STALL at GET DESCRIPTOR(CONFIGURATION)
tmk/USB_Host_Shield_2.0#4
@tmk tmk added the SOLVED label Nov 23, 2021
@Rowan-git
Copy link

Hello I am very noob to this keyboard and code lingo, I am here from this video https://www.youtube.com/watch?v=GZEoss4XIgc and have no clue how to apply this patch to my G213 as in this video the Hex files are given if the supplied hex files dont work or of there is something that I am missing please let me know.

@tmk
Copy link
Owner Author

tmk commented Jan 7, 2022

I don't know exactly what the video describes honestly.

You need to compile firmware yourself probably. Refer this for how to build tmk_keyboard.
https://github.com/tmk/tmk_keyboard/wiki#build-firmware

@tmk tmk closed this as completed May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants