Minor Fixes (#67)

* Minor Fixes

* replace sprintf with snprintf
* allow connection to a device which previously had a failed command, by resetting on new connection
* very hacky fix/workaround for attempting to read strings near the end of the image
* update picotool load help for -u

README.md

@@ -57,7 +57,7 @@ PICOTOOL:
 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 [-n] [-N] [-v] [-x] <filename> [-t <type>] [-o <offset>] [--bus <bus>] [--address <addr>] [-f] [-F]
+    picotool load [-n] [-N] [-u] [-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>]

cli.h

@@ -88,7 +88,7 @@ namespace cli {
 
         void add(const string& major_group, const string& minor_group, const string& option, const string& description) {
             auto &v = contents[major_group][minor_group];
-            // we don't want to repeated the same option
+            // we don't want to repeat the same option
             if (std::find_if(v.begin(), v.end(), [&](const auto &x) { return x.first == option; }) == v.end()) {
                 v.emplace_back(option, description);
             }
@@ -301,6 +301,7 @@ namespace cli {
 
         vector<string> synopsys() const override {
             string s = string("<") + this->_name + ">";
+            if (this->_max > 1) s += "..";
             return {s};
         }
 

main.cpp

@@ -344,7 +344,7 @@ struct info_command : public cmd {
     string get_doc() const override {
          return "Display information from the target device(s) or file.\nWithout any arguments, this will display basic information for all connected RP2040 devices in BOOTSEL mode";
     }
-} info_cmd;
+};
 
 struct verify_command : public cmd {
     verify_command() : cmd("verify") {}
@@ -367,7 +367,7 @@ struct verify_command : public cmd {
     string get_doc() const override {
         return "Check that the device contents match those in the file.";
     }
-} verify_cmd;
+};
 
 struct save_command : public cmd {
     save_command() : cmd("save") {}
@@ -393,7 +393,7 @@ struct save_command : public cmd {
     string get_doc() const override {
         return "Save the program / memory stored in flash on the device to a file.";
     }
-} save_cmd;
+};
 
 struct load_command : public cmd {
     load_command() : cmd("load") {}
@@ -420,7 +420,7 @@ struct load_command : public cmd {
     string get_doc() const override {
         return "Load the program / memory range stored in a file onto the device.";
     }
-} load_cmd;
+};
 
 struct help_command : public cmd {
     help_command() : cmd("help") {}
@@ -439,7 +439,7 @@ struct help_command : public cmd {
     string get_doc() const override {
         return "Show general help or help for a specific command";
     }
-} help_cmd;
+};
 
 struct version_command : public cmd {
     version_command() : cmd("version") {}
@@ -464,7 +464,7 @@ struct version_command : public cmd {
     string get_doc() const override {
         return "Display picotool version";
     }
-} version_cmd;
+};
 
 struct reboot_command : public cmd {
     bool quiet;
@@ -993,15 +993,19 @@ bool find_binary_info(memory_access& access, binary_info_header &hdr) {
 }
 
 string read_string(memory_access &access, uint32_t addr) {
-    const uint max_length = 256; // todo better incremental length handling
-    auto v = access.read_vector<char>(addr, 256);
-    uint length;
-    for (length = 0; length < max_length; length++) {
-        if (!v[length]) {
-            break;
+    // note this implementation is still wrong, it just tries a bit harder to not try to read off the end of the image (which causes
+    // an assertion failure)
+    uint max_length;
+    for(max_length = 8; max_length <= 1024; max_length *=2 ) {
+        auto v = access.read_vector<char>(addr, max_length);
+        uint length;
+        for (length = 0; length < max_length; length++) {
+            if (!v[length]) {
+                return string(v.data(), length);
+            }
         }
     }
-    return string(v.data(), length);
+    return "<failed to read string>";
 }
 
 struct bi_visitor_base {
@@ -1573,17 +1577,18 @@ string missing_device_string(bool wasRetry) {
         strcpy(b, "No ");
     }
     char *buf = b + strlen(b);
+    int buf_len = b + sizeof(b) - buf;
     if (settings.address != -1) {
         if (settings.bus != -1) {
-            sprintf(buf, "accessible RP2040 device in BOOTSEL mode was found at bus %d, address %d.", settings.bus, settings.address);
+            snprintf(buf, buf_len, "accessible RP2040 device in BOOTSEL mode was found at bus %d, address %d.", settings.bus, settings.address);
         } else {
-            sprintf(buf, "accessible RP2040 devices in BOOTSEL mode were found with address %d.", settings.address);
+            snprintf(buf, buf_len, "accessible RP2040 devices in BOOTSEL mode were found with address %d.", settings.address);
         }
     } else {
         if (settings.bus != -1) {
-            sprintf(buf, "accessible RP2040 devices in BOOTSEL mode were found found on bus %d.", settings.bus);
+            snprintf(buf, buf_len, "accessible RP2040 devices in BOOTSEL mode were found found on bus %d.", settings.bus);
         } else {
-            sprintf(buf, "accessible RP2040 devices in BOOTSEL mode were found.");
+            snprintf(buf, buf_len,"accessible RP2040 devices in BOOTSEL mode were found.");
         }
     }
     return b;

picoboot_connection/picoboot_connection_cxx.h

@@ -28,11 +28,16 @@ namespace picoboot {
 
     struct connection {
         explicit connection(libusb_device_handle *device, bool exclusive = true) : device(device), exclusive(exclusive) {
+            // do a device reset in case it was left in a bad state
+            reset();
             if (exclusive) exclusive_access(EXCLUSIVE);
         }
         ~connection() {
             if (exclusive) {
-                picoboot_exclusive_access(device, NOT_EXCLUSIVE);
+                if (picoboot_exclusive_access(device, NOT_EXCLUSIVE)) {
+                    // failed to restore exclusive access, so just reset
+                    reset();
+                }
             }
         }
         void reset();