Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add idf.py monitor argument --no-reset (-R) (IDFGH-7189) #8788

Merged
merged 1 commit into from
May 15, 2022
Merged

Add idf.py monitor argument --no-reset (-R) (IDFGH-7189) #8788

merged 1 commit into from
May 15, 2022

Conversation

nonoo
Copy link
Contributor

@nonoo nonoo commented Apr 17, 2022

Add idf.py monitor argument --no-reset (-R) to be able to optionally prevent resetting the CPU on monitor startup.

@espressif-bot espressif-bot added the Status: Opened Issue is new label Apr 17, 2022
@github-actions github-actions bot changed the title Add idf.py monitor argument --no-reset (-R) Add idf.py monitor argument --no-reset (-R) (IDFGH-7189) Apr 17, 2022
@Alvin1Zhang
Copy link
Collaborator

Thanks for your contribution.

@dobairoland dobairoland linked an issue Apr 19, 2022 that may be closed by this pull request
idf_monitor.py: allow monitor without resetting target (IDFGH-5963) #7651
Closed
@espressif-bot espressif-bot added Status: In Progress Work is in progress and removed Status: Opened Issue is new labels May 3, 2022
@dobairoland
Copy link
Collaborator

Hi @nonoo. https://github.com/espressif/esp-idf/runs/6052952029?check_suite_focus=true is failing. Can you please set up the pre-commit hooks locally (https://docs.espressif.com/projects/esp-idf/en/latest/esp32/contribute/install-pre-commit-hook.html) and fix the failures.

dobairoland
dobairoland requested changes May 3, 2022
View reviewed changes
Copy link
Collaborator

@dobairoland dobairoland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. Looks good to me in general.

tools/idf_monitor_base/argument_parser.py Outdated Show resolved Hide resolved
tools/idf_py_actions/serial_ext.py Outdated Show resolved Hide resolved
tools/idf_monitor.py Outdated Show resolved Hide resolved
@nonoo
Copy link
Contributor Author

nonoo commented May 3, 2022

There's one caveat, if no --port argument is given, tools/idf_py_actions/serial_ext.py calls esptool.get_default_connected_device() which opens the serial port by setting the chip into reset state. This needs to be fixed in esptool.get_default_connected_device() (both DTR and RTS needs to be set high in the esptool Python module).

A temporary workaround for this could be that we require the user to define the --port argument when using --no-reset.

@dobairoland
Copy link
Collaborator

dobairoland commented May 3, 2022 •
edited
Loading

@nonoo

There's one caveat, if no --port argument is given, tools/idf_py_actions/serial_ext.py calls esptool.get_default_connected_device() which opens the serial port by setting the chip into reset state. This needs to be fixed in esptool.get_default_connected_device() (both DTR and RTS needs to be set high in the esptool Python module).

I don't think we could avoid that.

A temporary workaround for this could be that we require the user to define the --port argument when using --no-reset.

Yes, this looks good to me. Maybe don't need to "require" it. We could just print a warning that "--no-reset is ignored. Please specify the port with the --port argument in order to use this option".

@dobairoland
Copy link
Collaborator

One more thing I forgot to mention.

Please squash the commits into one. (https://www.git-tower.com/learn/git/faq/git-squash/)

@nonoo
Add idf.py monitor argument --no-reset (-R) to prevent resetting the …
9266a7c 
…MCU target on monitor startup

Add idf.py monitor argument --no-reset (-R) to prevent resetting the CPU on monitor startup

idf.py monitor: fix type signature

idf.py monitor: fix reset key shortcut when --no-reset (-R) argument is used

idf.py monitor: change --no-reset (-R) argument descriptions in help

idf.py monitor: simplify --no-reset (-R) argument checks

idf.py monitor: add warning if --no-reset is used, but --port is not given

idf.py monitor: ignore --no-reset if --port is not given
@nonoo nonoo force-pushed the idf-monitor-no-reset branch from 1bd6c8b to 9266a7c Compare May 3, 2022 12:56
@dobairoland dobairoland mentioned this pull request May 5, 2022
Closed
@dobairoland dobairoland linked an issue May 5, 2022 that may be closed by this pull request
ESP32-S3 RTC slow memory data is lost when using USB Serial/JTAG Controller Console (IDFGH-7301) #8889
Closed
@dobairoland
Copy link
Collaborator

Thanks for the corrections!

@dobairoland
Copy link
Collaborator

sha=9266a7c52e078389e36fb90f1fba905683db5eae

@dobairoland dobairoland added the PR-Sync-Merge Pull request sync as merge commit label May 5, 2022
@espressif-bot espressif-bot added Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: In Progress Work is in progress Resolution: NA Issue resolution is unavailable labels May 10, 2022
@espressif-bot espressif-bot merged commit 9266a7c into espressif:master May 15, 2022
espressif-bot pushed a commit that referenced this pull request May 15, 2022
Tools: Add --no-reset option for IDF Monitor in order to avoid resett…
c02c0cc 
…ing the chip target upon connection

Closes #8889

Closes IDFGH-7189, IDFGH-7301, IDFGH-5963

Closes #7651

Merges #8788
@nonoo nonoo mentioned this pull request May 16, 2022
Merged
espressif-bot pushed a commit to espressif/esp-idf-monitor that referenced this pull request Jan 16, 2023
Tools: Add --no-reset option for IDF Monitor in order to avoid resett…
ffa2107 
…ing the chip target upon connection

Closes espressif/esp-idf#8889

Closes IDFGH-7189, IDFGH-7301, IDFGH-5963

Closes espressif/esp-idf#7651

Merges espressif/esp-idf#8788
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dobairoland dobairoland dobairoland requested changes

Assignees
No one assigned
Labels
PR-Sync-Merge Pull request sync as merge commit Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Milestone
No milestone
4 participants
@nonoo @Alvin1Zhang @dobairoland @espressif-bot