Bugfix parsing control transfer requests

[SeanHowsonAdvCo]

Hi Folks,

I've been porting a product over to USBX from a previous driver & noticed what i'm fairly confident is a small error in the function "_ux_device_stack_control_request_process"

An if statement toward the top of the function evaluates the request type being sent by the host to determine if it is something other than a standard request (class or vendor).

But the statement evaluates "request_value", which usually contains information specific/contextual to each request.
The request type is stored in bits 6 & 7 of bmRequestType & as far as i can see this is stored in request_type...

Several other statements below evaluate what i believe to be the correct variable.

I've modified the file & added a note to the top too. This is actually the first time I've contributed to an open-source project! So apologies in advance if i've missed anything, i've tried to pattern match things like branch-names etc, it seemed most of the bug-fix type PR's i could see were being merged to the dev branch, but if there's anything that needs tweaking let me know.

Best regards
Sean

P.S. If it turns out i'm mistaken, no offence will be taken to a declined PR!

[SeanHowsonAdvCo]

...currently working on the failed check! Appreciate you folks could probably just raise a parallel PR and sort the issue, but i'm keen to contribute here... IOW, please bear with me!

[fdesbiens]

Hi @SeanHowsonAdvCo.

Thank you for this contribution!

Before we can accept it, you need to sign the Eclipse Contributor Agreement (ECA). The purpose of the ECA is to provide a written record that you have agreed to provide your code and documentation contributions under the licenses used by the Eclipse ThreadX project. It also makes it clear that you are promising that what you are contributing to Eclipse is code you wrote, and you have the necessary rights to contribute it to our projects. And finally, it documents a commitment from you that your open source contributions will be permanently on the public record.

Signing the ECA requires an Eclipse Foundation account if you do not already have one. You can create one for free at https://accounts.eclipse.org.

Be sure to use the same email address when you register for the account that you intend to use on Git commit records.

Here is the link to sign the ECA:
https://accounts.eclipse.org/user/login?destination=user/eca

[fdesbiens]

Once you sort this out, @SeanHowsonAdvCo, I will ask a team member to review the PR.

If it passes muster, I will merge to dev, and this should ship with our Q4 2025 release.

[SeanHowsonAdvCo]

Once you sort this out, @SeanHowsonAdvCo, I will ask a team member to review the PR.

If it passes muster, I will merge to dev, and this should ship with our Q4 2025 release.

Hey! Thanks for the quick response, yep, i'm sorting the agreement now (like i say, first time i've done this!). Thanks for the guidance above too.

Hopefully it measures up! But if i am simply mistaken... then that's good all the same!

[SeanHowsonAdvCo]

@fdesbiens sorted! (turns out if you press the same button enough times, it just works)

[fdesbiens]

@SeanHowsonAdvCo I just forced the check to run. All is good. Assigning a knowledgeable committer to review now.

[SeanHowsonAdvCo]

@SeanHowsonAdvCo I just forced the check to run. All is good. Assigning a knowledgeable committer to review now.

Ha, i did think just pressing the same button again and getting a different result seemed a bit odd! Thanks! :)

[SeanHowsonAdvCo]

Hi Folks, just checking back in here, wondering is there's any movement on this? Not sure how often you guys go through PRs, so i don't want to hassle anyone! Just keen to get a bit of feedback, that way i can also filter this into my own use of USBX.
@rahmanih @fdesbiens

[fdesbiens]

Hi @SeanHowsonAdvCo.

Thank you for your patience. If you check our public roadmap (see link below), you will see your PR is in the "in review" column. I also assigned knowledgeable team members to review your contribution.

ThreadX Roadmap

However, we received a flurry of USBX and FileX bug fixes in the last two weeks ahead of our December release. Let's say that @rahmanih has a lot to review right now...

Do not worry: you will get a notification once we have reviewed your PR. I wish I could do it myself, but I am a literal beginner when it comes to USBX.

[ABOUSTM]

using request_type is not correct in this context especially with >> 8 shift, since the request_type takes the first byte of transfer_request_setup, then request_type >> 8 = 0

Could you please provide more details on the issue you faced to identify the root cause
which host request is causing the issue? Class req, vendor req?

[SeanHowsonAdvCo]

using request_type is not correct in this context especially with >> 8 shift, since the request_type takes the first byte of transfer_request_setup, then request_type >> 8 = 0

Could you please provide more details on the issue you faced to identify the root cause which host request is causing the issue? Class req, vendor req?

Hey pal,
Thanks for getting back to me.

My application only enters the function i've queried for vendor specific requests, (the Renesas driver that is used for my DCD layer handles standard requests) so my application never actually reaches line 142. I do agree it wouldnt be right to shift request_type (think i meant to remove this, oops, sorry, i'll push another commit), but neither do i think request_value should be used. Having stepped line by line through the function it really does appear to be using the wrong variable.

request_type is the variable used to store bmRequestType. bits 5 & 6 of this are used to indicate standard/class/vendor (UX_REQUEST_TYPE = 0x60u).

Could i ask you to please evaluate the code below as a comparison against the area i've changed? As this existing code does look at request_type (not shifted) to perform the same check, using the same macros, to establish the type of request being sent from the host:

To try and illustrate what i'm referring to below is a snippet from a reliable site i've used in the past, why would we want to evaluate request_value to deduce request type? (when request_value isn't used to store standard/class/vendor request type).

Thanks ,
Sean

[SeanHowsonAdvCo]

Hi Folks, i posted a response to the initial review a couple of weeks back, hoping someone might review this. Admittedly there was some logic astray in my original commit, but I've since rectified this and think the proposed change needs a little more insight before being fully dismissed. :)
All the best,
Sean

[fdesbiens]

@SeanHowsonAdvCo Thank you for nudging us.

I met with @rahmanih last week to discuss our current slate of USBX PRs, and we reviewed yours as well. Our friends at ST wanted to perform in-depth validation on hardware before merging this, hence the delay. Our window of opportunity is getting smaller for the 202601 release, but I have good hope this will be at least merged to dev in the upcoming weeks.

I appreciate your patience and understanding.

[rahmanih]

Hi @SeanHowsonAdvCo,
thanks for your contribution, as stated by @fdesbiens, the change needs to be tested on real HW with custom class.

keep you updated.

regards
Haithem.

[SeanHowsonAdvCo]

@fdesbiens No worries at all! Just skimming back through my notebook and trying to tick off some loose ends so thought i'd check back in with you guys.

Interested to see what comes back from hardware validation. Fingers crossed!
Cheers,
Sean

[SeanHowsonAdvCo]

@fdesbiens @rahmanih

Hey folks, just went to solve the merge conflicts here, looks like change logs from the top of each file have now been removed.

Just want to double check I've interpreted this correctly before i go ahead an merge this change in from develop, removing the change log from the affected file in this PR?

Cheers,
Sean

[fdesbiens]

@SeanHowsonAdvCo I solved the conflicts and committed my fixes. Please take a look and confirm this looks OK to you.

[fdesbiens]

@SeanHowsonAdvCo BTW, this happened because I removed the old revision histories in all repos. I forgot to mention this in my release notes and my announcement email. My apologies. Since we use Git, it is possible to identify the provenance of the changes much more precisely than the old headers would allow.

[SeanHowson94]

@fdesbiens Hiya,

Yep the commit looks fine to me! Thanks for sorting. No apology needed :)

& yep, we did the same thing at the place i work a while back. Just another part of the file to maintain, when git does the same thing, generally a lot better! :)

[ABOUSTM]

@SeanHowsonAdvCo unfortunately the proposed fix still not fully correct, since this will break with HID, in fact as an example host send get HID report request, bmReqType=0x81, wValue High = 0x22 with proposed code the request will be handled as standard request (0x81 & 0x60) = 0x0 --> request follow standard path and get stalled

I'm working on more safe guard that I will share with you sooner for testing

Regards

[ABOUSTM]

proposed fix breaks HID class requests that are seen as standard request, please refer to section 7.1.1 of HID spec

[SeanHowson94]

@SeanHowsonAdvCo unfortunately the proposed fix still not fully correct, since this will break with HID, in fact as an example host send get HID report request, bmReqType=0x81, wValue High = 0x22 with proposed code the request will be handled as standard request (0x81 & 0x60) = 0x0 --> request follow standard path and get stalled

I'm working on more safe guard that I will share with you sooner for testing

Regards

Hiya,

Thanks for getting back to me, im a bit snowed under with other stuff at work at the moment, so will digest the finer details soon.

No worries, i wasn't expecting a perfect result, ha. Though, can you give me a little more feedback/guidance here, im trying to understand if you're agreeing that the original code (before i raised this PR) isn't quite right? - I'm not saying my fix is the right solution either... just simply keen to understand if you guys are seeing the same problem I am or not?

When i initially raised the PR that's all i was really looking for... just see if i was actually on to something or not :)

The device i'm working with works only as a CDC-ACM peripheral, which probably explains why i didn't pick this up, apologies!

All the best,
Sean

[ABOUSTM]

@SeanHowson94 absolutely the original code does not cover all cases too, I propose this fix for testing

    /* Host-tolerance for GET_DESCRIPTOR/SET_DESCRIPTOR commands.
       Some hosts (notably HID stacks per HID 1.11 section 7.1.1) issue
       GET_DESCRIPTOR for a class-defined descriptor (e.g. Report 0x22,
       Physical 0x23) using bmRequestType=STANDARD instead of CLASS.
       Detect this via the high byte of wValue (bDescriptorType) which,
       per USB-IF allocation, lies in the class-reserved range
       0x21..0x2F. In that case re-route the request to the class layer.
       Vendor descriptors (>= 0x40) and standard descriptors (incl. BOS
       0x0F) are intentionally untouched.  */
    if ((request == UX_GET_DESCRIPTOR || request == UX_SET_DESCRIPTOR) &&
        ((request_type & UX_REQUEST_TYPE) == UX_REQUEST_TYPE_STANDARD) &&
        (((request_value >> 8) & 0xFFu) >= 0x21u) &&
        (((request_value >> 8) & 0xFFu) <= 0x2Fu))
    {

        /* This request is to be handled by the class layer.  */
        request_type &= (ULONG)~UX_REQUEST_TYPE;
        request_type |= UX_REQUEST_TYPE_CLASS;
    } 

Regards,

[SeanHowson94]

Hiya,

Thanks for the clarification!

I think im now finally seeing why request_value was used in the original code, to cover the eventuality you've covered with your fix, but honestly, it's taken me this entire thread and 5 iterations of this response for that to become even remotely clear, it kind of seems like the HID STANDARD request would have entered the original if-statement by sheer luck as opposed to by intentional, explicit, design, the original code checked a couple of bits of the VALUE for a non-zero value, but really it needs the more indepth and explicit condition you've added to your statement. So i can see why my solution would break the HID code, but i think the length of time it's taken me to understand the true intention of that statement suggests a need for improved readability? (which your solution definitely helps with).

My issue was that we were or-ing a "value" with a "type" and then comparing "type". so it just seemed plain wrong. But it seems as though someone chose to use the UX_REQUEST_TYPE and UX_REQUEST_TYPE_STANDARD definitions because their values happened to work, but the actual words in those definitions really dont stack up to me (the original code isn't self documenting). Feel free to explain if i've misunderstood.

Either way, I think the statement you've put forward very cleverly covers both of the problems we've highlighted, my only thoughts going forward are to make sure morons like myself can interpret it and understand that while the statement appears to be aimed heavily at HID's it covers a few other eventualities and instances like mine (CDC-ACM vendor request) have been considered too.... this could definitely be achieved with a comment or two! :)
Cheers,
Sean

[fdesbiens]

@ayedm1 and @rahmanih: Where are we standing on this one? I understand an additional fix was proposed. I would like to close this one in time for the Q2 2026 release, if possible.

[fdesbiens]

@SeanHowsonAdvCo Can you please look at the code suggested by @ABOUSTM above? Does it fix the issue for you?

[SeanHowson94]

@SeanHowsonAdvCo Can you please look at the code suggested by @ABOUSTM above? Does it fix the issue for you?

Hiya, i'm sure i checked fully when i last replied, but give me 18hours to double check this and confirm. Just leaving for the day. will check first thing tomorrow. Cheers!

[SeanHowson94]

@SeanHowsonAdvCo Can you please look at the code suggested by @ABOUSTM above? Does it fix the issue for you?

Hiya,

Yes im happy with the proposed solution, though obviously it needs actually committing to the file.

I can make an additional commit to the file today to add in @ABOUSTM 's fix.

But yes, i think it solves the original issue i'd raised and any other eventualities im personally aware of.

Cheers,
Sean

[fdesbiens]

Hi @SeanHowsonAdvCo

So you waited for so long, and I was working on the release anyway, I integrated @ABOUSTM's fix myself. Thank you again for your patience and understanding. I am merging into dev now, and this will ship with our Q2 2026 release next week.