8000 Espressif: fix handling of single-partition CIRCUITPY (non-CIRCUITPY_STORAGE_EXTEND) - fixes MEMENTO bug by dhalbert · Pull Request #8952 · adafruit/circuitpython · GitHub
[go: up one dir, main page]

Skip to content

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

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
Feb 20, 2024

Conversation

dhalbert
Copy link
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.
Copy link
@jepler jepler left a comment

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.

@dhalbert dhalbert requested a review from jepler February 20, 2024 00:58
@FoamyGuy
Copy link
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.

Copy link
@jepler jepler left a comment

Choose a reason for hiding this comment

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

thanks for fixing that!

8000
@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
Copy link
Member
tannewt commented Feb 20, 2024

@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
0