Skip to content

machine/atsamd21,atsamd51,nrf52840: correct USB CDC #1007

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 29, 2020
Merged

machine/atsamd21,atsamd51,nrf52840: correct USB CDC #1007

merged 2 commits into from
Mar 29, 2020

Conversation

@deadprogram
Copy link
Member

@deadprogram deadprogram commented Mar 28, 2020 •
edited
Loading

This PR corrects the USB CDC composite descriptors for the ATSAMD21, ATSAMD51, and NRF52840 processors.

Turns out that the clues provided by @sago35 were correct, the descriptor needed to be changed.

I tested the example/echo program with these boards:

  • Circuit Playground Express
  • ItsyBitsy M4
  • Circuit Playground Bluefruit

On these OS:

  • Ubuntu
  • macOS
  • Windows 10.

@deadprogram
machine/atsamd21,atsamd51,nrf52840: correct USB CDC composite descrip…
a296c6c 
…tors

Signed-off-by: Ron Evans <ron@hybridgroup.com>
@deadprogram deadprogram changed the title machine/atsamd21,atsamd51,nrf52840: correct USB CDC composite descrip… machine/atsamd21,atsamd51,nrf52840: correct USB CDC Mar 28, 2020
aykevl
aykevl reviewed Mar 28, 2020
View reviewed changes
Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two notes below. Not blocking this PR (there's already a lot of heap allocation) but noting it in case you'd want to fix that.

@deadprogram
machine/atsamd21,atsamd51,nrf52840: refactor USB CDC device descripto…
6dc9f24 
…r to reduce code duplication and heap allocations

Signed-off-by: Ron Evans <ron@hybridgroup.com>
@deadprogram
Copy link
Member Author

deadprogram commented Mar 29, 2020 •
edited
Loading

i added another commit to this PR that addresses the heap allocation.

More importantly, it removes a bunch of code duplication by moving the creation of the device descriptor into shared code for all USB CDC supported boards.

@aykevl
Copy link
Member

aykevl commented Mar 29, 2020

That looks much better!

@aykevl aykevl merged commit 03fa9dd into dev Mar 29, 2020
@aykevl aykevl deleted the correct-usb-cdc-descriptors branch March 29, 2020 15:55
@niaow
Copy link
Member

niaow commented Mar 29, 2020

This commit appears to have broken USB registration with my desktop.

@deadprogram
Copy link
Member Author

deadprogram commented Mar 30, 2020

@jaddr2line what OS are you running on desktop? Also, what program are you running on your Arduino Nano33 board? You should try to test with examples/serial and examples/echo.

@niaow
Copy link
Member

niaow commented Mar 30, 2020

Linux (Manjaro) 4.14-rt, this is also the same machine that used to time out in the USB interrupt

@deadprogram
Copy link
Member Author

deadprogram commented Mar 30, 2020

Now that we are sending the correct vendor and product id, you might need a udev rule for them to be able to be used by non-root user.

@niaow
Copy link
Member

niaow commented Mar 30, 2020 •
edited
Loading

No, that is not it. The device is not enumerating. From dmesg:

[58032.758947] usb 1-7: new full-speed USB device number 73 using xhci_hcd
[58032.915657] usb 1-7: device descriptor read/64, error -71
[58033.152321] usb 1-7: device descriptor read/64, error -71
[58033.385607] usb 1-7: new full-speed USB device number 74 using xhci_hcd
[58033.542360] usb 1-7: device descriptor read/64, error -71
[58033.779007] usb 1-7: device descriptor read/64, error -71
[58033.885653] usb usb1-port7: attempt power cycle
[58034.528961] usb 1-7: new full-speed USB device number 75 using xhci_hcd
[58034.610115] usb 1-7: device descriptor read/8, error 0
[58034.740124] usb 1-7: device descriptor read/8, error 0
[58034.968948] usb 1-7: new full-speed USB device number 76 using xhci_hcd
[58035.046187] usb 1-7: device descriptor read/8, error 0
[58035.179221] usb 1-7: device descriptor read/all, error 0
[58035.179264] usb usb1-port7: unable to enumerate USB device

It looks like it is unable to read the device descriptor.

niaow
niaow reviewed Mar 31, 2020
View reviewed changes
src/machine/machine_atsamd21.go
setEPINTENCLR(0, sam.USB_DEVICE_EPINTENCLR_STALL1)
}
} else {
sendZlp()
Copy link
Member

@niaow niaow Mar 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line appears to have caused the regression. What is it supposed to be doing?

Copy link
Member Author

@deadprogram deadprogram Mar 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I added to send a zlp when the other cases were not handled, which seemed to correct an issue with the USB enumeration on Windows 10, based on a comment from @sago35

Removing that line gets it to work on Linux and macOS, but does not work on Windows 10. I suppose that means we need to further investigate what Windows in sending as the initial request.

Copy link
Member

@sago35 sago35 Mar 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a proposal for sendZlp() below and in the comments that follow.
However, I'm not sure that this idea is correct.
#884 (comment)

The samd51 I'm trying has five USB Device EndPoint Interrupt Flags.

https://www.mouser.com/datasheet/2/268/60001507A-1130176.pdf
P.1161.

I did an additional check on atsamd51 and USB-CDC worked if you called sendZlp() on TRCPT0.
It may be better to process TRCPT0 as follows.
My environment is Windows 10 + PyPortal (samd51).

sago35@f80cc33.

Since sendZlp() clears the bottom 12 bits of PCKSIZE, it assumes that BYTE_COUNT = 0.
The PCKSIZE is described on P.1197.

Copy link
Member

@sago35 sago35 Mar 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

URB_FUNCTION_GET_MS_FEATURE_DESCRIPTOR, which probably exists only in Windows, needs to be handled well.
Maybe a careful reading of the Aruduino and circuitpython sources will solve the problem.

#884 (comment)

Copy link
Member

@sago35 sago35 Mar 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/adafruit/ArduinoCore-samd/blob/master/cores/arduino/USB/USBCore.cpp#L928

Copy link
Member

@sago35 sago35 Mar 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This process is not in machine_atsamd51.go.
On the else side, sendZlp () may not be necessary or should not be called.

https://github.com/adafruit/ArduinoCore-samd/blob/master/cores/arduino/USB/USBCore.cpp#L951

Copy link
Member Author

@deadprogram deadprogram Mar 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see #1013 I think you have the right idea @sago35

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aykevl aykevl aykevl left review comments

@sago35 sago35 sago35 left review comments

@niaow niaow niaow left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@deadprogram @aykevl @niaow @sago35