USB driver cleanups and descriptors for EV3 #355
Conversation
|
Note that I've only compile-tested this for platforms other than the EV3. |
|
Just updated with all the changes. While I was in here, I also unified and const-ified the USB string descriptors. This might need @BertLindeman 's testing at some point since it touches every platform (make sure USB still works and still has correct manufacturer+product strings) |
On git hash 0ac2a66:NXT USB looks fine on linux mint Xia:On windows unknown:
What else usefull? EV3 On windows looks OK:
On linux mint strange.At the linux machine it did not look ok. So after quit some time it was OK. And I could not get it done for the third time. |
|
Serious question wrt EV3: Did you try turning it off and on again? USB definitely doesn't come up correctly until after at least one full power cycle, but I have no idea why. |
Yes I did a few times. I kept plugging and booting until I had a recognized usb on linux again. |
|
Wait... What era of host PC is this? This isn't a USB 3.0 port. |
|
Ehrm, is the EV3 USB 3 compatible? |
|
It's not USB 3 compatible, but USB 3 ports use a completely different driver interface (even if only talking to USB 2 devices) compared to a USB 2 port talking to a USB 2 device. Totally different driver interface means a totally different possible set of bugs. |
This is also wrong. The NXT should not support a device qualifier descriptor as it is not capable of high speed. |
|
Better news: Another linux machine has one usb-3 port: Seems a lot better. [EDIT] The port is at the backside of the machine, so hard to plug-in. |
|
Ugh, if this is indeed an EHCI-specific issue this is going to be very unfun to debug... |
|
@BertLindeman can you try 1067b35 on NXT? |
|
Erm, a bit of a weird question, but can you try the EV3 on one of the rear USB ports on the XPS 8300? I want to rule out that it's not a physical or signal integrity issue with aging hardware. |
|
The rear port is another server (called mumba) the XPS 8300 is mammoet. First the NXT on a usb-2 port: Direct OK even without reboot: Next the EV3 at git hash 57ac538 on a linux usb-3 port: and Looks a bit better ;-) |
|
is it usefull to try the EV3 too at githash 57ac538 on usb-2 and usb-3? The port I tested the EV3 on (front usb port) is the port I constantly use to flash the bricks with. [EDIT] The NXT at this githash 57ac538 on windows still not ok. |
|
Hmm okay, I'm not sure what the problem is here. Might defer this for now, since it works on some systems? @dlech any ideas? |
There was a problem hiding this comment.
I've just been testing NXT so far. There is one issue I can't figure out. When the descriptors we are sending have a size that is a multiple of MAX_EP0_SIZE (8), then the OUT ACK isn't sent when we are done sending the data. I think this is what is causing Windows to fail to enumerate the device. This happens for the configuration descriptor and string descriptor #1.
OK, figured it out. diff --git a/lib/pbio/drv/usb/usb_nxt.c b/lib/pbio/drv/usb/usb_nxt.c
index 13d52e761..364a6e608 100644
--- a/lib/pbio/drv/usb/usb_nxt.c
+++ b/lib/pbio/drv/usb/usb_nxt.c
@@ -286,7 +286,7 @@ static void pbdrv_usb_nxt_write_data(int endpoint, const void *ptr_, uint32_t le
/* If there is more data than can fit in a single packet, queue the
* rest up.
*/
- if (length > packet_size) {
+ if (length >= packet_size) {
length -= packet_size;
pbdrv_usb_nxt_state.tx_data[tx] = (uint8_t *)(ptr + packet_size);
pbdrv_usb_nxt_state.tx_len[tx] = length;
@@ -757,8 +757,7 @@ static void pbdrv_usb_nxt_isr(void) {
}
/* and we will send the following data */
- if (pbdrv_usb_nxt_state.tx_len[endpoint] > 0
- && pbdrv_usb_nxt_state.tx_data[endpoint] != NULL) {
+ if (pbdrv_usb_nxt_state.tx_data[endpoint] != NULL) {
pbdrv_usb_nxt_write_data(endpoint, pbdrv_usb_nxt_state.tx_data[endpoint],
pbdrv_usb_nxt_state.tx_len[endpoint]);
} else {This forces setting the |
I found a chrombook with a USB 2 host port and did some sniffing. For starters, we aren't handling this SETUP request on EV3: 
And we don't seem to be sending a stall in response as we should for messages we can't handle. |
| break; | ||
| switch (setup_pkt.s.bmRequestType & BM_REQ_TYPE_MASK) { | ||
| case BM_REQ_TYPE_STANDARD: | ||
| switch (setup_pkt.s.bmRequestType & BM_REQ_RECIP_MASK) { |
There was a problem hiding this comment.
We don't have a recipient mask check at this level on stm32 or nxt. I think this could be part of the issues with EV3.
There was a problem hiding this comment.
Are you sure this request is being directed at the EV3's USB address?
This is a hub request.
There was a problem hiding this comment.
I am seeing a stall in response to the packet shown
There was a problem hiding this comment.
No, not too sure. I can try again tonight.
|
@BertLindeman could you please retest d812e58 |
This should be less error-prone compared to manually declaring each byte.
Although these are not standard USB descriptors, Pybricks does require them, and they logically fit in with the rest of the descriptor definitions in this file.
This allows for more friendly device setup when connecting to a computer.
These descriptors are expected to be the same on all devices, so moving them to a new file reduces duplicate code.
This changes pbdrv_usb_nxt_write_data to accept a void *, which removes a lot of ugly casting. It also adds a note that a serial number should be implemented in the future
These descriptors are not referenced and thus can never be retrieved.
|
Sure, A bit later, just home from "work" |
Now that we have C11 enabled, we can declare these directly as UTF-16 strings and embed them as constant data. This deduplicates code for manually laying out the string descriptors.
The device qualifier descriptor is used to indicate that a USB 2.0 HS device performs differently from when it is functioning as a FS device. The NXT is not capable of high speed, so it should not implement this.
|
Using github for a long time for browsing only most of the time. Can you remind me how I get best from d812e58 to a build e.g. |
|
Since it's the latest version of this PR, I normally do: or you can wait for the slooooow CI process to finish |
OK as dinner is ready I'll wait for the slooooooooooooooooow CI process. |
|
Needed to flash twice (first time no activity on the display) on linux mint. USB at once OK on linux. USB to win11 👍 
No idea where I might see that device in device manager. [EDIT] |
|
Awesome, looks like the ZLP fix indeed fixes the NXT on Windows! This should all be good now other than the EV3 EHCI(?) issue, which we need to either defer or try and chase down. |
|
Yeah, I think this is ready to merge. Just one last request. We should squash "pbio/drv/usb/usb_nxt.c: Remove extra null packet in configuration descriptor" into "pbio/drv/usb/usb_nxt.c: Fix ZLPs on the control pipe" as we now know the reason for it - the config descriptor is 32 bytes (multiple of 8) and so hit the bug that that we fixed. |
This is needed to correctly indicate end of descriptor when the descriptor size is an exact multiple of the endpoint packet size. This fixes enumeration issues on Windows. Removes the former TODO in the configuration descriptor handling. It is not needed and was masking this issue.
|
One remark now I get the EV3 on this same firmware on windows: I see the device as: Will verify again, but did not see the NXT as such but as LEGO device: Not sure if this is a problem though. |
|
@dlech squashed @BertLindeman This looks like something which might be caused by having the stock Lego drivers and software installed (do you have that installed?) We might want to look into if it causes any actual problems. |
That is my guess too. If you double-click on the device to open the properties, we can see which driver it is using. It could be using the correct driver now and just have some other info left over from the Windows registry that is changing the name and category. |
|
Incidentally, do we want to intentionally put Pybricks devices in a "LEGO Devices" category? Can you do that with additional Microsoft descriptor registry entries? |
|
Correct: More: 
There is so much more in the details, I do not know waht to show.... |
|
Good news: I requested my wife to help pybricks and she plugged-in the NXT on her rather new win11. She never installs LEGO drivers. The NXT correctly recognized as USB device. |
|
The |
|
Will keep the LEGO packages and driver. If I ask nicely I can use my wife's PC. 
|
4 participants
This PR adds the WebUSB and Microsoft descriptors for the EV3. It also refactors the other USB drivers to use the newly-added
usb_ch9.hstructs, which is more readable than defining every byte manually.