-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Move Updater from core to libraries. #6393
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
Conversation
Caveat: ESP.updateSketch() becomes Update.updateSketch(). Quick Google research indicates that last mentions of use of updateSketch() are 4 years old.
I looked at doing this last night and gave up - it's not as simple as you may at first think. Largely because of this: ESP8266WiFi depends on Updater.h (at leas the BearSSL does). The two appear to be interwoven in a rather recursive way. |
libraries/Updater/library.properties
Outdated
@@ -0,0 +1,10 @@ | |||
name=Updater | |||
version=1.0 | |||
author= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
OK - but library to library dependencies are fine, or are they? |
They are fine, but relative paths like that aren't great. However in this instance they are kind of needed. What you want to avoid is every sketch that uses ESP8266WiFi.h also having to manually include Updater.h. It's less of an issue now with arduino-builder doing a better job of finding the right libraries to include recursively, but not everyone uses arduino-builder, and it's still fairly needed to manually specify any libraries required by libraries within your sketch (including SPI.h when you use SD.h for example) and for a library like ESP8266WiFi that would be just silly - especially as really the Updater has nothing at all to do with the WiFi library - except that BearSSL has some hashing functionality that ties in with the Updater. The problem of A needing B and B needing A is really the core of this issue. As long as all that is needed is the header file for the class structures and it doesn't rely on the code of the library being compiled then it's not too much of an issue, but you should keep the path as flat as possible. A better path would be |
I would suggest a much smaller, compatible change: core/Updater.cpp only references an extern of a class that it knows about, no need to include anything from BSSL in that case. |
After looking at the code a bit, I think this PR opens a much larger can of worms. Thanks for the proposal, but I think this is not the way forward. |
@earlephilhower In doc/ota_updates/readme.rst, there's a description that it's done in sketch code. A bit confusing. |
I'm not sure where you're seeing that. Could you link to the line? The magic w/signing was that it's done w/o any changes to user code. Plop in the cert and key, and you'll get a cryptographically signed XXX.bin.signed file out, and the internal updater will then only accept updates signed w/that private key. |
Here: https://github.com/esp8266/Arduino/blob/master/doc/ota_updates/readme.rst#manual-signing-binaries Is this what you have in mind? Shall I make a PR off it? |
Thanks for the doc pointer. The Linux/Mac-only is also wrong, it works under Windows, too, assuming you have openssl.exe in the path. The doc was written before the automatic IDE handling of signing was done, I think, and definitely before the inclusion of Python under Windows (since the signing is under control of a Python script). The branch, that's basically what I meant, but missing a couple things. Need to add #include <updater_signing.h> (since the parsed key and flag are defined in there). Also need to add the ifndef arduino_signing -> define arduino_signing 0 bit, too. Other build systems may not define the flag, so we try and be safe instead of sorry. The namespace gymnastics you've done, I don't understand the purpose. @devyte's the master of all things namespace or template, so I'll let him chime in when he gets a chance. |
@earlephilhower @devyte I've a new PR #6398 that implements our discussion results. |
Caveat: ESP.updateSketch() becomes Update.updateSketch(). Quick Google research indicates
that last mentions of use of updateSketch() are 4 years old.
Fixes Issue #6389 AFAIK (grep, find). Pending CI - there'll be a few things to do for host builds, I suspect :-)