arduino / arduino/ArduinoCore-samd

Problems related to USB device support

Open
#366 2 comments 0 reactions 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.