Skip to content

Give every firmware its own partition table in the release - #161

Merged
ewowi merged 2 commits into
MoonModules:mainfrom
AnjinMeili:fix-partition-table-per-firmware
Oct 28, 2026
Merged

Give every firmware its own partition table in the release#161
ewowi merged 2 commits into
MoonModules:mainfrom
AnjinMeili:fix-partition-table-per-firmware

Conversation

@AnjinMeili

@AnjinMeili AnjinMeili commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

The release named the partition table by flash size, assuming every firmware of one size shares a layout. They don't: esp32-pico's MoonBase table puts the app at 0xF0000, esp32s3-n8r8's dual-OTA table at 0x10000, and esp32-16mb's MoonBase table differs from the 16 MB OTA table of the N16R8, P4 and S31 variants. Every build job overwrote the shared file, so v7.0.0 publishes the MoonBase 8 MB table and the OTA 16 MB table, and the N8R8 and esp32-16mb web installers write the wrong layout.

The table now carries the firmware prefix, like the app and bootloader. Only the ota-data, identical for every firmware, stays shared. Releases before this one keep their own names on Pages.

Found through Copilot's review of #160. N8R2 uses pico's table, so #160 adds no new collision.

Tested: test_host (Python and JS). A new test fails against the old generator.

@AnjinMeili @claude
Give every firmware its own partition table in the release
c885948 
The release named the partition table by flash size, assuming every firmware of one size shares a layout. They do not: esp32-pico's MoonBase table puts the app at 0xF0000 and esp32s3-n8r8's dual-OTA table at 0x10000, and esp32-16mb's MoonBase table differs from the 16 MB OTA table of esp32s3-n16r8, the P4s and esp32s31. Each matrix job overwrote the shared file, so v7.0.0 publishes the MoonBase 8 MB table and the OTA 16 MB table, and the N8R8 and esp32-16mb web installers write the wrong layout. The table now carries the firmware prefix, like the app and bootloader; only the ota-data, identical for every firmware, stays shared.

**Scripts/MoonDeck**
- generate_manifest: the partition-table part is `firmware-<F>-v<ver>-partition-table.bin`; the flash-size lookup goes.
- preview_installer stages the same name.

**Tests**
- `test_every_firmware_names_its_own_partition_table`: every shipped ESP32 manifest names a table of its own. Control: the previous generator gives esp32-pico and esp32s3-n8r8 both `partition-table-8mb.bin`.
- The installer merge test rejects the per-firmware table as an app image.
- test_host passes: Python 616, JS 350.

**Docs/CI**
- release.yml stages the per-firmware table and no longer uploads `partition-table-*.bin`; the Pages restage keeps that pattern for the releases that published it.
- install.js: the esptool fallback for esp32s31 names the per-firmware table.

**Reviews**
- Copilot on MoonModules#160 (high): N8R2 and N8R8 share `partition-table-8mb.bin` despite different app offsets. Done: the collision was already on main (pico against N8R8, esp32-16mb against the OTA variants) and is fixed here for all of them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 28, 2026 11:41
@coderabbitai

coderabbitai Bot commented Oct 28, 2026 •
edited
Loading

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@AnjinMeili AnjinMeili mentioned this pull request Oct 28, 2026
Merged
Copilot AI reviewed Oct 28, 2026
View reviewed changes

Copilot AI 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.

🟢 Approval recommended

No active review issues are reported for the changes that give each firmware a distinct partition-table artifact name.

No open issues 🎉

What changed in this PR

This PR prevents release artifacts from overwriting distinct partition layouts for firmwares with the same flash size. Each firmware’s partition table now uses its versioned firmware prefix, keeping installer manifests aligned with the correct build.

Changes:

  • Align release staging, manifest generation, and local previews with per-firmware partition-table names; keep OTA data shared.
  • Update manual flashing instructions while preserving older releases’ published manifest names on Pages.
  • Add a regression test for unique partition-table names and update the JavaScript asset-parsing fixture.
File Description
test/​python/​test_installer_manifests.py Verify every ESP32 firmware names its own partition table.
test/​js/​installer-firmware-merge.test.mjs Update the partition-table asset fixture to the new naming scheme.
mooninstaller/​install.js Update ESP32-S31 manual flashing instructions.
moondeck/​run/​preview_installer.py Stage local partition tables with firmware prefixes.
moondeck/​build/​generate_manifest.py Generate per-firmware partition-table paths without flash-size grouping.
.github/​workflows/​release.yml Publish uniquely named partition tables and clarify historical naming comments.

Review effort: Balanced

@ewowi @claude
Pin the manifest contract to the names a release publishes
8f058e7 
The web installer's contract test accepts only the file names a new release uploads, so a manifest naming a shared partition table, which no release after v7.0.0 publishes, fails the test instead of 404ing in the installer.

**Tests**
- test_installer_manifests: `STAGED_GLOBS` drops `partition-table-*.bin`, which only the releases up to v7.0.0 publish; the per-firmware table matches `firmware-*.bin`. The synthetic flasher_args drops `flash_settings`, which the generator no longer reads.
- test_host (Python 615 passed, JS 350 passed), check_prose, check_specs, check_taglines, check_docgen (0 errors, warnings unchanged), docs build --strict.

**Docs/CI**
- testing.md: the contract test's globs, with each firmware's table under `firmware-*.bin`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ewowi
ewowi merged commit 04472e0 into MoonModules:main Oct 28, 2026
10 checks passed
@ewowi

ewowi commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator

Thanks James, merged! You found this through Copilot's note on #160, traced it to a bug that predates it, and fixed it with a test that fails on the old generator. The v7.0.0 installers for esp32s3-n8r8 and esp32-16mb write the wrong table, so this matters.

I added one maintainer commit: the manifest contract test (STAGED_GLOBS, its docstring and the line in testing.md) still accepted partition-table-*.bin, which a new release no longer publishes, so a manifest pointing at it would have passed the test and 404'd in the installer. It now accepts only firmware-*.bin and shared-*.bin. The release workflow's Pages step keeps that pattern for the older releases.

Looking forward to the next one. The processes behind the review are on MoonWalk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@AnjinMeili @ewowi