arduino / arduino/ArduinoCore-samd
Problems related to USB device support
- Dominant language
- C
- Stars
- 502
- Forks
- 740
- PR merge metrics
- No merged PRs in 30d
Description
I just saw the two commits from January and August related to the DoubleBufferedEPOutHandler class. In fact this area still has several remaining issues:
When a SET_CONFIGURATION request is received, the code deletes the bulk-OUT endpoint's existing handler (along with its buffers) and creates a new one. This is wasteful; it would be better to continue using the old handler and simply reinitialize the endpoint.
Also, deleting the existing buffers means that positive values previously returned by available() would become invalid. Continuing to use the same handler and buffers avoids this problem.
The code seems to assume that the host will send a SET_CONFIGURATION request only when the device is enumerated following each attach. This is not always true; the host can send SET_CONFIGURATION at any time and some hosts do this as a weak form of reset. When this happens the host expects the device to set the endpoints' data toggles back to 0, but the existing code does not do this. (The host also expects the device to clear the endpoints' HALT feature; this is a little less of an issue because nothing in the existing code ever sets the HALT feature, but it still should be addressed.)
In the same way, hosts sometimes use SET_INTERFACE as a very weak form of reset. The existing code doesn't implement this request correctly at all, which perhaps isn't surprising since it doesn't implement alternate settings for interfaces. Still, the device shouldn't pretend to handle a request and then ignore it. Fortunately the USB spec allows a device to STALL a SET_INTERFACE request if alternate settings aren't supported; that's what the code should do here.
The code that handles SET_FEATURE, CLEAR_FEATURE, and GET_STATUS requests for endpoint HALT is completely bogus. It doesn't even try to look at the endpoint number in the SETUP packet!
Attached is a proposed patch that addresses all these issues except the last. There are a few other odd things still remaining, such as why DoubleBufferedEPOutHandler::handleEndpoint() should need to use "synchronized" and the reason for including USBDeviceClass::stall(). We can discuss them later.
[usb-fixes.txt](https://github.com/arduino/ArduinoCore-samd/files/2479863/usb-fixes.txt)
Contributor guide
No contributing guide indexed for this repository
Research direction
Start by reading DoubleBufferedEPOutHandler and USBDeviceClass, then inspect the handling of SET_CONFIGURATION, SET_INTERFACE, SET_FEATURE, CLEAR_FEATURE, and GET_STATUS requests. Compare the behavior with the attached usb-fixes.txt proposal and verify that endpoint buffers, data toggles, HALT handling, and unsupported alternate settings match the USB expectations described in the issue.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c
- Domain
- embedded-iot
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 42/100