Reboots but not to bootloader

alecrivers commented 2 months ago

Thanks for this useful utility.

Unfortunately when I follow the instructions, I'm not seeing intended / expected behavior. I see Van Ooijen Technische Informatica Teensy Rebootor in lsusb, and I get this far:

$ cargo run --release --example teensy4_selfrebootor
    Finished release [optimized] target(s) in 0.02s
     Running `python3 examples/run.py target/thumbv7em-none-eabihf/release/examples/teensy4_selfrebootor`
Flashing target/thumbv7em-none-eabihf/release/examples/teensy4_selfrebootor ...
Teensy Loader, Command Line, Version 2.2
Read "/tmp/tmpk8p16hp7/firmware.hex": 36952 bytes, 1.8% usage
Hard Reboot performed
Waiting for Teensy device...
 (hint: press the reset button)

But then I must press the button to get the rest:

Found HalfKay Bootloader
Read "/tmp/tmpk8p16hp7/firmware.hex": 36952 bytes, 1.8% usage
Programming.....................................
Booting
Teensy successfully flashed.

Without this firmware I don't even get the Hard Reboot performed line, so something is going right, but I'm not sure what state it's getting into.

This is a Teensy 4.1 if it matters.

Finomnis commented 2 months ago

Hmm. That sucks :/ it works with my teensy 4.0. I sadly don't have a 4.1 :/

Might have to think about it or buy a 4.1...

Finomnis commented 2 months ago

I've taken the code from here:

FLASHMEM __attribute__((noinline)) void _reboot_Teensyduino_(void)
{
    if (!(HW_OCOTP_CFG5 & 0x02)) {
        asm("bkpt #251"); // run bootloader
    } else {
        __disable_irq(); // secure mode NXP ROM reboot
        USB1_USBCMD = 0;
        IOMUXC_GPR_GPR16 = 0x00200003;
        // TODO: wipe all RAM for security
        __asm__ volatile("mov sp, %0" : : "r" (0x20201000) : );
        __asm__ volatile("dsb":::"memory");
        volatile uint32_t * const p = (uint32_t *)0x20208000;
        *p = 0xEB120000;
        ((void (*)(volatile void *))(*(uint32_t *)(*(uint32_t *)0x0020001C + 8)))(p);
    }
    __builtin_unreachable();
}

My ported version is here:

/// Reboots the device into the HalfKay bootloader,
/// putting it into firmware flashing mode.
pub fn reboot_to_bootloader() -> ! {
    loop {
        unsafe {
            core::arch::asm!("bkpt #251");
        }
    }
}

Maybe your device runs into the second case I didn't implement?

alecrivers commented 2 months ago

Hi, thanks for the quick reply.

I did some digging in:

I confirmed that asm("bkpt #251"); does reboot to the bootloader, if I just manually run it.
It seems that to get the message "Hard Reboot performed" from teensy_loader_cli (which I was seeing with the standard examples), the device needs to be (1) presenting the correct VID / PID, and (2) handle the control message sent by teensy_loader_cli, namely r = usb_control_msg(rebootor, 0x21, 9, 0x0200, 0, "reboot", 6, 100);. So, those two things were happening, yet the reboot command wasn't being run.
In my application, I need USB serial. I also thought I could use that to debug what was going on. But implementing it meant replacing all the USB handling in the rebootor code. So I did so (which in turn required hackily modifying usbd-serial to support custom command message handling), and published it as a new example teensy4_selfrebootor_with_usb_serial on my fork here: https://github.com/alecrivers/teensy4-selfrebootor , and it works, but of course it means I replaced all the code that was causing the original issue -- so I'm not sure what the cause was.

Since teensy4_selfrebootor_with_usb_serial works for me and is the basis for what I need in my own project, I'm not going to spend more time digging in, but hopefully by publishing this if anyone else hits this issue they'll have something to Google.

Thanks again.

Finomnis commented 2 months ago

Thanks for letting me know! I'm quite new to Rust's USB system, so I might well have done something wrong. I'll take a look and adapt, if possible.

Finomnis commented 2 months ago

@alecrivers was the asynchronous reboot after 10ms necessary? Because my library does it right from the usb handler. That's the major difference I see.

Finomnis commented 2 months ago

Hmm, it seems your code doesn't work for me. It says it doesn't find the rebootor (I'm under Windows, in case that makes a difference)

Finomnis commented 2 months ago

I have a strong feeling that the current way I poll is janky:

    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" {
                        log::info!("Rebooting to HalfKay ...");
                        reboot::reboot_to_bootloader();
                    }
                }
                Err(usb_device::UsbError::WouldBlock) => (),
                Err(_) => {}
            }
        }
    }

The pull_raw_output might be the wrong thing to do, but it was the only thing I got working. I'm also not sure if my hid_descriptor is good the way it is now, but again, it's copy paste until it worked. I don't really understand the USB crate, and it doesn't seem to be documented too well.

You don't explicitely check for the reboot string that the rebootor delivers to the USB node; also I'm not sure why your device doesn't get recognized by the teensy_loader_cli tool. Any idea?

Finomnis commented 2 months ago

@alecrivers If you use my code, what's in the buf return value of pull_raw_output? I have a feeling that the buf == b"reboot" check fails for you for some reason.

alecrivers commented 2 months ago

OK, I finally had time to get back to this (this is a personal project for me) and I can report success! But first to answer questions:

@alecrivers was the asynchronous reboot after 10ms necessary? Because my library does it right from the usb handler. That's the major difference I see.

If I didn't include this, then I got an error message from teensy_loader_cli. I presume because the "ack" never went out for the control packet?

Hmm, it seems your code doesn't work for me. It says it doesn't find the rebootor (I'm under Windows, in case that makes a difference)

That is odd. The rebootor should be found if the device VID & PID are set to UsbVidPid(0x16C0, 0x0477) and the device gets as far as booting.

You don't explicitely check for the reboot string that the rebootor delivers to the USB node

I modified my code to check for this (I haven't pushed yet) but it made no difference to behavior.

@alecrivers If you use my code, what's in the buf return value of pull_raw_output?

I set up a USB serial device so I could dig into this. The result of poll was true, but the result of pull_raw_output was Err(WouldBlock), indicating no packet. I dug in in a bunch more places and faffed about quite a bit before I found that the USB device was receiving a control packet, namely Request { direction: Out, request_type: Class, recipient: Interface, request: 9, value: 512, index: 0, length: 6 }. Looking into usbd-hid, I saw that that was a "SET_REPORT" packet, and as of https://github.com/twitchyliquid64/usbd-hid/commit/cdafaf7cc2c264ddde8e0ae872b816c4df566247, those were handled by shunting them into a special buffer which could then be accessed with pull_raw_report. So, with this diff:

diff --git a/src/lib.rs b/src/lib.rs
index 2f70be0..8d9f9bb 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -69,10 +69,10 @@ impl<'a> Rebootor<'a> {
         if self.configured {
             let mut buf = [0u8; 6];

-            let result = self.class.pull_raw_output(&mut buf);
+            let result = self.class.pull_raw_report(&mut buf);
             match result {
                 Ok(info) => {
-                    let buf = &buf[..info];
+                    let buf = &buf[..info.len];
                     if buf == b"reboot" {
                         log::info!("Rebooting to HalfKay ...");
                         reboot::reboot_to_bootloader();

I made the code check the output of pull_raw_report instead of pull_raw_output, and it works!

Whew. A bit of a lengthy investigative trail but got there in the end.

I'm hesitant to make this a pull request because it seems like the existing code works on your Teensy. I have no idea why it would change between Teensy 4.1 and Teensy 4.0. The only thing that would make sense would be if you were compiling against a (very) old usbd-hid in which SET_REPORT packets are treated like regular input, but I don't see how that could be given that the version is specified.

Finomnis commented 2 months ago

Are you on Windows or Linux? The are a lot of differences in the teensy tool between Windows and Linux, I wouldn't be surprised if that was the difference between you and me. I also can't imagine that 4.0 vs 4.1 would make a difference.

Finomnis commented 2 months ago

I'm hesitant to make this a pull request because it seems like the existing code works on your Teensy.

Please do, so I can test your changes on my teensy.

Finomnis commented 2 months ago

We could also check for both, pull_raw_output and pull_raw_report, to be robust.

alecrivers commented 1 month ago

I'm on Linux. I have made a PR. See if it still works on your Teensy 4.0. But yeah checking both would work, too.

Finomnis commented 1 month ago

Then I guess this is a windows/linux thing. Sadly your change broke it on my side; I modified your PR to use both methods. If it still works, I'll merge and publish.

Finomnis / teensy4-selfrebootor

Reboots but not to bootloader #3