search

imxrt-rs / imxrt-usbd

USB device driver for i.MX RT processors
Apache License 2.0
7 stars 9 forks source link

USB device may not enumerate on Windows #20

Open mciantyre opened 1 year ago

mciantyre commented 1 year ago

mciantyre/teensy4-rs#126 indicates that a Teensy 4 running the USB serial example (previously maintained here, now at https://github.com/imxrt-rs/imxrt-hal/pull/130) did not enumerate on a Windows host (unknown version). The same device will enumerate on a Raspberry Pi (Linux). macOS is also known to work.

Testing definitely used 0.2 of imxrt-usbd. Though I can't confirm, I believe all testing incorporated 0.2.9 of usb-device.

One workaround: patch usb-device by changing the bcdUSB version from 0x10 to 0x00. My understanding is that this prevents the host from querying for BOS descriptors. I haven't dug deeper into what else this affects.

I don't have a Windows host, so I'm not equipped to test this. Nevertheless, there's a few things I'm curious about:

Finomnis commented 1 year ago

I can confirm that changing bcdUSB from 0x10 to 0x00 fixes the problem. Oddly, this only happened on one of my two Windows machines. The other one worked fine, no problem.

Is this a problem in this crate or in the usb-device upstream? Probably here?

mciantyre commented 1 year ago

Follow-on troubleshooting suggested that https://github.com/mciantyre/teensy4-rs/issues/126 is due to a misconfigured imxrt-log crate. When operating as a high-speed USB device, the logging crate should require a 512 max packet size (MPS) for bulk endpoints.

If increasing the MPS is the right fix, we should take a similar approach in the imxrt-hal USB examples (https://github.com/imxrt-rs/imxrt-hal/pull/130). I wrote down some thoughts while migrating these examples. A patch in usbd-serial that lets us specify the SerialPort MPS might do the trick.

Finomnis commented 1 year ago

I sadly have to report that

export IMXRT_LOG_USB_BULK_MPS=512
cargo clean
cargo run --release

did not fix the issue. Changing bcdUSB from 0x10 to 0x00 did fix it.

Note that I do not use the usb serial port crate. I'm attempting to port the Teensy Rebootor code to allow for self-rebooting during programming. Hence I write a VENDOR_DEFINED custom HID device.

        let class = HIDClass::new(bus_alloc, crate::hid_descriptor::Rebootor::desc(), 10);
        let device = UsbDeviceBuilder::new(bus_alloc, UsbVidPid(0x16C0, 0x0477))
            .product("Self-Rebootor")
            .manufacturer("PJRC")
            .self_powered(true)
            .build();

and

use usbd_hid::descriptor::generator_prelude::*;

#[gen_hid_descriptor(
    (collection = APPLICATION, usage_page = VENDOR_DEFINED_START, usage = 0x0100) = {
        (usage = 0x02,) = {
            output_buffer=output;
        };
    }
)]
pub struct Rebootor {
    output_buffer: [u8; 6],
}

and finally

pub fn poll(&mut self) {
        self.device.poll(&mut [&mut self.class]);

        if self.device.state() == UsbDeviceState::Configured {
            if !self.configured {
                self.device.bus().configure();
            }
            self.configured = true;
        } else {
            self.configured = false;
        }

        if self.configured {
            let mut buf = [0u8; 6];

            let result = self.class.pull_raw_output(&mut buf);
            match result {
                Ok(info) => {
                    let buf = &buf[..info];
                    if buf == b"reboot" {
                        reboot::do_reboot();
                    }
                }
                Err(usb_device::UsbError::WouldBlock) => (),
                Err(_) => {}
            }
        }
    }
Finomnis commented 1 year ago

Ok I just found out that what did help was to modify

        let class = HIDClass::new(bus_alloc, crate::hid_descriptor::Rebootor::desc(), 10);
        let device = UsbDeviceBuilder::new(bus_alloc, UsbVidPid(0x16C0, 0x0477))
            .product("Self-Rebootor")
            .manufacturer("PJRC")
            .self_powered(true)
            .build();

to 

        let class = HIDClass::new(bus_alloc, crate::hid_descriptor::Rebootor::desc(), 10);
        let device = UsbDeviceBuilder::new(bus_alloc, UsbVidPid(0x16C0, 0x0477))
            .product("Self-Rebootor")
            .manufacturer("PJRC")
            .self_powered(true)
            .max_packet_size_0(64)
            .build();

I guess it is the equivalent to the IMXRT_LOG_USB_BULK_MPS. I wonder why this is necessary though.

mciantyre commented 1 year ago

Good find. I believe a 64 byte MPS for EP0 is required for a high-speed USB device. Similar to the bulk endpoint MPS, I'm not sure why Windows doesn't enforce this until the device signals USB2.1 support.

Keep in mind that this USB bus defaults to high-speed mode. The usb-device ecosystem may not be ready to support high-speed devices. If you encounter other issues, it might help to reduce the speed.

If you're looking for collaborators, there's a discussion here about resetting a Teensy 4 over USB.

Finomnis commented 10 months ago

Related: https://github.com/rust-embedded-community/usb-device/pull/116

Might it be possible to update usb-device to 0.3.x, to allow those config options?

Note that this will require a major semver bump, and indirectly then also at imxrt-log and teensy4-bsp. Although personally I'm not a fan of the fact that teensy4-bsp depends on imxrt-log, especially that it just magically enables its features usbd, log and defmt, which I don't need for my usecase. In my opinion that's pretty hefty dependency bloat.

mciantyre commented 10 months ago

25 starts the update to 0.3. I have early signal that the TestClass test suite is still passing, but I haven't made all the other logging changes.

A 0.3 update also affects imxrt-hal, since it re-exports -usbd. I'm thinking about breaking that dependency so that HAL users can bring-their-own -usbd.

I've grown to agree with your BSP opinions. We can coordinate the teensy4-bsp simplifications in mciantyre/teensy4-rs#150.