Fixed race condition when multiple SETUP messages queued. #64
Conversation
When USB_Device_ProcessControlRequest() is called, it first does the HID-specific EVENT_USB_Device_ControlRequest() processing. If that doesn't handle the request, then since Endpoint_isSETUPReceived() is still true, then to do the generic processing. If _that_ doesn't handle the request, then it calls Endpoint_ClearSETUP() and stalls the transaction. In some cases, a new SETUP message is arriving between the Endpoint_ClearSETUP() in EVENT_USB_Device_ControlRequest() and the check for Endpoint_isSETUPReceived() right after EVENT_USB_Device_ControlRequest() returns. In this case, the switch statement in USB_Device_ProcessControlRequest() tries to process the HID-specific message with the generic processing. It doesn't handle the HID-specific message, so it falls through to the STALL code. My suggestion is to only check for Endpoint_isSETUPReceived() once at the top of USB_Device_ProcessControlRequest(). If it is set, then try first the specific handler, then if that doesn't satisfy the request, then try the generic handler. If _that_ doesn't handle the request, then generate the STALL. I did this by using a request_handled variable, and modifying EVENT_USB_Device_ControlRequest() to return a value that says whether it handled the request or not.
|
I think this can me merged with a single commit, not those thousands. Also a bool would suit better and not consume so much memory. I have not looked at the functionality at all, just those facts I saw. |
|
@agprimatic thanks for the PR! Looks like you spent a ton of time on this, and I really appreciate it. I agree with @NicoHood in that the return type should be I'm not concerned with the number of commits (I certainly don't worry about it on my repos, as anyone who splunks through it will quickly discover) but I can always squash merge it into a single commit. One issue I see with this is that Endpoint_IsSETUPReceived() is used all over the USB class drivers to determine if a request has been handled; if the semantics are going to change this will have to be refactored in some manner. I'll have to have a bit of a think how best to solve it in a way that will cause the least disruption to users while solving the majority of the edge cases. |
|
Hi Dean, I'm glad to help. You've written a great piece of software. It makes USB I see your point about Endpoint_IsSETUPReceived() being used all over to And, yes, a bool would make more sense as a return code for these little Regards, Ag On Wed, Nov 18, 2015 at 5:15 AM, Dean Camera notifications@github.com
|
I was having a problem with STALL messages in my HID device, and I traced it down to this:
When USB_Device_ProcessControlRequest() is called, it first does the HID-specific EVENT_USB_Device_ControlRequest() processing. If that doesn't handle the request, then since Endpoint_isSETUPReceived() is still true, then to do the generic processing. If that doesn't handle the request, then it calls Endpoint_ClearSETUP() and stalls the transaction.
In my case, it looks like a new SETUP message is arriving between the Endpoint_ClearSETUP() in EVENT_USB_Device_ControlRequest() and the check for Endpoint_isSETUPReceived() right after EVENT_USB_Device_ControlRequest() returns.
In this case, the switch statement in USB_Device_ProcessControlRequest() tries to process the HID-specific message with the generic processing. It doesn't handle the HID-specific message, so it falls through to the STALL code.
My suggestion is to only check for Endpoint_isSETUPReceived() once at the top of USB_Device_ProcessControlRequest(). If it is set, then try first the specific handler, then if that doesn't satisfy the request, then try the generic handler. If that doesn't handle the request, then generate the STALL. I did this by using a request_handled variable, and modifying EVENT_USB_Device_ControlRequest() to return a value that says whether it handled the request or not.
This fixed my problem with the race condition. I think my EVENT_USB_Device_ControlRequest() is taking longer than expected, so another message is queued up behind it. By the time I retire the request, another request has arrived.