espressif / esptool

Espressif SoC serial bootloader utility
https://docs.espressif.com/projects/esptool
GNU General Public License v2.0
5.6k stars 1.39k forks source link

add hard reset to rfc2217 server (ESPTOOL-760) #929

Closed 20162026 closed 1 year ago

20162026 commented 1 year ago

add hard reset functionality to rfc2217 server and modify bootloader reset to trigger on DTR_ON instead of RTS_ON. This modification allows to use a rfc2217 server with idf_monitor and other compatible serial monitoring tools. I'm not a python or rfc2217 expert and was assuming the default reset sequence, so there could be some issues with custom sequences.

This change fixes the following bug(s):

892

I have tested this change with the following hardware & software combinations:

I have tested esp_rfc2217_server.py on windows10 and Ubuntu 22.04.2 LTS using custom esp32-wroom-32 board with the usual automatic bootloader circuit.

I have run the esptool.py automated integration tests with this change and the above hardware:

NO TESTING

github-actions[bot] commented 1 year ago
Messages
:book: Good Job! All checks are passing!

👋 Welcome 20162026, thank you for your first contribution to espressif/esptool 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 :no_entry_sign: dangerJS against d66de5ce839b4014327e21be22d1a429a1f77320

radimkarnis commented 1 year ago

Hi @20162026, thank you for your contribution! This is a very welcome addition.

I have synced the PR to our internal queue and will be testing the change soon. After a first look, the code LGTM!

I would like to ask you to do some stylistic changes: 1) The flake8 linter is failing, please see the failing GH Actions checks below. 2) Please change the commit message according to the conventional commits standard. Something like feat(rfc2217_server): Add hard reset sequence could work.

Thank you!

20162026 commented 1 year ago

hi @radimkarnis, stylistic changes are done.

radimkarnis commented 1 year ago

@20162026 thank you! Sorry to bother, but there is still a small change to make reported by the black formater. Sorry for this pedantic styling, esptool follows a certain code style and the commits cannot be merged until the code complies. I would make the changes myself, but then you'd lose a GitHub contributor credit, because the commit would be altered by me.

20162026 commented 1 year ago

@radimkarnis fixed. Im not quite sure why black did not catch error when i was commiting localy as I did use the precommit hook, however now I ran linters manually and it should be ok

dobairoland commented 1 year ago

@radimkarnis I've noticed that the link to the project contribution guide is invalid in https://github.com/espressif/esptool/pull/929#issuecomment-1781550930.

dobairoland commented 1 year ago

Hi @20162026. Thank you for your contribution. We will be happy to receive any feedback regarding the contribution process. Maybe we can something improve in our guides?

radimkarnis commented 1 year ago

@20162026 thank you again! Now merged to master. This will propagate to ESP-IDF for use with other tools with the next esptool release.