feat(esp-idf-monitor): Add --retry-open flag (IDFGH-13270)

[2opremio]

esp-idf-monitor frequently fails when trying to open the serial port of a device which deep-sleeps often.

e.g.:

# idf.py monitor -p /dev/cu.usbmodem6101 
--- esp-idf-monitor 1.4.0 on /dev/cu.usbmodem6101 115200 ---
--- Quit: Ctrl+] | Menu: Ctrl+T | Help: Ctrl+T followed by Ctrl+H ---
[Errno 2] could not open port /dev/cu.usbmodem6101: [Errno 2] No such file or directory: '/dev/cu.usbmodem6101'
Connection to /dev/cu.usbmodem6101 failed. Available ports:
/dev/cu.usbmodemF554393F21402
/dev/cu.AspergitosII
/dev/cu.Bluetooth-Incoming-Port

esp-idf-monitor currently already waits for the serial port if it was opened successfully. This, however, doesn't help when opening the device for the first time.

This makes developers add unnecessarily long sleeps when the main CPU is awake, in order to give esp-idf-monitor the chance to find the serial port.

This PR adds a new flag --retry-open which retries opening the port indefinitely until the device shows up:

# idf.py monitor -p /dev/cu.usbmodem6101  --retry-open
--- esp-idf-monitor 1.4.0 on /dev/cu.usbmodem6101 115200 ---
--- Quit: Ctrl+] | Menu: Ctrl+T | Help: Ctrl+T followed by Ctrl+H ---
[Errno 2] could not open port /dev/cu.usbmodem6101: [Errno 2] No such file or directory: '/dev/cu.usbmodem6101'
Retrying to open port ......
ESP-ROM:esp32s3-20210327
rst:0x15 (USB_UART_CHIP_RESET),boot:0x8 (SPI_FAST_FLASH_BOOT)
Saved PC:0x4037b516
[...]

[github-actions]

	Messages
📖	Good Job! All checks are passing!

---

👋 Welcome 2opremio, thank you for your first contribution to espressif/esp-idf-monitor project!

📘 Please check project Contributions Guide of the project for the contribution checklist, information regarding code and documentation style, testing and other topics.

Pull request review and merge process you can expect

We do welcome contributions in the form of bug reports, feature requests and pull requests via this public GitHub repository.

1. An internal issue has been created for the PR, we assign it to the relevant engineer
2. They review the PR and either approve it or ask you for changes or clarifications
3. Once the Github PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing
   • At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch

---

Generated by 🚫 dangerJS against caa5c82

[2opremio]

I edited the commit message so that it conforms the pre-commit hook (the PR description is slightly longer to help explaining what it does)

[2opremio]

CC @sudeep-mohanty who I am sure will appreciate the feaure when developing ULP code :)

[2opremio]

BTW, we should probably set ESPTOOL_RETRY_OPEN_SERIAL accordingly after/if espressif/esptool#995 is merged, just like it is done with ESPPORT:

esp-idf-monitor/esp_idf_monitor/idf_monitor.py

Line 400 in d49533f

os.environ.update({ESPPORT_ENVIRON: espport_val})

Happy to do that right away if you think it makes sense.

(It will also require to change serial_ext.py in the idf-esp repo)

[2opremio]

I ended up setting ESPTOOL_RETRY_OPEN_SERIAL based on --retry-open so that we can invoke esptool properly through idf.py.

This is needed in order to properly invoke esptool with --retry-open-serial through idf.py (I will create a patch in esp-idf later on)

[fhrbata]

Hello,

I'm wondering if it would be possible to re-use the current re-connect logic.

diff --git a/esp_idf_monitor/base/argument_parser.py b/esp_idf_monitor/base/argument_parser.py
index 0ec426210bdb..4ce780afba44 100644
--- a/esp_idf_monitor/base/argument_parser.py
+++ b/esp_idf_monitor/base/argument_parser.py
@@ -139,4 +139,10 @@ def get_parser():  # type: () -> argparse.ArgumentParser
         default=False,
         action='store_true')
 
+    parser.add_argument(
+        '--retry-open',
+        help='Indefinitely retry opening the port device for the first time',
+        default=False,
+        action='store_true')
+
     return parser
diff --git a/esp_idf_monitor/base/serial_reader.py b/esp_idf_monitor/base/serial_reader.py
index a49d02dfbe37..cbe913beffd9 100644
--- a/esp_idf_monitor/base/serial_reader.py
+++ b/esp_idf_monitor/base/serial_reader.py
@@ -25,14 +25,15 @@ class SerialReader(Reader):
     event queue, until stopped.
     """
 
-    def __init__(self, serial_instance, event_queue, reset, target):
-        #  type: (serial.Serial, queue.Queue, bool, str) -> None
+    def __init__(self, serial_instance, event_queue, reset, retry_open, target):
+        #  type: (serial.Serial, queue.Queue, bool, bool, str) -> None
         super(SerialReader, self).__init__()
         self.baud = serial_instance.baudrate
         self.serial = serial_instance
         self.event_queue = event_queue
         self.gdb_exit = False
         self.reset = reset
+        self.retry_open = retry_open
         self.reset_strategy = Reset(serial_instance, target)
         if not hasattr(self.serial, 'cancel_read'):
             # enable timeout for checking alive flag,
@@ -46,30 +47,40 @@ class SerialReader(Reader):
             try:
                 # We can come to this thread at startup or from external application line GDB.
                 # If we come from GDB we would like to continue to run without reset.
-                self.open_serial(reset=not self.gdb_exit and self.reset)
+                self.reset = not self.gdb_exit and self.reset
+                self.open_serial(reset=self.reset)
+                # Successfully connected, so any further reconnections should occur without a reset.
+                self.reset = False
             except serial.SerialException as e:
                 print(e)
-                # if connection to port fails suggest other available ports
-                port_list = '\n'.join([p.device for p in list_ports.comports()])
-                yellow_print(f'Connection to {self.serial.portstr} failed. Available ports:\n{port_list}')
-                return
+                if not self.retry_open:
+                    # If the connection to the port fails and --retry-open was not specified,
+                    # recommend other available ports and exit.
+                    port_list = '\n'.join([p.device for p in list_ports.comports()])
+                    yellow_print(f'Connection to {self.serial.portstr} failed. Available ports:\n{port_list}')
+                    return
             self.gdb_exit = False
         try:
             while self.alive:
                 try:
-                    data = self.serial.read(self.serial.in_waiting or 1)
+                    if self.serial.is_open:
+                        # in_waiting assumes the port is already open
+                        data = self.serial.read(self.serial.in_waiting or 1)
+                    else:
+                        raise serial.PortNotOpenError
                 except (serial.SerialException, IOError) as e:
                     data = b''
                     # self.serial.open() was successful before, therefore, this is an issue related to
                     # the disappearance of the device
-                    red_print(e.strerror)
+                    red_print(str(e))
                     yellow_print('Waiting for the device to reconnect', newline='')
                     self.close_serial()
                     while self.alive:  # so that exiting monitor works while waiting
                         try:
                             time.sleep(RECONNECT_DELAY)
                             # reset on reconnect can be unexpected for wakeup from deepsleep using JTAG
-                            self.open_serial(reset=False)
+                            self.open_serial(reset=self.reset)
+                            self.reset = False
                             break  # device connected
                         except serial.SerialException:
                             yellow_print('.', newline='')
diff --git a/esp_idf_monitor/idf_monitor.py b/esp_idf_monitor/idf_monitor.py
index 9d6b35938591..d97e4cb5f550 100644
--- a/esp_idf_monitor/idf_monitor.py
+++ b/esp_idf_monitor/idf_monitor.py
@@ -90,6 +90,7 @@ class Monitor:
         make='make',  # type: str
         encrypted=False,  # type: bool
         reset=DEFAULT_TARGET_RESET,  # type: bool
+        retry_open=False,
         toolchain_prefix=DEFAULT_TOOLCHAIN_PREFIX,  # type: str
         eol='CRLF',  # type: str
         decode_coredumps=COREDUMP_DECODE_INFO,  # type: str
@@ -128,7 +129,7 @@ class Monitor:
             # testing hook: when running tests, input from console is ignored
             socket_test_mode = os.environ.get('ESP_IDF_MONITOR_TEST') == '1'
             self.serial = serial_instance
-            self.serial_reader = SerialReader(self.serial, self.event_queue, reset, target)  # type: Reader
+            self.serial_reader = SerialReader(self.serial, self.event_queue, reset, retry_open, target)  # type: Reader
 
             self.gdb_helper = GDBHelper(toolchain_prefix, websocket_client, self.elf_file, self.serial.port,
                                         self.serial.baudrate) if self.elf_exists else None
@@ -408,6 +409,7 @@ def main() -> None:
                       args.make,
                       args.encrypted,
                       not args.no_reset,
+                      args.retry_open,
                       args.toolchain_prefix,
                       args.eol,
                       args.decode_coredumps,

The passing of retry option for the cmake target (e.g., esptool) is not included. Additionally, if the reset logic only applies to the initial connection, the gdb_exit test might be unnecessary. This is just a preliminary idea, so feel free to ignore.

Thank you

[2opremio]

I'm wondering if it would be possible to re-use the current re-connect logic.

Thanks, I will take a look at the code and see if I can reuse it.

The passing of retry option for the cmake target (e.g., esptool) is not included.

Do you mean e894d66 ? (I think I may have forgotten to push it yesterday)

[fhrbata]

Hello,

I'm wondering if it would be possible to re-use the current re-connect logic.

Thanks, I will take a look at the code and see if I can reuse it.

As you stated in your original report

esp-idf-monitor currently already waits for the serial port if it was opened successfully. This, however, doesn't help when opening the device for the first time.

This made me wonder if we can reuse it. I might have overlooked something, so please take it as just an idea. Others who are more familiar with the monitor might have different opinion.

The passing of retry option for the cmake target (e.g., esptool) is not included.

Do you mean e894d66 ? (I think I may have forgotten to push it yesterday)

Yes, but I meant that I did not include this change in my patch I posted above. There is nothing missing in your PR. Sorry for the confusion, and thank you very much for looking into this!

[2opremio]

@fhrbata Ah, gotcha. Your code was much cleaner though

[2opremio]

I have also decreased the period with which we check the port to 100ms (while still keeping the screen feedback to 1 dot every 500ms)

This will greatly increase the chance of catching more logs in devices which deep-sleep.

[fhrbata]

I have also increased the period with which we check the port to 100ms (while still keeping the screen feedback to 1 dot every 500ms)

This will greatly increase the chance of catching more logs in devices which deep-sleep.

Thank you very much! I'm tagging @peterdragun, who is responsible for the monitor. Peter, what are your thoughts?

[peterdragun]

@2opremio Thank you for your contribution! Overall the changes look good to me.

Can you please squash the commits so the git history will be cleaner?

[2opremio]

@peterdragun Done! (Although I am sure you know it can be done a merge-time on GitHub)

[peterdragun]

Thank you!

(Although I am sure you know it can be done a merge-time on GitHub)

yes, you are right, but it helps our internal process if the PR author did the squash and rebase. Also, I would add myself as a co-author and "steal" the credits.

[2opremio]

Fair enough :)

[2opremio]

@peterdragun I added a minor correction (I think it was a preexisting problem) by extending the exception capturing. Do you want me to squash it?

[peterdragun]

I added a minor correction (I think it was a preexisting problem) by extending the exception capturing. Do you want me to squash it?

It is okay to keep it as a separate commit. Thank you.

Just a clarification from the PR description:

This makes developers add unnecessarily long sleeps when the main CPU is awake, in order to give esp-idf-monitor the chance to find the serial port.

I don't think this is true and also I don't think this PR will help to remove such waits. The wait is there mainly for USB JTAG/Serial, the reason why it was necessary to add it is because unlike with a USB-to-UART bridge, the port will disappear and it takes some time for the OS to notice the port. AFAIK, the main purpose is not to allow users to catch this moment, but rather for the already running monitor to establish the connection. The only part that can help is the reduced RECONNECT_DELAY time.

[2opremio]

I don't think this is true and also I don't think this PR will help to remove such waits. The wait is there mainly for USB JTAG/Serial, the reason why it was necessary to add it is because unlike with a USB-to-UART bridge, the port will disappear and it takes some time for the OS to notice the port. AFAIK, the main purpose is not to allow users to catch this moment, but rather for the already running monitor to establish the connection. The only part that can help is the reduced RECONNECT_DELAY time.

What I meant is that, without indefinite retries, you had to wait longer not just for the OS to show the device, but to give you enough to "catch" the device when running the monitor command for the first time.

Without a long delay (before this PR) the chances of starting up the monitor command right when the device is awake is much lower.

[peterdragun]

Hi @2opremio, I am sorry that it is taking so long, but I have one last request for you. Can you please remove the dependency on Esptool PR (the env variable part)? That way we won't rely on the esptool PR getting merged and we would unblock this PR.

We will reconsider the connection between the tools in the future and maybe we will get it back once the esptool PR gets merged.

Thank you for your cooperation and contribution!

[2opremio]

Sure thing, but it is not really a dependency per se (in the sense that the env variable can simply be ignored if not used).

However, I understand you don't want to have a dangling env variable which may not be used.

[2opremio]

I have removed the env variable but please note it's impossible to have a complete system which retries opening (i.e. esptool) without it.

I myself am running a patched version of esptool locally which doesn't work without the env variable.

[2opremio]

What's holding this PR back? Is it just the conflicts? (It's been ready for a while)

[2opremio]

I've just rebased and fixed the conflicts

[peterdragun]

What's holding this PR back?

I am sorry that it takes so long. The delay was mainly caused by the vacation season. We have also decided that we will wait for the esptool PR to merge in the end. We will get this merged soon, but there will probably be some small changes compared to your solution.

Thank you for being so patient.