Skip to content

Espressif: fix handling of single-partition CIRCUITPY (non-CIRCUITPY_STORAGE_EXTEND) - fixes MEMENTO bug#8952

Merged
jepler merged 5 commits into
adafruit:mainfrom
dhalbert:memento-storage-crash
Feb 20, 2024
Merged

Espressif: fix handling of single-partition CIRCUITPY (non-CIRCUITPY_STORAGE_EXTEND) - fixes MEMENTO bug#8952
jepler merged 5 commits into
adafruit:mainfrom
dhalbert:memento-storage-crash

Conversation

@dhalbert

Copy link
Copy Markdown
Collaborator

  • Fix supervisor_flash_write_blocks() to write instead of read when handling a single firmware partition (thanks @jepler).
  • Fix adafruit_esp32s3_camera/mpconfigboard.mk (MEMENTO)to use proper setting name for partition file.
  • Rename partition files to have consistent names: no-ota` if only one firmware partition.
  • Forbid CIRCUITPY_DUALBANK = 1 and CIRCUITPY_STORAGE_EXTEND = 1 if there is only one firmware partition. Thanks @jepler for initial version of validation script.

Also, some cleanups of harmless obsolete things, encountered while debugging, which are unrelated to this bug.

  • Remove obsolete *SUPERVISOR_ALLOCATION makefile settings.
  • Remove obsolete definition from circuitpy_mpconfig.h .

After loading this firmware, and doing storage.erase_filesystem(), the MEMENTO will have a proper CIRCUITPY filesystem (tested). Formerly it was too big.


While debugging this, @jepler came across this code:

if (_cache_lba != block_address) {
supervisor_flash_read_blocks(_cache, sector_offset / FILESYSTEM_BLOCK_SIZE, blocks_per_sector);
_cache_lba = sector_offset;
}

We are unsure whether this code is setting _cache_lba to the right value.

@tannewt if you remember this code, could you take a look? But this code was present before CIRCUITPY_DUALBANK and CIRCUITPY_STORAGE_EXTEND were present.

dhalbert and others added 4 commits February 17, 2024 22:42
betweek sdkconfig and circuitpython. For now there's a single check,
for CIRCUITPY_STORAGE_EXTEND & CIRCUITPY_DUALBANK that require
there to be an ota_1 partition.

@jepler jepler left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thank you -- excellent cleanups done along the way. However, can you double check this comment you added? It's the opposite of what I understood from the discussions.

Comment thread ports/espressif/boards/adafruit_esp32s3_camera/mpconfigboard.mk Outdated
@dhalbert dhalbert requested a review from jepler February 20, 2024 00:58
@FoamyGuy

Copy link
Copy Markdown
Collaborator

I tested a build from this branch successfully on the device. With this version the drive size reported to OS makes it so it will warn me that the file is too large to fit when I try pasting it if there is already other things on the drive.

I also have been able to successfully use this build with a modified version of the basic camera example from learn and been able to take a few photos successfully with it.

@jepler jepler left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

thanks for fixing that!

@jepler jepler merged commit cbdaaea into adafruit:main Feb 20, 2024
@jepler jepler deleted the memento-storage-crash branch February 20, 2024 02:11
@tannewt

tannewt commented Feb 20, 2024

Copy link
Copy Markdown
Member

@tannewt if you remember this code, could you take a look? But this code was present before CIRCUITPY_DUALBANK and CIRCUITPY_STORAGE_EXTEND were present.

It doesn't look right to me and it looks like RP2040 has the same bug. I've always meant to factor out this cache code but never have...

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.

Large Files on MEMENTO Causes Device Reset & Eventual Hard Crash

4 participants