Skip to content

Add missing esptool upload modes for none and dtrset #4228

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

Merged
merged 5 commits into from
Jan 24, 2018
Merged

Add missing esptool upload modes for none and dtrset #4228

merged 5 commits into from
Jan 24, 2018

Conversation

mribble
Copy link
Contributor

@mribble mribble commented Jan 23, 2018

The boards.txt file already exposes different ways to upload binaries using esptool's "Reset Method". This just adds 2 reset methods (none and dtrset) exposed by that tool to the generic board files.

This page explains what those modes do: https://github.com./igrr/esptool-ck

@d-a-v
Copy link
Collaborator

d-a-v commented Jan 24, 2018

@mribble boards.txt is generated by the tools/boards.txt.py python script.
This is the one you should patch.

@mribble
Copy link
Contributor Author

mribble commented Jan 24, 2018

@d-a-v Sorry for the mistake. I should have read the comment at the boards.txt. I think I have made the correct change to tools/boards.txt.py and then generated boards.txt from it. Could you review this change now?

@d-a-v d-a-v merged commit 8a24598 into esp8266:master Jan 24, 2018
@d-a-v
Copy link
Collaborator

d-a-v commented Jan 24, 2018

@mribble I had not read this comment before merging.
I understand now that this option should only be used via boards.local.txt and would not appear in the default menu.
I'm afraid that this commit should be reverted.

@mribble
Copy link
Contributor Author

mribble commented Jan 24, 2018

@igrr or @d-a-v, I thought that boards.local.txt was a temporary solution. I would rather not have people having to locate the esp8266 directory and dropping in extra files there for my project.

It seems reasonable to me to expose these two extra reset options that esptool-ck already supports since that lets more boards work. The dtrset mode is the one I care around and I'm not the only one who would like it. This mode is how you can program the esp8266 through a sam3x (Arduino Due) chip. There are lots of people on the web asking how to do this, and I checked the solution into esptool-ck with help from igrr. It seems nice to put this functionality into the Arduino IDE when using the esp8266 boards.

If you disagree can someone explain the problem with adding the functionality in this pull request?

Thanks!

@igrr
Copy link
Member

igrr commented Jan 25, 2018

Is there a development board which needs this solution for uploading? Do the boards listed in this PR (espino, espresso lite, and so on) need this reset method in order to work?
The problem is that extra option adds clutter to the menu and confusion to users who will need to figure out what this option is for. I am pretty sure that the boards which got this reset option in this PR don't actually need it, and can work with nodemcu reset method.

@mribble
Copy link
Contributor Author

mribble commented Jan 25, 2018

Is there a development board which needs this solution for uploading?

There are shipping development boards that require the "none" solution (Digistump Oak). I thought it was wrong that there was a way to expose this option for specific boards, but it was not available to the general reset mode macro. That's why I none.

Let me give you a little background for drtset. Besides the esptool-ck that your project uses there is a python based esptool (https://github.com./espressif/esptool). It also has a version of "none" and it behaves as drtset. As we have discussed in other threads when I readed through definitions uart protocols and how drt is supposed to behave I actually think the drtset behavior is more correct way of implementing "none", but I do agree with you that it's too late to change behavior of your tool since that would risk breaking existing boards depending on existing behavior. My point here is that is I think if we expose "none" we should also expose "drtset" since it's the way usb->serial drivers expect DRT to behave.

Do the boards listed in this PR (espino, espresso lite, and so on) need this reset method in order to work?

I don't know what those boards require today. I actually find it odd they are using the generic upload macro instead of just setting it to a fixed mode that works for them (that's what most of the non-generic dev boards do). I wonder if they are using this multioption macro by mistake. This agrees with your comment saying:

I am pretty sure that the boards which got this reset option in this PR don't actually need it, and can work with nodemcu reset method.

If that's the case these dev boards should be forcing the reset method with "resetmethod_ck" and not using "resetmethod_menu". I would think only the general board solutions should be using resetmethod_menu unless these other boards really have a reason to expose multiple reset methods (perhaps different versions use different reset methods).

I am working on an unreleased board that does require dtrset though. It will be a board specifically targeting high speed photography. I have released previous versions without wifi support and have thousands of users of those boards (the project is called Camera Axe). I'm planning to launch a Kickstarter for this new wifi enabled Camera Axe in a few months. I have a private github repo with over 500 commits from several github accounts that contains all the CA software and kicad PCB files. I can share this private repo with you if you'd like to look over it. I will be making everything open source after launching the kickstarter. While I might make a tool that simplifies firmware updates, I would really like having as many people use the source to update firmware as possible. Having people update firmware using the source in the past has really helped get a lot more people contributing to my projects. To make this happen I need it easy for people to upload new versions through the Arduino IDE tools without lots of extra steps like copying a local boards.txt file to some directory most of these people would never have to know about.

The problem is that extra option adds clutter to the menu and confusion to users who will need to figure out what this option is for.

I agree with you on that and often fight for that position. However, I don't think the clutter is very bad in this case. You are adding 2 more options at the end of a submenu. Most users won't know what these options are and will just go with the default. If users look up what these options are having 2 more in the table doesn't add much more complexity. More likely if they need to change this the board they are using will have directions on what to set them to (that's what I'm planning to do for the Camera Axe).

Just as a side note. If you want to clean things up I think removing many of those dev boards listed would help a lot. It feels like that list could grow to the point of being pretty crazy. If the general boards listed all the configuration options then those specific dev boards in there instructions could just list a few of those general settings to change instead of having users have to look through 24 (and growing) dev boards with a very good possibility the board someone is using isn't in that list. If a board really wants it's own listing they could add their own boards.local.txt (perhaps add a better way to upload that -- for example if it could be in a sketch folder that would solve my complaints with board.local.txt, but I know that is hard because this file is read during init for the IDE). That said I am not an idealist and am willing to follow the conventions of the code I'm working with. If your project would rather have lots of boards I would be fine adding a Camera Axe board.

Sorry this got so long...

@d-a-v
Copy link
Collaborator

d-a-v commented Jan 25, 2018

I wonder if they are using this multioption macro by mistake.

About some of the boards having two reset methods, now four, it is the former boards.txt's legacy.
For each board, the generator provides the reset menu, or force one method with no choice. I followed what was in the former boards.txt (link to version 2.4.0rc2, check espino, espresso, they have both).

If the dtr reset method is to be left in the menu, I can arrange the generator for it to appear only in the generic boards so the others would have only the two methods as before.

I agree that this menu should appear only with the generic boards. I could not check myself which of the reset methods was the good one for those boards (espino, espresso, ...) so I had no other option than to leave it as it was.

Just as a side note. If you want to clean things up I think removing many of those dev boards listed would help a lot.

I believe the arduino IDE is aimed at beeing easy to use and help beginners to straightly go to the fun part. So having their board listed not only saves them from trying to understand everything from scratch, but also gives confidence with that it's going to work. Pretty much the same as your Camera Axe users that do not want to bother with reset configuration and are eager to play the game. I reckon that your board is not listed because it does not exist yet (still you have now the needed reset option), but who knows ?

@mribble
Copy link
Contributor Author

mribble commented Jan 25, 2018

I do feel the generic boards should list all the options. d-a-v if you want to make that change that would make me happy. I understand the code well enough to make that change if you are too busy, but will leave it to you unless you ask me to to make the change.

If you guys would rather try to clean this up I can look through git history and see if I can find the people who added this code to the original boards.txt file and see if they can confirm the intent. Or maybe I'd reach out to the owner of these boards on the web and see if they can confirm the reset method they want. That said, just doing what d-a-v suggested is the easiest and I guess I'd rather do that unless one you want me to try and track down original intent.

@d-a-v
Copy link
Collaborator

d-a-v commented Jan 25, 2018

I of course don't mind you update the code, thanks for the proposal :) either way, restricting dtr+none to both generic esp8266&esp8285 or cleaning up boards with reset menu (that would be the hard way).

@mribble
Copy link
Contributor Author

mribble commented Jan 25, 2018

I will write update the python code so it only updates generic esp8266&esp8285.

Can I do that with this same pull request, or do I need to create a new one since this one has been closed?

@d-a-v
Copy link
Collaborator

d-a-v commented Jan 25, 2018

Yes it is merged, so you need to make a new one.

@mribble
Copy link
Contributor Author

mribble commented Jan 25, 2018

This is continued in pull request #4241

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants