abcminiuser
Fixed race condition when multiple SETUP messages queued.
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.
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 bool as int is 16-bit on AVR8 targets.
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 accessible to those of us that don't want to have to learn everything about USB and write everything from scratch just to get a simple project done. Thank you.
I see your point about Endpoint_IsSETUPReceived() being used all over to determine if a request has been handled. If you have a cleaner way to make sure another message doesn't sneak in while processing the first one, that would probably be a better solution, as this change touches just about every piece of USB Device code.
And, yes, a bool would make more sense as a return code for these little 8-bit micros.
Regards,
Ag
On Wed, Nov 18, 2015 at 5:15 AM, Dean Camera [email protected] wrote:
@agprimatic https://github.com/agprimatic thanks for the PR! Looks like you spent a ton of time on this, and I really appreciate it.
I agree with @NicoHood https://github.com/NicoHood in that the return type should be bool as int is 16-bit on AVR8 targets.
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.
— Reply to this email directly or view it on GitHub https://github.com/abcminiuser/lufa/pull/64#issuecomment-157668171.