Fix USB CDC NCM endpoint allocation ordering

[RoseBeaupreARA]

The USB CDC NCM device enumerates correctly on my Ubuntu 24.04 machine, but the interface stays down. I use an STM32F405RGT7 chip.

Allocating a dummy endpoint_interrupt_in(None, 8, 255); after comm_ep also fixes the issue. Specifying the endpoint numbers as well.

I suspect that allowing the endpoint allocation to do its thing allocates 0x02 for the bulk in, and 0x81 for the bulk out, leading to a mismatch. They should be on 0x01/0x81, or 0x02/0x82.

Moving the comm_ep allocation after the bulk endpoint allocations therefore fixes the mismatch.

[Dirbaio]

There's nothing in the USB spec that requires IN and OUT endpoint numbers to match. Using 0x02/0x81 should be perfectly fine.

There must be a bug somewhere else and this change is masking it by accident.

• What embassy-usb driver implementation are you using? It might be a bug there.
• Any errors in dmesg?
• Can you share a usb pcap from wireshark?
• Can you share your code?

[RoseBeaupreARA]

• cargo.toml
• main.rs
• dmesg -wH
• Probe-RS run without suggested fix
  with suggested fix
• usbpcap.zip

I would guess that it's the synopsys one, however i'm unsure how to know for sure.
Nothing relevant in dmesg that I can see, output is similar with/without the fix.

[bugadani]

As per NCM11.pdf

6.2.:

The class-specific descriptors must be followed by an Interrupt IN endpoint descriptor.

Just this sentence suggests that at the very least the Interrupt IN endpoint shouldn't be removed from the first interface.

6.3.:

The second alternate setting (alternate setting 1) is used for normal operation, and shall include one bulk IN endpoint and one bulk OUT endpoint.

Nothing about an interrupt endpoint there.

One slight discrepancy I see with the spec is that it refers to one bulk IN endpoint and one bulk OUT endpoint consistently, but that order shouldn't matter, right?

Another random, offtopic-to-this-PR finding I have is this paragraph that differs from implementation:

NCM functions are required to send ConnectionSpeedChange and NetworkConnection notifications in a specific order. To simplify the coding of host device drivers, functions that are going to send a NetworkConnection notification with wValue == 0001h must first send a ConnectionSpeedChange
notification that indicates the connection speed that will be in effect when the new connection takes effect.

[RoseBeaupreARA]

I did also spot that missing notification. I tried to add it, it didn't fix the issue of the interface staying down. The minimal fix really was moving the interrupt endpoint after the bulks. I had a working implementation in C++, and the biggest significant changes were the notifications and the endpoint ordering.

[bugadani]

But the question is, if embassy-usb conforms to the spec, why should it be "fixed"? To me this suggests that the root cause of the issue is somewhere else.

[RoseBeaupreARA]

I think my initial hypothesis is misleading. Moving the line down a few lines adds the comm endpoint to the 2nd alt setting instead of the 1st. Could that be the reason for it to work with my suggested fix?

[bugadani]

Could that be the reason for it to work with my suggested fix?

The current embassy-usb implementation (with an ESP32-S3 device) works just fine on Windows, but not on Ubuntu. Your PR makes it work with Ubuntu, but breaks Windows.

[ExplodingWaffle]

linux logic is here doesn't feel that intended to me...

[bugadani]

Now that #5552 is merged, @RoseBeaupreARA could you check if your problem still exists? There's also a possibility that for some unknown reason you need to insert some delay before running the USB stack, like Ariel OS does.

[RoseBeaupreARA]

@bugadani I tried removing my patch, adding the same delay as ariel-os and adding the following dependency in my project:

embassy-usb-synopsys-otg = { git = "https://github.com/embassy-rs/embassy", rev = "c3631bc01c6b30e6a698e34664e312cbf8bec698" }

The ethernet interferface still stays down. I'm not a Rust veteran, let me know if this was not the right way to test. I took the commit referenced in the PR you mentioned.