Skip to content

ports/esp32 add api support for ota updates - #3576

Closed
andynd wants to merge 11 commits into
micropython:masterfrom
andynd:ota
Closed

andynd wants to merge 11 commits into
micropython:masterfrom
andynd:ota

Conversation

@andynd

@andynd andynd commented Jan 21, 2018 •

Copy link
Copy Markdown

add the following APIs:
esp.partition_find_first
esp.ota_set_boot_partition
esp.ota_get_boot_partition
esp.ota_get_running_partition
esp.ota_get_next_update_partition

move the vfs to its own partition if it exists
use an ota capable partition layout

add the following APIs:
	esp.partition_find_first
	esp.ota_set_boot_partition
	esp.ota_get_boot_partition

move the vfs to its own partition if it exists
use an ota capable partition layout
@andynd andynd changed the title add support for ota updates for the esp32 port ports/esp32 add api support for ota updates Feb 4, 2018
@andynd

andynd commented Feb 27, 2018

Copy link
Copy Markdown
Author

Any plans to merge this? Pull request was created more than a month ago.

@ThomasWaldmann

ThomasWaldmann commented Mar 19, 2018 •

Copy link
Copy Markdown

Having OTA capability would be nice!

Do I understand it correctly that it would update the partition with micropython, but not the one with the filesystem?

Comment thread ports/esp32/modules/_boot.py Outdated
try:
if bdev:
uos.mount(bdev, '/')
print("Mounted /")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

indentation looks wrong. tab? one space missing?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

oh.. yes. thx Will fix it.

Comment thread ports/esp32/modules/flashbdev.py Outdated
if not offset:
self.START_SEC = esp.flash_user_start() // self.SEC_SIZE
else:
self.START_SEC = offset // self.SEC_SIZE

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

again: indentation looks wrong.

@andynd

andynd commented Mar 19, 2018

Copy link
Copy Markdown
Author

Do I understand it correctly that it would update the partition with micropython, but not the one with the filesystem?

It moves the fs to its own partition, right now micropython ignores the partition table and uses space after micropython. This would corrupt a 2nd partition. You can update the fs too, but you do not need any code in mp to do it, it can be done in pure python code so this pull doesn't contain any code for it

Is the flash big enough for this? How would a typical layout with mp and fs look like?

Depends on your esp32 module. Most I work with have 4mb flash, enought for two mp partitions with 1.7mb each and a 512kb fs partition. My folder struktur is two folders at / one named "factory" and one named "ota_0". My boot.py then moves in the folder named after the boot partition and executes the boot.py and main.py there. Like that i can update the fs too, using a downloaded tar.

@ThomasWaldmann

Copy link
Copy Markdown

Thanks for explaining. C&P that to some docs file?

@andynd

andynd commented Mar 25, 2018 •

Copy link
Copy Markdown
Author

Updated to current master, fixed indentation. I will try to get our internal ota updater written in python added to the docu, just have to get an ok for it first. What more is needed to merge this?

app_update was included multiple times
@andynd

andynd commented Aug 11, 2018

Copy link
Copy Markdown
Author

I did not yet have time to publish our python side for this, but are there any plans to merge this?

@Lisa999

Lisa999 commented Aug 11, 2018 •

Copy link
Copy Markdown

@andynd: can you fix the makefile error? It's the ULP which has been added to the makefile.
Some examples of use would be nice, like:
esp.partition_find_first(1,129,None)
esp.ota_get_boot_partition()

@andynd

andynd commented Aug 12, 2018

Copy link
Copy Markdown
Author

Merged upstream master. Any idea where to put the examples? I don't want to add yet another readme file like I had to for the ulp.

Andreas Valder added 3 commits August 12, 2018 14:39
esp.ota_get_running_partition()
esp.ota_get_next_update_partition(start_from=None)
@andynd

andynd commented Aug 12, 2018 •

Copy link
Copy Markdown
Author

I've added a README.ota.md on how to use this APIs. The code (in the README) is extracted from our product source code and some (irrelevant) parts removed. I have not tested it after I removed this parts but the production code is working so it should at most have some minor errors.

@Lisa999

Lisa999 commented Aug 12, 2018 •

Copy link
Copy Markdown

I've integrated it into my micropython tree, works just fine (after fixing the makefile merge)!
Thx for the good work, this is just what i needed!

Ps, you can't use a 512K fatvfs! The minimum for 4K sectors is 512K, but.... an additional 16K is being used for the WL (wear level). Here's what espressif said:

With 4k sector size, the smallest FAT partition size is 512K (128 sectors), plus 4 sectors used by WL, makes the minimum of 528K.

https://www.esp32.com/viewtopic.php?t=1897

@andynd

andynd commented Aug 12, 2018

Copy link
Copy Markdown
Author

Well the micropython implementation doesn't seem to have this limitation. Micropython doesn't use any of the esp-idf filesystem code.

@andynd

andynd commented Aug 16, 2018

Copy link
Copy Markdown
Author

Merged upstream master again. What is missing to finally merge this? The original PR is 7 months old.

@ThomasWaldmann

Copy link
Copy Markdown

@andynd guess it is the usual "many contributions, few reviewers/maintainers / little time" issue.

Comment thread ports/esp32/Makefile

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a rest of a merge conflict, no?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes it was, thx for pointing it out. Funny how it still was able to built without errors. Fixed.

@andynd

andynd commented Aug 27, 2018

Copy link
Copy Markdown
Author

poke

@andynd

andynd commented Sep 5, 2018

Copy link
Copy Markdown
Author

more poke

@mirko

mirko commented Jan 29, 2019 •

Copy link
Copy Markdown

Final linking fails on latest master - probably due to a recent upgrade of used ESP-IDF/SDK version:

xtensa-esp32-elf-ld -nostdlib -Map=build/application.map --cref --gc-sections -static -EL -u call_user_start_cpu0 -u uxTopUsedPriority -u ld_include_panic_highint_hdl -u __cxa_guard_dummy  -L/home/mirko/esp-idf/components/esp32/ld -T build/esp32_out.ld -T build/esp32.common.ld -T esp32.rom.ld -T esp32.peripherals.ld -T esp32.rom.spiram_incompatible_fns.ld -o build/application.elf   --start-group -L/usr/local/xtensa-esp32-elf/bin/../lib/gcc/xtensa-esp32-elf/5.2.0/ -lgcc -L/usr/local/xtensa-esp32-elf/bin/../lib/gcc/xtensa-esp32-elf/5.2.0/../../../../xtensa-esp32-elf/lib/ -lstdc++ /home/mirko/esp-idf/components/newlib/lib/libc.a /home/mirko/esp-idf/components/newlib/lib/libm.a /home/mirko/esp-idf/components/esp32/libhal.a -L/home/mirko/esp-idf/components/esp32/lib -lcore -lmesh -lnet80211 -lphy -lrtc -lpp -lwpa -lsmartconfig -lcoexist -lwps -lwpa2  build/py/mpstate.o build/py/nlr.o build/py/nlrx86.o build/py/nlrx64.o build/py/nlrthumb.o build/py/nlrxtensa.o build/py/nlrsetjmp.o build/py/malloc.o build/py/gc.o build/py/pystack.o build/py/qstr.o build/py/vstr.o build/py/mpprint.o build/py/unicode.o build/py/mpz.o build/py/reader.o build/py/lexer.o build/py/parse.o build/py/scope.o build/py/compile.o build/py/emitcommon.o build/py/emitbc.o build/py/asmbase.o build/py/asmx64.o build/py/emitnx64.o build/py/asmx86.o build/py/emitnx86.o build/py/asmthumb.o build/py/emitnthumb.o build/py/emitinlinethumb.o build/py/asmarm.o build/py/emitnarm.o build/py/asmxtensa.o build/py/emitnxtensa.o build/py/emitinlinextensa.o build/py/formatfloat.o build/py/parsenumbase.o build/py/parsenum.o build/py/emitglue.o build/py/persistentcode.o build/py/runtime.o build/py/runtime_utils.o build/py/scheduler.o build/py/nativeglue.o build/py/stackctrl.o build/py/argcheck.o build/py/warning.o build/py/map.o build/py/obj.o build/py/objarray.o build/py/objattrtuple.o build/py/objbool.o build/py/objboundmeth.o build/py/objcell.o build/py/objclosure.o build/py/objcomplex.o build/py/objdeque.o build/py/objdict.o build/py/objenumerate.o build/py/objexcept.o build/py/objfilter.o build/py/objfloat.o build/py/objfun.o build/py/objgenerator.o build/py/objgetitemiter.o build/py/objint.o build/py/objint_longlong.o build/py/objint_mpz.o build/py/objlist.o build/py/objmap.o build/py/objmodule.o build/py/objobject.o build/py/objpolyiter.o build/py/objproperty.o build/py/objnone.o build/py/objnamedtuple.o build/py/objrange.o build/py/objreversed.o build/py/objset.o build/py/objsingleton.o build/py/objslice.o build/py/objstr.o build/py/objstrunicode.o build/py/objstringio.o build/py/objtuple.o build/py/objtype.o build/py/objzip.o build/py/opmethods.o build/py/sequence.o build/py/stream.o build/py/binary.o build/py/builtinimport.o build/py/builtinevex.o build/py/builtinhelp.o build/py/modarray.o build/py/modbuiltins.o build/py/modcollections.o build/py/modgc.o build/py/modio.o build/py/modmath.o build/py/modcmath.o build/py/modmicropython.o build/py/modstruct.o build/py/modsys.o build/py/moduerrno.o build/py/modthread.o build/py/vm.o build/py/bc.o build/py/showbc.o build/py/repl.o build/py/smallint.o build/py/frozenmod.o build/extmod/moductypes.o build/extmod/modujson.o build/extmod/modure.o build/extmod/moduzlib.o build/extmod/moduheapq.o build/extmod/modutimeq.o build/extmod/moduhashlib.o build/extmod/moducryptolib.o build/extmod/modubinascii.o build/extmod/virtpin.o build/extmod/machine_mem.o build/extmod/machine_pinbase.o build/extmod/machine_signal.o build/extmod/machine_pulse.o build/extmod/machine_i2c.o build/extmod/machine_spi.o build/extmod/modussl_axtls.o build/extmod/modussl_mbedtls.o build/extmod/modurandom.o build/extmod/moduselect.o build/extmod/modwebsocket.o build/extmod/modwebrepl.o build/extmod/modframebuf.o build/extmod/vfs.o build/extmod/vfs_reader.o build/extmod/vfs_posix.o build/extmod/vfs_posix_file.o build/extmod/vfs_fat.o build/extmod/vfs_fat_diskio.o build/extmod/vfs_fat_file.o build/extmod/utime_mphal.o build/extmod/uos_dupterm.o build/lib/embed/abort_.o build/lib/utils/printf.o build/build/frozen_mpy.o build/main.o build/uart.o build/gccollect.o build/mphalport.o build/fatfs_port.o build/help.o build/modutime.o build/moduos.o build/machine_timer.o build/machine_pin.o build/machine_touchpad.o build/machine_adc.o build/machine_dac.o build/machine_pwm.o build/machine_uart.o build/modmachine.o build/modnetwork.o build/network_lan.o build/network_ppp.o build/modsocket.o build/modesp.o build/esp32_ulp.o build/modesp32.o build/espneopixel.o build/machine_hw_spi.o build/machine_wdt.o build/mpthreadport.o build/machine_rtc.o build/extmod/modbtree.o build/lib/berkeley-db-1.xx/btree/bt_close.o build/lib/berkeley-db-1.xx/btree/bt_conv.o build/lib/berkeley-db-1.xx/btree/bt_debug.o build/lib/berkeley-db-1.xx/btree/bt_delete.o build/lib/berkeley-db-1.xx/btree/bt_get.o build/lib/berkeley-db-1.xx/btree/bt_open.o build/lib/berkeley-db-1.xx/btree/bt_overflow.o build/lib/berkeley-db-1.xx/btree/bt_page.o build/lib/berkeley-db-1.xx/btree/bt_put.o build/lib/berkeley-db-1.xx/btree/bt_search.o build/lib/berkeley-db-1.xx/btree/bt_seq.o build/lib/berkeley-db-1.xx/btree/bt_split.o build/lib/berkeley-db-1.xx/btree/bt_utils.o build/lib/berkeley-db-1.xx/mpool/mpool.o build/extmod/modonewire.o build/lib/mp-readline/readline.o build/lib/netutils/netutils.o build/lib/timeutils/timeutils.o build/lib/utils/pyexec.o build/lib/utils/interrupt_char.o build/lib/utils/sys_stdio_mphal.o build/lib/oofatfs/ff.o build/lib/oofatfs/option/unicode.o build/drivers/bus/softspi.o build/drivers/dht/dht.o build/esp-idf/driver/libdriver.a build/esp-idf/esp32/libesp32.a build/esp-idf/esp_ringbuf/libesp_ringbuf.a build/esp-idf/heap/libheap.a build/esp-idf/soc/libsoc.a build/esp-idf/cxx/libcxx.a build/esp-idf/ethernet/libethernet.a build/esp-idf/expat/libexpat.a build/esp-idf/pthread/libpthread.a build/esp-idf/freertos/libfreertos.a build/esp-idf/vfs/libvfs.a build/esp-idf/json/libjson.a build/esp-idf/log/liblog.a build/esp-idf/xtensa-debug-module/libxtensa-debug-module.a build/esp-idf/tcpip_adapter/libtcpip_adapter.a build/esp-idf/app_trace/libapp_trace.a build/esp-idf/app_update/libapp_update.a build/esp-idf/newlib/libnewlib.a build/esp-idf/nghttp/libnghttp.a build/esp-idf/nvs_flash/libnvs_flash.a build/esp-idf/smartconfig_ack/libsmartconfig_ack.a build/esp-idf/spi_flash/libspi_flash.a build/esp-idf/ulp/libulp.a build/esp-idf/lwip/liblwip.a build/esp-idf/mbedtls/libmbedtls.a build/esp-idf/wpa_supplicant/libwpa_supplicant.a --end-group
build/esp-idf/app_update/libapp_update.a(esp_ota_ops.o):(.literal.image_validate+0x4): undefined reference to `esp_image_verify'
build/esp-idf/app_update/libapp_update.a(esp_ota_ops.o):(.literal.rewrite_ota_seq$part$2+0x0): undefined reference to `bootloader_common_ota_select_crc'
build/esp-idf/app_update/libapp_update.a(esp_ota_ops.o):(.literal.esp_ota_set_boot_partition+0x0): undefined reference to `bootloader_common_get_active_otadata'
build/esp-idf/app_update/libapp_update.a(esp_ota_ops.o):(.literal.esp_ota_get_boot_partition+0x4): undefined reference to `bootloader_common_ota_select_invalid'
build/esp-idf/app_update/libapp_update.a(esp_ota_ops.o): In function `image_validate':
esp_ota_ops.c:(.text.image_validate+0x16): undefined reference to `esp_image_verify'
build/esp-idf/app_update/libapp_update.a(esp_ota_ops.o): In function `rewrite_ota_seq$part$2':
esp_ota_ops.c:(.text.rewrite_ota_seq$part$2+0xc): undefined reference to `bootloader_common_ota_select_crc'
build/esp-idf/app_update/libapp_update.a(esp_ota_ops.o): In function `esp_ota_set_boot_partition':
esp_ota_ops.c:(.text.esp_ota_set_boot_partition+0x62): undefined reference to `bootloader_common_get_active_otadata'
build/esp-idf/app_update/libapp_update.a(esp_ota_ops.o): In function `esp_ota_get_boot_partition':
esp_ota_ops.c:(.text.esp_ota_get_boot_partition+0x12): undefined reference to `bootloader_common_ota_select_invalid'
esp_ota_ops.c:(.text.esp_ota_get_boot_partition+0x23): undefined reference to `bootloader_common_ota_select_invalid'
esp_ota_ops.c:(.text.esp_ota_get_boot_partition+0x33): undefined reference to `bootloader_common_get_active_otadata'
Makefile:848: recipe for target 'build/application.elf' failed
make: *** [build/application.elf] Error 1

@andynd

andynd commented Jan 29, 2019

Copy link
Copy Markdown
Author

Yes, because this pull request is over a year old. Unless somebody tells me he is going to merge it I will not again spend the time to fix it.

@ThomasWaldmann

Copy link
Copy Markdown

@dpgeorge ^^^

@Lisa999

Lisa999 commented Mar 18, 2019

Copy link
Copy Markdown

@dpgeorge: Wake up call...

@Lisa999

Lisa999 commented Mar 18, 2019 •

Copy link
Copy Markdown

@andynd : i've build my work on your pr, could you please fix the problem? I'll stalk @dpgeorge

@Lisa999

Lisa999 commented Mar 26, 2019

Copy link
Copy Markdown

@dpgeorge: what does it take to get this merged?

@Lisa999

Lisa999 commented Apr 1, 2019

Copy link
Copy Markdown

@dpgeorge ^^^

@Lisa999

Lisa999 commented Apr 13, 2019

Copy link
Copy Markdown

@dpgeorge : as long as this doesn’t getting merged, i won’t create the PR requested in #4436 ...
What’s the point in making PR’s if they are open more then a year?

@ThomasWaldmann

Copy link
Copy Markdown

I am watching this since quite a while and I think this project needs more core developers who are trusted enough to review and merge PRs into this project.

I don't have personal interest to become a core dev (I don't like coding in C and also do not use micropython that much, I like micropython, but basically just played with it yet), but I know there are people who would qualify (just look at the PRs and forks).

Also, considering that revision control makes it easy to also revert stuff in case something is found to be problematic, maybe PRs should be accepted more easily. New stuff can be marked experimental for a while, without any guarantees (not even for staying).

Open a ticket about this?

@Lisa999

Lisa999 commented Apr 14, 2019

Copy link
Copy Markdown

@ThomasWaldmann: I suspect that @dpgeorge wants to be in control and therefor doesn’t want to delegate the PR merge rights. Maybe that’s because of the breakup with pfalcon.
I don’t have the rights, so i can’t do it myself. My company is happy to pay to get this, and other PR’s, merged...
There are already three PR’s i need to get merged, so the question is who can do the merge and how much is that going to cost (in donation, contract work or whatever).

@spacemanspiff2007

Copy link
Copy Markdown
Contributor

@Lisa999: The lack of progress really is super frustrating and it's a shame that pfalcon is not working at this repo any more. Maybe you can team up with pfalcon (or someone else) on his repo or find a fork where there are more activity (maybe LoBoris)?
I'd really like to see the project advancing so it would be nice to have people working together.

@dpgeorge

Copy link
Copy Markdown
Member

Sorry all for neglecting this. I'll try to make some progress here.

I think it's a good feature to have, so thanks @andynd for contributing it. But I think the way it's done, the way it is exposed at the Python level, needs to be changed.

The reason is because it looks like the approach here cannot support encrypted partitions (but please correct me if I'm wrong). That's because encrypted read/write must go through esp_partition_read/esp_partition_write (again, correct me if I'm wrong about this). If in the future encryption support was added to uPy we don't want to have to change everything, so it's good to think now how encryption would fit in.

As such my proposal would be to make the API more object-based, rather than pure function calls. In particular put emphasis on the concept of a Partition object. A Partition object would:

  • contain a pointer (at the C level) to a esp_partition_t struct
  • contain methods to retrieve the metadata (type, subtype, address, size, etc)
  • contain methods to read/write/erase the partition

Partition objects would be created by esp_partition_find_first(), esp_ota_get_boot_partition() and esp_ota_get_running_partition(), and be passed to esp_ota_set_boot_partition() and esp_ota_get_next_update_partition().

For example the API could be:

class Partition:
    @staticmethod
    def find_first(type, subtype, label) -> Partition: ...

    def metadata(self) -> (type, subtype, address, size, label, encrypted): ...

    def readinto(self, offset, buf): ....
    def write(self, offset, buf): ....
    def erase(self, offset, size): ....

def ota_get_boot_partition() -> Partition: ...
def ota_get_running_partition() -> Partition: ...
def ota_set_boot_partition(part): ...
def ota_get_next_update_partition(part) -> Partition: ...

By having a Partition object you don't need to worry about passing the correct offsets to esp.write_flash etc. And Partition could in the future support the block protocol allowing it to have filesystems on it and be mounted, etc (so it'd replace the FlashBdev class).

Comment thread ports/esp32/README.ota.md
import esp
import sys

def bytecompare(a,b):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this function necessary, isn't it enough to just do a == b? Or do I miss something?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Its not, (a == b) can be = false while bytecompare(a,b) returns true, because its compare by referenz and compare by value

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you give an example where it's not the same? MicroPython compares by value, eg:

>>> bytearray(b'\x01\x02\x03')==b'\x01\x02\x03'
True
>>> bytearray(b'\x01\x02\x03')==bytearray(b'\x01\x02\x03')
True
>>> b'\x01\x02\x03'==bytearray(b'\x01\x02\x03')
True

@dpgeorge

dpgeorge commented Jul 9, 2019

Copy link
Copy Markdown
Member

See #4910 for a proposed Partition class.

@Lisa999

Lisa999 commented Jul 19, 2019 •

Copy link
Copy Markdown

@andynd: #4910 partitions support is great, is this PR still needed?

@waneksx

waneksx commented Oct 17, 2019

Copy link
Copy Markdown

Is it posible to resolve those conflicts?

@Lisa999

Lisa999 commented Oct 17, 2019 •

Copy link
Copy Markdown

Is it posible to resolve those conflicts?

#4910 is integrated into the master branch, #3576 is obsolete now...

@mrwhy-orig

Copy link
Copy Markdown

Where are we on this one? I got a bit lost with all those cross references... Is there documentation on how to make OTA possible?

@tve

tve commented Jan 23, 2020

Copy link
Copy Markdown
Contributor

Why is this PR still open? Is it hoping for someone to turn pieces of the README.ota.md into docs?

@mrwhy-orig

Copy link
Copy Markdown

My guess this README.ota.md is outdated. @dpgeorge implemented the Partition Class which serves as a replacement for another Class which is used in this pull request.
I think there is no Updated README.ota.md yet.

Comment thread ports/esp32/README.ota.md
@@ -0,0 +1,405 @@
# Over the air updates

This is an example of how to do ota-updates using micropython and a very simple bottle server application. It assumes you have some way of notifying your esp of updates and transmit the update hashe securely (for example using ssl with certificate pinning).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

typo "hashe"

how about cryptographically signed updates rather? the esp-idf / esp32 support ecdsa signatures and can verify that on update (flashing) / on boot (kind of "secure boot"). they even support encrypted flash images, but there is no requirement to use encryption if you just want to have signatures.

that way, the security of the server having the update files and that of the transmission channel does not matter.
if the signature is ok, you know the file is authentic, complete, correct (as released / signed by the author).
at least as long as you manage to keep your private signing key private.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agreed. As long as there are no secrets or otherwise sensitive info embedded in the MP firmware being updated it should be fine to verify a hash over the firmware assuming the hash got transmitted securely. A signature is more flexible but much more of a hassle in many use-cases.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this has nothing to do with whether it has secrets inside, but whether you can authenticate the image. you can't authenticate with just a hash from same source.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It's save as long as you have a secure way to transfer this hash. We did not want signed updates because the signature verification was too complex and it was not needed for our usecase. We use certificate pinning and would have gained nothing except protection against a attacker controlled server, but that was not our attack scenario.

Comment thread ports/esp32/README.ota.md

def bytecompare(a,b):
if (len(a) != len(b)):
return False

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

tabs?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

tabs > spaces.

Comment thread ports/esp32/README.ota.md
def trackerprobe():
if do_update:
res['update'] = {}
res['update']['partition'] = {'url': '{}/chunk/part'.format(otahost), 'size': int(os.stat(partfile).st_size), 'hash': hashlib.sha256(open(partfile, 'rb').read()).hexdigest()}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

the problem with this is that if somebody hacks your server and puts some tampered update binaries on it (instead the original good ones), the code will compute the hash for the tampered ones and the updater code on the esp32 will happily flash the bad update.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

(see above how to avoid this)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That was not the attack vector we cared about. We wanted verified updates without confidentiality, thats why no encryption.

@tve tve left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm very interested in OTA (or rather, will be when I'm through with my current TODO list :-). But I'm not understanding what this PR is proposing. It's great to have a long readme, but I would prefer to first have a concise statement about how OTA is supposed to work and which use-cases it covers.

For example, it seems this PR intends the entire MP filesystem to be swapped with the OTA, as opposed to separating the filesystem from the MP firmware. Also, it seems this PR intends there to be 3 slots for firmware: factory, ota1, and ota2. Yet only 2 are really required, the factory one is optional (I wouldn't want it, for example).

I have used the Arduino Update class on the esp32 successfully implementing my own update and thought it abstracted the minutiae nicely without adding too many constraints. It would be worth taking a look at what it does: https://github.com/espressif/arduino-esp32/blob/master/libraries/Update/src/Updater.cpp

Comment thread ports/esp32/modesp.c
STATIC mp_obj_t mp_esp_ota_get_boot_partition (void) {
const esp_partition_t* part = esp_ota_get_boot_partition();
void* partptr = ∂
return mp_obj_new_bytes(partptr, sizeof(partptr));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is this returning in Python-speak? An opaque bunch of bytes?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes. The pointer as bytes.

try:
if bdev:
uos.mount(bdev, '/')
print("Mounted /")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why?

nvs, data, nvs, 0x9000, 0x6000,
phy_init, data, phy, 0xf000, 0x1000,
factory, app, factory, 0x10000, 0x180000,
nvs, data, nvs, 0x9000, 0x4000

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Gratuitous whitespace change?
Why the size reduction?

Comment thread ports/esp32/README.ota.md

## Folders

We are using the following folder structure:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

On the host or on the device?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

device

Comment thread ports/esp32/README.ota.md
└─ ota_1
```

The folders "factory", "ota_o" and "ota_1" musst have corresponding partitions. "VERSION" contains our currently running software version identifier, for example a git commit hash.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why 3 partitions, i.e. copies of the firmware? Or am I misunderstanding?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

in the product where this code is from, we used "factory" with the factory image that never got updated and was the fallback code for some specific scenarios. You can use only two partitions as well.

@andynd

andynd commented Jan 26, 2020

Copy link
Copy Markdown
Author

In general: please understand that this code was written for a specific product with specific tradeoffs and making it open source was just an afterthought. Now that there is a Partition class, you can write some parts different and in a nicer way.

Please understand as well that we wanted secure (as in someone who controlles the network can not tamper with them) and not confidential (an attacker can read firmware images, they are not secret and we do not care if someone gets them). An attacker who attacks our server and steals the SSL certificate or replaces the binary was not our concern. For this scenario it is fine to transfere a hash of the update over a ssl protected connection with certificate pinning and not to do signature verification and to transmit the update chunks unencrypted.

@andynd

andynd commented Feb 22, 2020

Copy link
Copy Markdown
Author

#4910 and #5027 implemented all changes I did here in a better way, closing this PR.

@andynd andynd closed this Feb 22, 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.