Closed 2opremio closed 1 month ago
This is a counterpart to https://github.com/espressif/esp-idf-monitor/pull/15
CC @sudeep-mohanty who I am sure will appreciate the feaure when developing ULP code :)
Messages | |
---|---|
:book: | π Good Job! All checks are passing! |
π Hello 2opremio, we appreciate your contribution to this project!
This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.
DangerJS is triggered with each push
event to a Pull Request and modify the contents of this comment.
Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.
We do welcome contributions in the form of bug reports, feature requests and pull requests via this public GitHub repository.
This GitHub project is public mirror of our internal git 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 synchronize it into our internal git repository.
4. In the internal git repository 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.
5. If the change is approved and passes the tests it is merged into the default branch.
5. On next sync from the internal git repository merged change will appear in this public GitHub repository.
Generated by :no_entry_sign: dangerJS against 940e1dc4cdbf03581a2b0127e5370220304b0477
CC @sudeep-mohanty who I am sure will appreciate the feaure when developing ULP code :)
Thanks for the contribution @2opremio! Seems like a useful feature! But, I'd let our colleagues from the esptool team to further evaluate the changes. Thanks! :)
I am not sure I understand the linux-arm32
error on CI but it seems it's also failing in master
I am away this week, so, feel free to address the comments yourselves. Otherwise I will do so next week once I am back.
On August 5, 2024, GitHub @.***> wrote:
@peterdragun commented on this pull request.
Hi, first of all, thank you for your contribution, and I am sorry for the late response.
We have discussed this with the team and we think that this would be a nice addition to esptool. Overall we like the feature, but I have left some comments regarding the implementation.
Please let me know if you want to address them, if not we will do it by ourselves.
Thank you!
In docs/en/esptool/configuration-file.rst https://github.com/espressif/esptool/pull/995#discussion_r1703926561:
@@ -109,6 +109,8 @@ Complete list configurable options: +------------------------------ +----------------------------------------------------------- +----------+ | custom_reset_sequence | Custom reset sequence for resetting into the bootloader | | +------------------------------ +----------------------------------------------------------- +----------+ +| retry_open_serial | Retry opening the serial port indefinitely | False | Please fix the indentation to match the rest of the table
In esptool/init.py https://github.com/espressif/esptool/pull/995#discussion_r1703930400:
- parser.add_argument( + "--retry-open-serial", + help=( + "Retry opening the serial port indefinitely. " + "Default: %s" % DEFAULT_RETRY_OPEN_SERIAL + ), + default=os.environ.get("ESPTOOL_RETRY_OPEN_SERIAL", DEFAULT_RETRY_OPEN_SERIAL), + action="store_true", + ) We are trying to keep the number of arguments for esptool as slim as possible. It should be sufficient to configure this function using ENV and config variables.
In esptool/init.py https://github.com/espressif/esptool/pull/995#discussion_r1703938427:
- _esp = get_default_specific_connected_device( + chip, + each_port,
- initial_baud, + trace, + before, + connect_attempts, + retry_open_serial, + ) I think it only makes sense to run the retry open if the user has specified the port. In this case, it will also trigger on the auto- detected port.
In esptool/init.py https://github.com/espressif/esptool/pull/995#discussion_r1703939222:
- ) as e: + if not retry_open_serial: + raise + if _esp and _esp._port: + _esp._port.close() + _esp = None + if retry_attempts == 0: + print(e) + print("Retrying failed connection ", end="", flush=True) + else: + if retry_attempts % 9 == 0: + # print a dot every second + print(".", end="", flush=True) + time.sleep(0.1) + retry_attempts += 1 + continue this continue is not necessary
β Reply to this email directly, view it on GitHub https://github.com/espressif/esptool/pull/995#pullrequestreview- 2218676394, or unsubscribe https://github.com/notifications/unsubscribe- auth/AASA4JDD7Q35K52GJ7XXKEDZP5NADAVCNFSM6AAAAABK7TMW2SVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEMJYGY3TMMZZGQ. You are receiving this because you were mentioned.Message ID: @.***>
@peterdragun PTAL.
I've rebased the pre-existing code into a single commit and addressed all the comments (into a separate commit to differentiate it).
esptool
frequently fails when trying to open the serial port of a device which deep-sleeps often:This makes developers add unnecessarily long sleepswhen the main CPU is awake, in order to give
esptool
the chance to find the serial port.This PR adds a new flag
--retry-open-serial
(with corresponding env variable and cfg file entry) which retries opening the port indefinitely until the device shows up:I have tested this change with the following hardware & software combinations:
ESP32S3 + esptool master branch
I have run the esptool.py automated integration tests with this change and the above hardware:
"NO TESTING" (I can do so if considered necessary in order to merge)
I did test the flag, the env variable and the config file entry though.