raspberrypi / picotool

BSD 3-Clause "New" or "Revised" License
555 stars 95 forks source link

Add ability to access device using stdio_usb #23

Closed kilograham closed 2 years ago

lurch commented 3 years ago

If I use the -f or -F options on an RP2040 which is already in BOOTSEL mode, I get an error saying "ERROR: Forced command requires a single rebootable RP2040 device to be targeted." which could be slightly confusing. Maybe if the RP2040 is already in BOOTSEL mode it should ignore any 'force' flags?

kilograham commented 3 years ago

If I use the -f or -F options on an RP2040 which is already in BOOTSEL mode, I get an error saying "ERROR: Forced command requires a single rebootable RP2040 device to be targeted." which could be slightly confusing. Maybe if the RP2040 is already in BOOTSEL mode it should ignore any 'force' flags?

Good point; -f should be "reboot it if you have to"

lurch commented 3 years ago

When you've got multiple RP2040 devices plugged in at once (and so you need to use the --address option to specify which one you're targeting), the 'force' option becomes kinda confusing. This is because when the device reboots, it enumerates with a different address, and so the command that you were trying to run then reports "ERROR: Communication with RP2040 device failed". It pains me to suggest removing functionality, but maybe the easiest way to solve this is to drop the -F flag (which IMHO is confusing anyway (but maybe that's just the help-string)), and have reboot be the only command that accepts a -f flag? And then once the device has been force-rebooted the user can work out what the device's new address is, and run picotool a second time to run the command they actually want? A more complex solution might be to disallow the -f or -F flags being used with any command that isn't reboot when you're also specifying the --address option? :shrug:


Also, I wonder if a pictool list command might be useful? EDIT: moved to #26


And finally (for now) it would be nice if picotool was able to tell the difference between a "RP2040 device with a USB serial connection" where the reset vendor interface is active (so the -f flag will (probably?) work), and a "RP2040 device with a USB serial connection" where the reset vendor interface isn't active (so the -f flag (definitely?) won't work). I think that information might be available from the USB descriptor, but I've obviously got no idea how tricky it'd be to add that support to picotool.

kilograham commented 3 years ago

When you've got multiple RP2040 devices plugged in at once (and so you need to use the --address option to specify which one you're targeting), the 'force' option becomes kinda confusing. This is because when the device reboots, it enumerates with a different address, and so the command that you were trying to run then reports "ERROR: Communication with RP2040 device failed".

Yes, this is a (known) problem... sadly libusb on Windows does not support the good way to handle this (i.e. the ability to watch for devices appearing/disappearing)

It pains me to suggest removing functionality, but maybe the easiest way to solve this is to drop the -F flag (which IMHO is confusing anyway (but maybe that's just the help-string)), and have reboot be the only command that accepts a -f flag? And then once the device has been force-rebooted the user can work out what the device's new address is, and run picotool a second time to run the command they actually want?

We do not want to throw the baby out with the bath water. The single Pico case is likely to be common and an integratedpicotool -f with all sensible commands means the ability to use in scripts etc where user intervention is not desriable.

We should try to improve the multiple Pico case in the future (we can probably do a diff of the set of USB descriptors to figure out which device was rebooted in the majority of cases)

A more complex solution might be to disallow the -f or -F flags being used with any command that isn't reboot when you're also specifying the --address option? 🤷

That seems reasonable

Also, I wonder if a pictool list command might be useful? Consider the following scenario:

% lsusb | grep 2e8a                                       
Bus 020 Device 012: ID 2e8a:000a 2e8a Pico  Serial: 000000000000
Bus 020 Device 009: ID 2e8a:0005 2e8a Board in FS mode  Serial: 000000000000
% ./picotool info
No accessible RP2040 devices in BOOTSEL mode were found.

but:

Device at bus 20, address 9 appears to be a RP2040 MicroPython device not in BOOTSEL mode.
Device at bus 20, address 12 appears to be a RP2040 device with a USB serial connection, so consider -f or -F.

if I then plug in a Pico with a blank Flash chip (wiped using flash_nuke.uf2) I get:

% lsusb | grep 2e8a
Bus 020 Device 012: ID 2e8a:000a 2e8a Pico  Serial: 000000000000
Bus 020 Device 009: ID 2e8a:0005 2e8a Board in FS mode  Serial: 000000000000
Bus 020 Device 013: ID 2e8a:0003 2e8a RP2 Boot  Serial: E0C912D24340
% ./picotool info
Program Information
 none

In this latter case it would be nice if e.g. a ./picotool list could tell me:

Device at bus 20, address 9 appears to be a RP2040 MicroPython device not in BOOTSEL mode.
Device at bus 20, address 12 appears to be a RP2040 device with a USB serial connection, so consider -f or -F.
Device at bus 20, address 13 appears to be a RP2040 device in BOOTSEL mode.

For bonus points, it could even include the other VID/PID combos from https://github.com/raspberrypi/usb-pid ? 😉

And finally (for now) it would be nice if picotool was able to tell the difference between a "RP2040 device with a USB serial connection" where the reset vendor interface is active (so the -f flag will (probably?) work), and a "RP2040 device with a USB serial connection" where the reset vendor interface isn't active (so the -f flag (definitely?) won't work). I think that information might be available from the USB descriptor, but I've obviously got no idea how tricky it'd be to add that support to picotool.

All of this should be discussed in a separate issue

lurch commented 3 years ago

For cross-linking purposes: I believe this fixes both #13 and #14 and links to https://github.com/raspberrypi/pico-sdk/pull/197

lurch commented 3 years ago

I'm seeing this behaviour (on Linux), not sure if it's explained by picotool not waiting long enough between issuing the 'reset' command and querying the 'info' ? (this with only a single Pico, plugged directly into my laptop)

$ sudo picotool info
No accessible RP2040 devices in BOOTSEL mode were found.

but:

Device at bus 3, address 61 appears to be a RP2040 device with a USB serial connection, so consider -f or -F.
$ sudo picotool info -F
The device was rebooted as requested into BOOTSEL mode so the command can be executed.

No accessible RP2040 devices in BOOTSEL mode were found.
$ sudo picotool info -F
ERROR: Forced command requires a single rebootable RP2040 device to be targeted.
$ sudo picotool info
Program Information
 name:      hello_usb
 web site:  https://github.com/raspberrypi/pico-examples/tree/HEAD/hello_world/usb
 features:  USB stdin / stdout

Just to be clear, I think it's the "No accessible RP2040 devices in BOOTSEL mode were found." that isn't quite right - it should instead be displaying the "Program Information" that a subsequent call to picotool info displays?

kilograham commented 3 years ago

yes, i bet that is it

lurch commented 3 years ago

If I use the -f or -F options on an RP2040 which is already in BOOTSEL mode, I get an error saying "ERROR: Forced command requires a single rebootable RP2040 device to be targeted." which could be slightly confusing. Maybe if the RP2040 is already in BOOTSEL mode it should ignore any 'force' flags?

I'm still seeing this behaviour on Linux - your previous reply implied this might be changing?

lurch commented 3 years ago

If a Pico is in BOOTSEL mode, and I run sudo ./picotool reboot -u it successfully reboots back into BOOTSEL mode, but it also prints ERROR: Communication with RP2040 device failed which seems a bit odd?

EDIT: on Linux x64

kilograham commented 3 years ago

can you include platform you are running on with the issues

lurch commented 3 years ago

Dunno if it's deliberate behaviour or not, but on Linux when I run picotool help reboot the -f option is listed under the Selecting the device to reboot heading, whereas on Windows when I run the same command the -f option appears under the Reboot type heading. (The -F option is also missing on Windows, but I realise that's intentional)

lurch commented 3 years ago

On Windows, if I run picotool reboot -f -a -u it reboots into BOOTSEL mode, but if I run picotool reboot -f -u -a it reboots to application mode! It should probably just error if you specify both the -a and the -u flags?

Ahh, same behaviour on Linux too.

kilograham commented 3 years ago

On Windows, if I run picotool reboot -f -a -u it reboots into BOOTSEL mode, but if I run picotool reboot -f -u -a it reboots to application mode! It should probably just error if you specify both the -a and the -u flags?

Ahh, same behaviour on Linux too.

will fix this

lurch commented 3 years ago

The Mac build seems to be broken again? :confused:

[ 25%] Building CXX object CMakeFiles/picotool.dir/main.cpp.o
/Users/andrew/pico/picotool/main.cpp:472:5: error: use of undeclared identifier 'Sleep'
    Sleep(ms);
    ^
1 error generated.
make[2]: *** [CMakeFiles/picotool.dir/main.cpp.o] Error 1
lurch commented 3 years ago

On Linux x64, the "info <filename>" seems to be broken? If I do

./picotool info ../../pico-examples/build/blink/blink.uf2

it just hangs (doesn't do anything), and when I Ctrl-C I get Segmentation fault (core dumped) :slightly_frowning_face:

lurch commented 3 years ago

On Linux x64, if I do

sudo ./picotool reboot -a -u

it now reports:

ERROR: Cannot specify both -u and -a reboot options
Segmentation fault

The first line is good, the second line is bad :wink:

But I can confirm the "ignore -f / -F flag if specified but not needed" issue seems to have been fixed :tada:

Hmmm, if I do multiple

sudo ./picotool reboot -u

in a row, it sometimes works with no error message, but sometimes it prints ERROR: Communication with RP2040 device failed and sometimes prints ERROR: The RP2040 device returned an error: <unknown>.

Doing multiple

sudo ./picotool reboot -a -f

or

sudo ./picotool reboot -a -F

in a row always seems to work with no errors reported. :+1:

I dunno if it's worth "fixing", but these commands work:

sudo ./picotool reboot -a -f -F
sudo ./picotool reboot -a -F -f
sudo ./picotool reboot -f -F -a
sudo ./picotool reboot -F -f -a

but

sudo ./picotool reboot -f -a -F

reports:

ERROR: unexpected option: -F

SYNOPSYS:
    picotool reboot [-a] [-u] [--bus <bus>] [--address <addr>] [-f] [-F]

Use "picotool help reboot" for more info

and

sudo ./picotool reboot -F -a -f

reports:

ERROR: unexpected option: -f

SYNOPSYS:
    picotool reboot [-a] [-u] [--bus <bus>] [--address <addr>] [-f] [-F]

Use "picotool help reboot" for more info
lurch commented 3 years ago

On Windows 10, if I do

picotool -a -u

I get: image

(EDIT: I also get the same error popup if I run picotool info ..\..\hello_world.uf2)

I noticed that on Linux I get a nice "The device was rebooted into [application/BOOTSEL] mode" if I run "sudo picotool reboot [-a/-u] -f", but there's no message printed when I run picotool reboot -a -f or picotool reboot -u -f on Windows?

Windows sometimes also prints ERROR: Communication with RP2040 device failed after running picotool reboot -u -f.

I dunno if it's worth special-casing the error-message, but if the Pico is in application mode (on Windows) and I run

picotool reboot -a

it tells me:

No accessible RP2040 devices in BOOTSEL mode were found.

but:

Device at bus 2, address 23 appears to be a RP2040 device with a USB serial connection, not in BOOTSEL mode. You can force reset it into BOOTSEL mode via 'picotool reboot -f -u' first.

however it seems that picotool reboot -a -f would work in this scenario?

kilograham commented 3 years ago

On Linux x64, the "info " seems to be broken? If I do

./picotool info ../../pico-examples/build/blink/blink.uf2

it just hangs (doesn't do anything), and when I Ctrl-C I get Segmentation fault (core dumped) slightly_frowning_face

uninitialized variable bug was introduced, will push shortly

kilograham commented 3 years ago

as for the -F -a -f stuff, i wrote a whole new command line parser for this tool, and there are a few edge cases... trying to fix them without breaking other stuff is not worth it. (it does work if you put the options in the right place after all)

kilograham commented 3 years ago

@lurch can you give this a final once over, i want to merge

lurch commented 3 years ago

(Testing on a Pi400, but I wouldn't expect other platforms to behave differently...)

There seems to be an odd interaction between picotool load -x and the 'force' flags?

$ sudo ./picotool load -x p4.uf2 -f
The device was rebooted as requested into BOOTSEL mode so the command can be executed.

Loading into Flash: [==============================]  100%
The device has been left accessible, but without the drive mounted; use 'picotool reboot' to reboot into regular BOOTSEL mode or application mode.
$ sudo ./picotool load -x p4.uf2 -F
The device was rebooted as requested into BOOTSEL mode so the command can be executed.

Loading into Flash: [==============================]  100%
The device has been left accessible, but without the drive mounted; use 'picotool reboot' to reboot into regular BOOTSEL mode or application mode.

Note how the -f and -F flags are behaving identically, which I don't think is right? :confused:

kilograham commented 3 years ago

(Testing on a Pi400, but I wouldn't expect other platforms to behave differently...)

There seems to be an odd interaction between picotool load -x and the 'force' flags?

$ sudo ./picotool load -x p4.uf2 -f
The device was rebooted as requested into BOOTSEL mode so the command can be executed.

Loading into Flash: [==============================]  100%
The device has been left accessible, but without the drive mounted; use 'picotool reboot' to reboot into regular BOOTSEL mode or application mode.
$ sudo ./picotool load -x p4.uf2 -F
The device was rebooted as requested into BOOTSEL mode so the command can be executed.

Loading into Flash: [==============================]  100%
The device has been left accessible, but without the drive mounted; use 'picotool reboot' to reboot into regular BOOTSEL mode or application mode.

Note how the -f and -F flags are behaving identically, which I don't think is right? 😕

yeah, good catch, the main issue is that -x causes a reboot so should be considered similarly to a reboot command

lurch commented 3 years ago

I built and tested the latest version this afternoon on Linux x64, Raspberry Pi OS (on a Pi400), Windows 10 x64 and MacOS Catalina x64. Apart from my "little catches" mentioned earlier, it all seems to work pretty well :+1: (with the obvious difference that it's only the 'reboot' command on Windows which accepts a 'force' flag)

However I do wonder if it wouldn't be simpler / more consistent to simply make:

        -F, --force-no-reboot
            Force a device not in BOOTSEL mode but running compatible code to reset
            so the command can be executed. After executing the command (unless the
            command itself is a 'reboot') the device will be left connected and
            accessible to picotool, but without the RPI-RP2 drive mounted

behave the same (on Linux and Mac) as picotool reboot -u i.e. have the -F option leave the RP2040 in "full BOOTSEL mode", rather than this unusual "the device will be left connected and accessible to picotool, but without the RPI-RP2 drive mounted" mode? Unless there's a good reason for introducing this latter mode? :shrug:

lurch commented 3 years ago

Oh, and I always need to apply the change from #27 before I'm able to build on Windows.

kilograham commented 3 years ago

I built and tested the latest version this afternoon on Linux x64, Raspberry Pi OS (on a Pi400), Windows 10 x64 and MacOS Catalina x64. Apart from my "little catches" mentioned earlier, it all seems to work pretty well +1 (with the obvious difference that it's only the 'reboot' command on Windows which accepts a 'force' flag)

However I do wonder if it wouldn't be simpler / more consistent to simply make:

        -F, --force-no-reboot
            Force a device not in BOOTSEL mode but running compatible code to reset
            so the command can be executed. After executing the command (unless the
            command itself is a 'reboot') the device will be left connected and
            accessible to picotool, but without the RPI-RP2 drive mounted

behave the same (on Linux and Mac) as picotool reboot -u i.e. have the -F option leave the RP2040 in "full BOOTSEL mode", rather than this unusual "the device will be left connected and accessible to picotool, but without the RPI-RP2 drive mounted" mode? Unless there's a good reason for introducing this latter mode? shrug

yeah this is my preferred behavior... -f is preferable because you don't end up with the drive appearing as USB for no reason, so I'd like to keep it, confusing as it may be

kilograham commented 3 years ago

Oh, and I always need to apply the change from #27 before I'm able to build on Windows.

i'm not 100% convinced that it is a good general fix, but i have included it (here now), because it is probably not worse than what was there.

kilograham commented 3 years ago

I made a bunch of subtle changes to the wording of messages, so hopefully things make more sense now.

If does seem possible to get the Ubuntu USB stack confused (apparently) in so far as the device can be back in USBBOOT mode, but the OS still thinks it is connected with CDC available. (This seems to be the cause of -f saying it is rebooting, then the device not being found anyway... i have made the error messages better in this eventuality)

lurch commented 2 years ago

Running the latest version on x64 Linux, I get this sequence of commands:

$ ./picotool info
No accessible RP2040 devices in BOOTSEL mode were found.

but:

Device at bus 3, address 46 appears to be a RP2040 device with a USB serial
    connection, so consider -f (or -F) to force reboot in order to run the
    command.
$ ./picotool info -F
ERROR: Unable to access device to reboot it; Use sudo or setup a udev rule

$ sudo ./picotool info -F
The device was asked to reboot into BOOTSEL mode so the command can be executed.

Program Information
 name:      hello_usb
 web site:  https://github.com/raspberrypi/pico-examples/tree/HEAD/hello_world/usb
 features:  USB stdin / stdout

            The device has been left accessible, but without the drive mounted;
            use 'picotool reboot' to reboot into regular BOOTSEL mode or
            application mode.
$ ./picotool info
No accessible RP2040 devices in BOOTSEL mode were found.

but:

Device at bus 3, address 49 appears to be a RP2040 device in BOOTSEL mode, but
    picotool was unable to connect
$ sudo ./picotool info
Program Information
 name:      hello_usb
 web site:  https://github.com/raspberrypi/pico-examples/tree/HEAD/hello_world/usb
 features:  USB stdin / stdout

Would it perhaps make sense to also display the "Use sudo or setup a udev rule" message as part of the "Device at bus 3, address 49 appears to be a RP2040 device in BOOTSEL mode, but picotool was unable to connect" output in the penultimate command above? Or does "a RP2040 device in BOOTSEL mode, but picotool was unable to connect" cover more error cases than just "Use sudo or setup a udev rule" ?

lurch commented 2 years ago

the main issue is that -x causes a reboot so should be considered similarly to a reboot command

This appears to be fixed now :+1: (both sudo ./picotool load p4.uf2 -x -f and sudo ./picotool load p4.uf2 -x -F display "The device was rebooted to start the application.") However I wonder if the (unless the command itself is a 'reboot') in the -F, --force-no-reboot section of the help output should now say (unless the command itself is a 'reboot' or a 'load -x') ?

lurch commented 2 years ago

yeah this is my preferred behavior... -f is preferable because you don't end up with the drive appearing as USB for no reason, so I'd like to keep it, confusing as it may be

That's fair enough. However there's part of me that wonders if, for the sake of symmetry, there ought to be a dedicated flag to the reboot command (e.g. -p) that could also be used to enter this "connected and accessible to picotool, but without the RPI-RP2 drive mounted" state? :shrug:

lurch commented 2 years ago

Another minor-nitpick thing; in the output of picotool help I see:

SYNOPSYS:
    picotool info [-b] [-p] [-d] [-l] [-a] [--bus <bus>] [--address <addr>] [-f]
                [-F]
    picotool info [-b] [-p] [-d] [-l] [-a] <filename> [-t <type>]
    picotool load [-v] [-x] <filename> [-t <type>] [-o <offset>] [--bus <bus>]
                [--address <addr>] [-f] [-F]
    picotool save [-p] [--bus <bus>] [--address <addr>] [-f] [-F] <filename> [-t
                <type>]
    picotool save -a [--bus <bus>] [--address <addr>] [-f] [-F] <filename> [-t
                <type>]
    picotool save -r <from> <to> [--bus <bus>] [--address <addr>] [-f] [-F]
                <filename> [-t <type>]
    picotool verify [--bus <bus>] [--address <addr>] [-f] [-F] <filename> [-t
                <type>] [-r <from> <to>] [-o <offset>]
    picotool reboot [-a] [-u] [--bus <bus>] [--address <addr>] [-f] [-F]
    picotool version [-s]
    picotool help [<cmd>]

Looks a bit odd that sometimes the command-options are listed before the [--bus <bus>] [--address <addr>] [-f] [-F] part, and at other times they're listed after the [--bus <bus>] [--address <addr>] [-f] [-F] part?

lurch commented 2 years ago
$ ./picotool help save
SAVE:
    Save the program / memory stored in flash on the device to a file.

SYNOPSYS:
    picotool save [-p] [--bus <bus>] [--address <addr>] [-f] [-F] <filename> [-t
                <type>]
    picotool save -a [--bus <bus>] [--address <addr>] [-f] [-F] <filename> [-t
                <type>]
    picotool save -r <from> <to> [--bus <bus>] [--address <addr>] [-f] [-F]
                <filename> [-t <type>]

OPTIONS:
    Selection of data to save
        -p, --program
            Save the installed program only. This is the default
        -a, --all
            Save all of flash memory
        -r, --range
            Save a range of memory. Note that UF2s always store complete 256
            byte-aligned blocks of 256 bytes, and the range is expanded
            accordingly
        <from>
            The lower address bound in hex
        <to>
            The upper address bound in hex
    Source device selection
        --bus <bus>
            Filter devices by USB bus number
        --address <addr>
            Filter devices by USB device address
        -f, --force
            Force a device not in BOOTSEL mode but running compatible code to
            reset so the command can be executed. After executing the command
            (unless the command itself is a 'reboot') the device will be
            rebooted back to application mode
        -F, --force-no-reboot
            Force a device not in BOOTSEL mode but running compatible code to
            reset so the command can be executed. After executing the command
            (unless the command itself is a 'reboot') the device will be left
            connected and accessible to picotool, but without the RPI-RP2 drive
            mounted
    File to save to
        <filename>
            The file name
        -t <type>
            Specify file type (uf2 | elf | bin) explicitly, ignoring file
            extension
$ sudo ./picotool save p3.elf
ERROR: Save to ELF file is not supported

Perhaps it shouldn't mention elf in the save help-text?

kilograham commented 2 years ago

Perhaps it shouldn't mention elf in the save help-text?

this i don't care about - we might support it in the future

lurch commented 2 years ago

Not that it matters, but any idea why an out-of-the-box build creates a Release build on Linux and macOS, but a Debug build on Windows? (according to the output of picotool version).

I can confirm that as long as the PICO_SDK_PATH and LIBUSB_ROOT environment variables are set appropriately, this PR now builds on Windows without any extra tweaks needed. :+1: