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.

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

Support multiple reset technologies in esptool (besides nodemcu) (ESPTOOL-164) #508

Closed
wants to merge 1 commit into from

Conversation

middelink
Copy link
Contributor

@middelink middelink commented Jan 24, 2020

Description of change

Split out bootloader reset logic from _connect_attempt() to bootloader_reset() to make esptool.py more Monkey Patch friendly in regard to alternative reset methods.

Also moved the hard_reset print statement into hard_reset itself, as different non-nodemcu reset methods might not use the RTS pin...

This change fixes the following bug(s):

#507

@projectgus
Copy link
Contributor

projectgus commented Feb 10, 2020

Hi @middelink,

Thanks for opening the issue and sending the PR.

To be honest, the reason that esptool.py hasn't added any other reset modes until now is that they seem so rarely used:

Similar to the explanation in #513, if these options aren't going to be used by anyone (and the code is going to be difficult or impossible to test) then my preference is to keep the support out of esptool.py. Although - as mentioned in the linked issue - maybe we can make it easier to edit esptool.py and "hack in" an alternative reset methods by editing some specific functions.

I'm happy to change my mind about merging this if you know of any popular or current boards that use these reset methods, though.

@middelink
Copy link
Contributor Author

Hi,

I did not search for an official Wifio board, I just happen to create my ESP projects with such an interface as it allows for an unmodified FTDI USB converter to program the ESP8266 (it has only one output (RTS) and not DTR, which the nodemcu method requires. Nor do I want to suggest everyone adds a uart to their project just to program it for the one or two times they need it...)

I took the ck method from an older esptoolpy fork (https://github.com/igrr/esptool-ck), so no hardware i can test with. Happy to remove it though.

The "patch" method is exactly what I used, adding "flash" and "app" reset methods to the two functions which need them. Also, I don't think that strategy is going to work for things like platformio: forking this tool just to add a single (patch) file and then redistributing it.

While I understand the principal grounds as to why you don't want to add extra (untestable for you) features, it also implies that this tool is seemingly solely espressif driven and not a community effort.
The criteria that a board has to be popular and current is just a fancy way of saying "no" IMO.

Oh well, thanks for looking at the PR and I guess I'll go back to plan B by patching PlatformIO to use esptool-ck for the all esp8266 environments

@projectgus
Copy link
Contributor

features, it also implies that this tool is seemingly solely espressif driven and not a community effort.

Usability and maintainability is both an Espressif concern and a community concern. I've been maintaining esptool since well before I worked at Espressif, and it didn't support alternative reset modes then either (noone has said they needed them until now).

The criteria that a board has to be popular and current is just a fancy way of saying "no" IMO.

It's a way of saying that the tool should be reliable and easy to use for the things that most of our users need to use it for. I'm afraid this means not adding new command line options, unless they're going to be used by more than one person each.

Every new option is something that every new user needs to read and understand whether it applies to them. Each new piece of functionality is something which we want to have some amount of test coverage for.

@projectgus
Copy link
Contributor

projectgus commented Feb 11, 2020

patching PlatformIO to use esptool-ck for the all esp8266 environments

FWIW, if this is for your own project then it's not terribly hard to either use your esptool.py, patch the local _connect_attempt() function, or to wrap esptool.py in a module which monkey-patches it.

Or maybe I'm missing the bigger picture for your goal, here?

@middelink
Copy link
Contributor Author

FWIW, if this is for your own project then it's not terribly hard to either use your esptool.py, patch the local _connect_attempt() function, or to wrap esptool.py in a module which monkey-patches it.

Hmm, that is not a bad idea, although I would have to come up with an way to extend the --before choices (as that is the only argument handed over to _connect_attempt() and hard_reset().

Ah, the script doing the monkey patching can parse its own arguments to fish out a new --reset_method flag and use that in the monkey patched reset routines, hopefully argparse.parse_known_flags() leaves enough of the original cmdline intact for main() to work.

Ok, would you be ok if I separate _connect_attempt()'s reset logic from its core logic and place it in bootloader_reset() (better name?). It would make monkey patching the resetting part of the tool easier.

@middelink
Copy link
Contributor Author

PTAL, PR adjusted

Copy link
Contributor

@projectgus projectgus left a comment

Choose a reason for hiding this comment

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

One small request, but this looks like a great solution - thanks.

esptool.py Outdated
@@ -435,6 +435,34 @@ def _setRTS(self, state):
# request is sent with the updated RTS state and the same DTR state
self._port.setDTR(self._port.dtr)

def _sendBreak(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

This private function isn't used any more, can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I need it in the one of reset methodologies and instead of digging through the innards of the esptool, i though it would be handier to just have this small function here. I can make it non-private though, if that helps, but in that case I would suggest the RTS and DTR function to become non-private too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest it makes sense for setDTR(), setRTS(), and port to be "private" (for Python's permissive definition of "private"), because the ESPROM class expects to manage the state of these things itself rather than have that state manipulated by a user of the ESPROM object.

If subclassing the ESPROM class (or patching) then you can still call these in the subclass (or the patch), with the implicit association that you're messing with the "internals" here and the parent class might also change this state sometimes.

Suggest it's also correct to add a function like _sendBreak() in that same external place, where it is needed and will be used.

@frippe75
Copy link

Not sure this is the place to add this suggestion. But allowing multiple ways of getting the ESP32 into programming mode would be really useful. I'm using the cp2104 from Silicon Labs. It actually has two dedicated GPIO pins that could be used to control RESET/GPIO0. Then the whole auto-reset circuitry could be removed lowering part count.

@radimkarnis radimkarnis changed the title Support multiple reset technologies in esptool (besides nodemcu) Support multiple reset technologies in esptool (besides nodemcu) (ESPTOOL-164) Dec 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support multiple reset technologies in esptool (besides nodemcu) (ESPTOOL-160)
3 participants