Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to access device using stdio_usb #23

Merged
merged 2 commits into from
Oct 21, 2021
Merged

Add ability to access device using stdio_usb #23

merged 2 commits into from
Oct 21, 2021

Conversation

kilograham
Copy link
Contributor

No description provided.

main.cpp Outdated Show resolved Hide resolved
main.cpp Outdated Show resolved Hide resolved
main.cpp Outdated Show resolved Hide resolved
@lurch
Copy link
Contributor

lurch commented Apr 29, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor

lurch commented Apr 29, 2021

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? 🤷


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.

main.cpp Outdated Show resolved Hide resolved
@kilograham
Copy link
Contributor Author

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

main.cpp Outdated Show resolved Hide resolved
liamfraser
liamfraser previously approved these changes May 4, 2021
@lurch
Copy link
Contributor

lurch commented May 7, 2021

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

@lurch
Copy link
Contributor

lurch commented May 7, 2021

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
Copy link
Contributor Author

yes, i bet that is it

@lurch
Copy link
Contributor

lurch commented Jun 9, 2021

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?

main.cpp Outdated Show resolved Hide resolved
@lurch
Copy link
Contributor

lurch commented Jun 9, 2021

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
Copy link
Contributor Author

can you include platform you are running on with the issues

@lurch
Copy link
Contributor

lurch commented Jun 9, 2021

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
Copy link
Contributor

lurch commented Jun 9, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor

lurch commented Jun 25, 2021

The Mac build seems to be broken again? 😕

[ 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

main.cpp Outdated Show resolved Hide resolved
@lurch
Copy link
Contributor

lurch commented Jun 25, 2021

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) 🙁

@lurch
Copy link
Contributor

lurch commented Jun 25, 2021

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 😉

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

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. 👍

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
Copy link
Contributor

lurch commented Jul 19, 2021

(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? 😕

@kilograham
Copy link
Contributor Author

kilograham commented Jul 19, 2021

(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 lurch mentioned this pull request Jul 19, 2021
@lurch
Copy link
Contributor

lurch commented Jul 19, 2021

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 👍 (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? 🤷

@lurch
Copy link
Contributor

lurch commented Jul 19, 2021

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

@kilograham
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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)

main.cpp Show resolved Hide resolved
@lurch
Copy link
Contributor

lurch commented Sep 2, 2021

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
Copy link
Contributor

lurch commented Sep 2, 2021

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

This appears to be fixed now 👍 (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
Copy link
Contributor

lurch commented Sep 2, 2021

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? 🤷

@lurch
Copy link
Contributor

lurch commented Sep 2, 2021

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
Copy link
Contributor

lurch commented Sep 2, 2021

$ ./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
Copy link
Contributor Author

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
Copy link
Contributor

lurch commented Sep 2, 2021

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. 👍

Copy link

@liamfraser liamfraser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like this has been discussed to death and LGTM so approving

@kilograham kilograham merged commit b1568b7 into develop Oct 21, 2021
@kilograham kilograham added this to the 1.1.0 milestone Oct 21, 2021
@kilograham kilograham deleted the reset branch October 21, 2021 20:00
@lurch lurch mentioned this pull request Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants