Add error message if --force fails #57

playduck · 2022-08-31T11:49:24Z

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.

- changed the -f and -F help texts - added an error message if a force load failed

lurch · 2022-08-31T12:08:49Z

README.md

-            without the RPI-RP2 drive mounted
+            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. Make sure the device is using USB CDC (USB stdio)


Missing hyphen in USB-CDC here?

No hyphen is needed (example) and I don't think I've ever seen it written with a hyphen. I usually write it as "USB CDC ACM".

lurch · 2022-08-31T12:09:29Z

README.md

-            the command (unless the command itself is a 'reboot') the device will be rebooted back to application mode
+            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)


Missing hyphen in USB-CDC here?

lurch · 2022-08-31T12:12:51Z

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 😉

playduck · 2022-08-31T12:26:21Z

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

lurch · 2022-08-31T12:41:29Z

main.cpp

        }
    }
+    if (settings.force) {
+        char* bufForceError = b + strnlen(b, bufferLen);
+        snprintf(bufForceError, bufferLen, "\nTo force a device into BOOTSEL mode, make sure the RP2040 is configured to use USB-CDC (USB stdio).");


If you're using snprintf to prevent string-overflows, do you also need to pass in a smaller value than bufferLen if the string b already contains some content?

True, the same holds for the bufErrorMessage strings above.

The length argument should be something along the lines of bufferLen - strnlen(b, bufferLen). I'm not sure if this causes an off-by-one error regarding the null terminator (e.g. should it be bufferLen - strnlen(b, bufferLen) - 1 or not)?

I'm not sure if this causes an off-by-one error

I dunno either 🤷‍♂️ I'm more of a Python programmer than a C programmer 😉

playduck · 2022-09-01T12:40:24Z

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.

lurch · 2022-09-01T13:27:40Z

main.cpp

        }
    } else {
        if (settings.bus != -1) {
-            sprintf(buf, "accessible RP2040 devices in BOOTSEL mode were found found on bus %d.", settings.bus);
+            snprintf(bufErrorMessage, bufferLen - currentLength, "accessible RP2040 devices in BOOTSEL mode were found found on bus %d.", settings.bus);


Haha, I just noticed that this says "found found" (even in the original) 😆
Would you mind fixing that at the same time please?

Oh wow, my line-wrapping broke the line right between the two and I didn't notice it either.
It's fixed now.

kilograham · 2023-02-09T16:46:56Z

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

merge conflicts were only in main.cpp. resolved by accepting all incoming changes and modifying the newly added error message in line 1607 to conform to all other messages no other conflicts or changes

lurch · 2023-02-19T23:10:31Z

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 😉

lurch · 2023-02-19T23:19:06Z

main.cpp

        }
    }
+    if (settings.force) {
+        snprintf(buf, buf_len, "\nTo force a device into BOOTSEL mode, make sure the RP2040 is configured to use USB-CDC (USB stdio).");


I think you need to recalculate buf and buf_len, otherwise you'll just be overwriting the previous text rather than appending to it?

playduck added 2 commits August 31, 2022 13:37

added additional info and error message for --force option

515717a

- changed the -f and -F help texts - added an error message if a force load failed

added missing dash in USB-CDC string

88ae308

lurch reviewed Aug 31, 2022

View reviewed changes

added another missing dash in USB-CDC string

0510533

lurch reviewed Aug 31, 2022

View reviewed changes

adjusted snprintf buffer length to appropriate length

6acb4f8

lurch reviewed Sep 1, 2022

View reviewed changes

removed redundant "found" in string

4800ccc

ernestguillem mentioned this pull request Nov 22, 2022

Warning on 'sprintf' while compiling. #60

Closed

Merge remote-tracking branch 'upstream/master'

e4374e7

merge conflicts were only in main.cpp. resolved by accepting all incoming changes and modifying the newly added error message in line 1607 to conform to all other messages no other conflicts or changes

lurch reviewed Feb 19, 2023

View reviewed changes

will-v-pi added this to the 1.6.1 milestone Jul 27, 2024

will-v-pi modified the milestones: 1.6.1, 1.7.0 Aug 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add error message if --force fails #57

Add error message if --force fails #57

playduck commented Aug 31, 2022

lurch Aug 31, 2022

DavidEGrayson Feb 8, 2023

lurch Aug 31, 2022

lurch commented Aug 31, 2022

playduck commented Aug 31, 2022

lurch Aug 31, 2022

playduck Aug 31, 2022

lurch Aug 31, 2022

playduck commented Sep 1, 2022

lurch Sep 1, 2022

playduck Sep 1, 2022

kilograham commented Feb 9, 2023 •

edited

Loading

lurch commented Feb 19, 2023

lurch Feb 19, 2023

Add error message if --force fails #57

Are you sure you want to change the base?

Add error message if --force fails #57

Conversation

playduck commented Aug 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lurch commented Aug 31, 2022

playduck commented Aug 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

playduck commented Sep 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kilograham commented Feb 9, 2023 • edited Loading

lurch commented Feb 19, 2023

Choose a reason for hiding this comment

kilograham commented Feb 9, 2023 •

edited

Loading