search

raspberrypi / picotool

BSD 3-Clause "New" or "Revised" License
525 stars 86 forks source link

Add error message if --force fails #57

Open playduck opened 1 year ago

playduck commented 1 year ago

This PR is in response to issue #56.

Using -f or -F requires the RP2040 to have enabled USB-CDC. As this isn't immediately obvious this PR adds a short reminder if a load command with a force flag fails. This also adds a small sentence to the help load text.

Running ./picotool load binary.elf -f with no responding device now outputs:

No accessible RP2040 devices in BOOTSEL mode were found. To force a device into BOOTSEL mode, make sure the RP2040 is configured to use USB-CDC (USB stdio).

The help load text now reads (note the last sentence):

-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. Make sure the device is using USB-CDC (USB stdio)

I've also modified the the printing routine for the error message to use snprintf and strncpy to avoid potential string-length issues as is good practice.

lurch commented 1 year ago

this PR adds a short reminder if a load command with a force flag fails.

It actually appears to be any command that uses the force flag, not just the load command? IMHO the behaviour you've implemented is what I'd expect, it's just that the PR description is a bit misleading :wink:

playduck commented 1 year ago

Added both missing dashes, must've missed them the first time around.

It actually appears to be any command that uses the force flag

Yes, I've just been testing it with load only, but other commands behave the same:

./picotool info -f
No accessible RP2040 devices in BOOTSEL mode were found.
To force a device into BOOTSEL mode, make sure the RP2040 is configured to use USB-CDC (USB stdio).
playduck commented 1 year ago

I subtracted the current string length from the maximum allowed length of the snprintf calls. This still produces the same output but is now more robust against buffer-overflows.

kilograham commented 1 year ago

it isn't strictly limited to USB stdio (though may be in practice as no one else implements the interface)

Perhaps "Force a device not in BOOTSEL mode but running compatible code (e.g. USB stdio)"

lurch commented 1 year ago

You probably want to change this to be based against develop instead of master? And whilst doing that, be aware of the change introduced in #72 :wink: