search

oseiler2 / CO2Monitor

https://oseiler2.github.io/CO2Monitor/
GNU General Public License v3.0
45 stars 5 forks source link

OTA Upgrade Improvements #5

Closed mattbnz closed 2 years ago

mattbnz commented 2 years ago

3 OTA improvements in this pull request:

1) Upgrades to a newer version of esp32FOTA that supports certificates on Filesystem (avoding the need to hardcode a cert into the repo) 2) Adds support for configuring the OTA URL via the configuration (both on disk and via MQTT) - agian the intent being to avoid needing to hardcode config paths into the repo 3) Implements "force OTA" support, so you can send an MQTT command to tell a device to upgrade to a specific firmware image - useful for remote development/testing.

(sorry, this pull request is a bit messy because it's from a branch off the LittleFS upgrade branch in my repo, but I don't know git/giithub well enough to figure out how to get that reflected here, so you see all the changes, not just the ones on top of the LittleFS branch...)

oseiler2 commented 2 years ago

Question: Is there much value in adding the OTA URL to the configuration? I'd have thought that it's rather static and sufficient to be specified at build time, especially with the additional option to forceOTA with providing a URL which should cover the need to overwrite any given URL. I'm trying to keep the configuration small to minimise memory footprint and also to keep the config portal UI simple (-ish - I know it's quite big already).

oseiler2 commented 2 years ago

It would be much nicer if we could avoid linking to this branch https://github.com/thorrak/esp32FOTA/tree/multi_filesystem and instead try to get this PR merged https://github.com/chrisjoyce911/esp32FOTA/pull/79

mattbnz commented 2 years ago

I don't like build parameters that can't be committed to the repo was why I made it a config option initially, but you're right that now that there's also the forceota method, it's hard to see a justification for this.

Thinking about my use-cases going forward, I can't see myself wanting devices to poll for OTAs, I'd much rather co-ordinate and trigger that centrally.

So yeah, I'd be happy to remove that code if you like.

oseiler2 commented 2 years ago

Cool. No need to do anything, I've almost got it working in a local branch, that saves you resolving the merge conflicts.

oseiler2 commented 2 years ago

See https://github.com/oseiler2/CO2Monitor/commit/20720971347865a6a7b4c3083d3479f7acc53ed9

mattbnz commented 2 years ago

I had to merge this anyway to pick up your changes so no drama.

There's just two little diffs left:

Thanks.

mattbnz commented 2 years ago

actually, I see you added root_ca.pem to .gitignore already, which confirms my (2) question.

I'll delete this, and you can take card of the header prototype direclty if you think important.

I'm going to blow away my forked repo and restart now that I've got a better idea how to work with github's workflow so it's not so messy in future...

oseiler2 commented 2 years ago

I'm currently not using Let's Encrypt for my OTA server, but I take your point it would be nice to have a default cert bundle that just works. Maybe something along the lines of this? https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/protocols/esp_crt_bundle.html

mattbnz commented 2 years ago

Ah, that's a neat set of certs, that would be fine for me, but I don't think it's a big deal either way, I can easily push a cert in my provisioning script, but if you have other users wanting a more plug and play experience, having some default certs loaded would be good.