Skip to content

Ask which chip it is, and which layout goes on it - #130

Merged
openipc-ai merged 2 commits into
masterfrom
flash-chip-and-partition-layout
Aug 29, 2026
Merged

Ask which chip it is, and which layout goes on it#130
openipc-ai merged 2 commits into
masterfrom
flash-chip-and-partition-layout

Conversation

@openipc-ai

Copy link
Copy Markdown
Collaborator

The report

A user could not revive a camera with a ruined overlay, however many times they
flashed the full image. It took them five minutes to work out why, and they
noted that a less experienced person would simply be stuck.

Their flash was 16MB and they had chosen the 8MB entry in the wizard. The
instructions that came out erase 0x0..0x800000 and stop. Every OpenIPC
mtdparts ends -(rootfs_data) — to the end of the device — so on a 16MB part
the overlay runs to 0x1000000, and the half above the erase was never touched.
general/overlay/init mounts a jffs2 it finds and only calls flash_eraseall
when that mount fails, so the old, broken overlay came back on every boot.

They got out of it by generating the 8MB page for its image, the 16MB page for
its sf erase 0x0 0x1000000, and editing 16 to 8 in the filename by hand.

The cause

One menu was answering two questions: how big the chip is, and how it is carved
up. Those are not always the same answer. nor32m was already the exception
that proved it — the controller rewrote it to the nor16m command set because
no bootloader upstream defines mtdpartsnor32m — so 32MB chip + 16MB layout
was expressible while 16MB chip + 8MB layout was not.

The change

The flash-type menu still means the chip and keeps its ?rom= permanent-link
key. A second menu picks the partition layout and defaults to the chip's own, so
every configuration the form could produce before this renders identically.
The assembled image is the size of the chip whatever layout goes inside it, so
the erase spans the whole part and a stale overlay has nowhere to survive.

Three things that were already wrong fell out of the split:

  • The nor32m → nor16m rewrite is gone. It only ever meant "a 32MB chip wears
    the 16MB layout", which Camera#partition_layout now says directly.
  • The rule keeping Ultimate off a 5120KB rootfs partition was keyed on the chip.
    That is the layout's doing, so it now catches the 8MB layout on a 32MB part
    too, and the message names whichever of the two is actually the limit.
  • The 32MB page never said which layout it was describing. It does now.

Images already cached keep their names: the layout only enters the filename when
it is not the one the chip would have had anyway
(openipc-<soc>-nor-lite-16mb-parts8m.bin).

Verification

  • 340 tests, 0 failures. New coverage for the layout default and its refusals,
    the chip-sized erase under a smaller layout, the filename rule, and
    Firmware's validation of a layout larger than the chip.
  • No new RuboCop offences in any touched file, measured against a stashed
    baseline. The test files came out four under.
  • i18n-tasks missing clean; en/ru/zh all carry the new strings.
  • Every chip × layout combination rendered against a live server and the emitted
    U-Boot commands read by hand, including NAND (layout menu hidden,
    uknand/urnand unchanged) and both refusal paths.

Noted, not fixed here

Camera#flash_size_blocks answers 0x16000 for nor32m — decimal 65536 with a
hex prefix, where it should be 0x10000. It would make the SD-card backup
mmc write 44MB from a 32MB read. It renders for nobody today, because the two
selects that reach that path are commented out of the form.

A camera with a ruined overlay could not be revived by a full reflash. Its
flash was 16MB, the 8MB entry had been chosen in the wizard, and the
instructions erased 0x0..0x800000 and stopped. Every OpenIPC mtdparts ends
`-(rootfs_data)`, meaning to the end of the device, so the overlay ran to
0x1000000 and the half above the erase survived. /init mounts a jffs2 it finds
and only reformats when that mount fails, so the old, broken overlay came back
every boot.

The reporter got out of it by generating the 8MB page for its image, the 16MB
page for its `sf erase 0x0 0x1000000`, and editing the filename by hand.

The menu was one field doing two jobs: how big the chip is, and how it is
carved up. Those are not always the same answer -- nor32m was already the
exception, rewritten to the nor16m command set because no bootloader upstream
defines mtdpartsnor32m. So split them. The flash type stays the chip and keeps
its permanent-link key; a second menu picks the layout and defaults to the
chip's own, which is what every combination the form could produce before this
already had. The assembled image is the size of the chip whatever layout goes
inside it, so the erase spans the whole part and there is nowhere for a stale
overlay to survive.

Everything that used to key on the chip and meant the layout now says so:
FlashLayout's offsets, the uk/ur/set macro names, and the rule that keeps
Ultimate off a 5120KB rootfs partition -- which is the layout's doing and not
the chip's, so it now applies to the 8MB layout on a 32MB part too.

Images already cached keep their names: the layout only enters the filename
when it is not the one the chip would have had anyway.
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Separate flash chip size from partition layout

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 40+ Minutes

Grey Divider

AI Description

• Separates physical flash capacity from selectable NOR partition geometry.
• Builds chip-sized images and erase commands, preventing stale overlays after reflashing.
• Validates layout compatibility while preserving legacy filenames, links, NAND behavior, and
 translations.
Diagram

graph TD
  W["Setup Wizard"] --> C["SoCs Controller"] --> M["Camera Config"] --> L["Flash Layout"]
  C --> F["Firmware Builder"] --> I["Chip-sized Image"]
  M --> H["Install Commands"]
  L --> F
  L --> H
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Always migrate to the chip-native layout
  • ➕ Removes the second user choice
  • ➕ Keeps chip size and partition geometry identical
  • ➖ Cannot accurately reflash cameras already using a smaller layout
  • ➖ Changes existing partitioning and overlay allocation
  • ➖ Risks bootloader and kernel layout mismatches
2. Infer layout from firmware edition
  • ➕ Avoids adding another form field
  • ➕ Could preserve a simpler wizard
  • ➖ Edition does not define bootloader partition geometry
  • ➖ Reintroduces coupling that previously produced incorrect offsets
  • ➖ Cannot represent all valid chip and layout combinations

Recommendation: Keep the explicit chip/layout split. It models the hardware and partitioning as independent concerns, preserves old defaults and cached filenames, and uses one selected geometry for both image assembly and installation commands; server-side validation appropriately protects hand-edited requests.

Files changed (19) +473 / -42

Enhancement (4) +129 / -11
selects_helper.rbProvide translated partition-layout selector options +7/-0

Provide translated partition-layout selector options

• Adds a helper that exposes the supported NOR 8MB and 16MB layouts to the wizard.

app/helpers/selects_helper.rb

camera.rbModel flash capacity and partition geometry separately +64/-1

Model flash capacity and partition geometry separately

• Introduces layout defaults, compatibility checks, translated names, and layout-derived NOR offsets. Permanent links now preserve the selected layout while NAND remains fixed to its UBI layout.

app/models/camera.rb

show.html.erbAdd a chip-aware partition-layout menu +49/-8

Add a chip-aware partition-layout menu

• Adds a layout selector that hides for NAND and disables layouts too large for the selected chip. Firmware edition filtering now follows rootfs capacity from the selected layout.

app/views/cameras/socs/show.html.erb

update.html.erbShow selected layout and pass it to image downloads +9/-2

Show selected layout and pass it to image downloads

• Displays the partition geometry assumed by the installation commands and includes it in download URLs. Non-default layout details are also shown beside the physical flash size.

app/views/cameras/socs/update.html.erb

Bug fix (3) +113 / -20
socs_controller.rbAccept and enforce independent partition-layout selections +58/-13

Accept and enforce independent partition-layout selections

• Adds layout handling to wizard submissions, permanent links, and full-image downloads. Bootloader commands now follow the selected layout, incompatible requests produce warnings, and 8MB rootfs edition limits are keyed to layout rather than chip size.

app/controllers/cameras/socs_controller.rb

installation_helper.rbGenerate chip-wide flashing commands for layout-specific images +11/-1

Generate chip-wide flashing commands for layout-specific images

• Includes non-default layouts in firmware filenames while retaining physical chip size for full-flash erase commands. This ensures stale overlay data cannot survive above a smaller partition layout.

app/helpers/installation_helper.rb

firmware.rbBuild and validate chip-sized images with selectable layouts +44/-6

Build and validate chip-sized images with selectable layouts

• Accepts an independent NOR layout, validates that it is supported and fits the chip, and uses it for image offsets. Non-default layouts receive distinct filename suffixes while legacy cache names remain unchanged.

app/models/firmware.rb

Tests (3) +210 / -11
socs_controller_test.rbCover layout-aware wizard and command behavior +85/-7

Cover layout-aware wizard and command behavior

• Adds regressions for full-chip erasure with smaller layouts, legacy defaults, incompatible layout warnings, edition limits, filenames, and permanent links. Existing selector assertions are updated to use layout-based limits.

test/controllers/socs_controller_test.rb

camera_test.rbTest camera layout defaults, geometry, and links +66/-2

Test camera layout defaults, geometry, and links

• Verifies chip-specific defaults, smaller valid layouts, rejected oversized or unknown layouts, NAND behavior, derived offsets, full overlay extent, and permalink persistence.

test/models/camera_test.rb

firmware_test.rbTest layout-aware image assembly and validation +59/-2

Test layout-aware image assembly and validation

• Covers cache-compatible filenames, non-default suffixes, chip-sized images with smaller geometry, and rejection of oversized or undefined NOR layouts.

test/models/firmware_test.rb

Other (9) +21 / -0
activemodel.en.ymlLabel the partition-layout field in English +1/-0

Label the partition-layout field in English

• Adds the English ActiveModel attribute label for the new selector.

config/locales/activemodel.en.yml

activemodel.ru.ymlLabel the partition-layout field in Russian +1/-0

Label the partition-layout field in Russian

• Adds the Russian ActiveModel attribute label for the new selector.

config/locales/activemodel.ru.yml

activemodel.zh.ymlLabel the partition-layout field in Chinese +1/-0

Label the partition-layout field in Chinese

• Adds the Chinese ActiveModel attribute label for the new selector.

config/locales/activemodel.zh.yml

activerecord.en.ymlExplain partition-layout behavior in English +1/-0

Explain partition-layout behavior in English

• Adds English help text describing defaults, overlay allocation, and the 8MB layout edition limit.

config/locales/activerecord.en.yml

activerecord.ru.ymlExplain partition-layout behavior in Russian +1/-0

Explain partition-layout behavior in Russian

• Adds Russian help text describing defaults, overlay allocation, and the 8MB layout edition limit.

config/locales/activerecord.ru.yml

activerecord.zh.ymlExplain partition-layout behavior in Chinese +1/-0

Explain partition-layout behavior in Chinese

• Adds Chinese help text describing defaults, overlay allocation, and the 8MB layout edition limit.

config/locales/activerecord.zh.yml

en.ymlAdd English layout names and installation guidance +5/-0

Add English layout names and installation guidance

• Defines translated layout choices and explains that full-image flashing covers the entire chip and clears the overlay.

config/locales/en.yml

ru.ymlAdd Russian layout names and installation guidance +5/-0

Add Russian layout names and installation guidance

• Defines translated layout choices and explains that full-image flashing covers the entire chip and clears the overlay.

config/locales/ru.yml

zh.ymlAdd Chinese layout names and installation guidance +5/-0

Add Chinese layout names and installation guidance

• Defines translated layout choices and explains that full-image flashing covers the entire chip and clears the overlay.

config/locales/zh.yml

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Aug 29, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Larger chips retain 8MB layout ✓ Resolved 🐞 Bug ≡ Correctness
Description
When the form starts on nor8m and the user changes the chip to nor16m or nor32m,
checkPartitionLayout leaves nor8m selected because it is still allowed, so the ordinary
larger-chip flow silently uses 8MB partitions and disables Ultimate. This contradicts the model's
natural 16MB default and changes configurations that the old single selector produced.
Code

app/views/cameras/socs/show.html.erb[162]

+        if (allowed.length && !allowed.includes(el.value)) el.value = allowed[allowed.length - 1];
Evidence
The controller initializes fresh forms as nor8m, and the model says larger chips naturally use
nor16m. The JavaScript allows both layouts on larger chips but changes the current value only when
it is not allowed; therefore the initial nor8m survives a chip change, and the layout-based
edition limiter then restricts the form to Lite.

app/controllers/cameras/socs_controller.rb[50-59]
app/models/camera.rb[219-228]
app/views/cameras/socs/show.html.erb[133-170]
app/views/cameras/socs/show.html.erb[196-210]
app/views/cameras/socs/show.html.erb[233-236]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Changing the flash-chip selector from 8MB NOR to a larger NOR chip leaves the 8MB partition layout selected because it remains valid. The ordinary 16MB/32MB selection must default to the natural 16MB layout while still allowing the user to deliberately select 8MB afterward.
## Issue Context
The page initially uses `nor8m`. `allowedLayouts` includes `nor8m` for larger chips, so the current invalid-only fallback never runs during an upward chip-size change; the server model, however, defines `nor16m` as the natural layout for 16MB and 32MB chips.
## Fix Focus Areas
- app/views/cameras/socs/show.html.erb[138-165]
- app/views/cameras/socs/show.html.erb[196-236]
- app/models/camera.rb[219-228]
- test/controllers/socs_controller_test.rb[537-540]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Rootfs can overwrite overlay ✓ Resolved 🐞 Bug ≡ Correctness
Description
For a 16MB/32MB image using the 8MB layout, Firmware places rootfs at the selected layout's offset
but validates it against the physical image end, allowing an oversized rootfs to cross the layout's
0x750000 overlay boundary. A direct Ultimate download can therefore produce an image whose rootfs
occupies space that the installed mtdparts exposes as rootfs_data.
Code

app/models/firmware.rb[432]

+    @nor_layout ||= FlashLayout.nor(@layout)
Evidence
The selected 8MB layout defines rootfs from 0x250000 through 0x750000, and Firmware now derives
the offset from @layout. However, its rootfs Part still uses the whole chip-sized image as the
validation limit, while validate_fit! trusts that limit; on a 16MB image this accepts payloads far
beyond the selected rootfs partition.

app/models/flash_layout.rb[27-33]
app/models/firmware.rb[221-237]
app/models/firmware.rb[332-341]
app/models/firmware.rb[427-448]
app/controllers/cameras/socs_controller.rb[162-184]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
NOR rootfs payloads must be bounded by the selected partition layout, not by the physical chip/image size. With an 8MB layout on a larger chip, the current validation permits rootfs bytes beyond `0x750000`, corrupting the area that will be treated as `rootfs_data`.
## Issue Context
The PR correctly selects rootfs offsets from `@layout`, but `layout` still gives the rootfs part `size` as its limit. Use the selected NOR layout's overlay offset (or equivalently rootfs offset plus rootfs max size) as the rootfs limit; retain the existing NAND behavior.
## Fix Focus Areas
- app/models/firmware.rb[232-237]
- app/models/firmware.rb[332-341]
- app/models/firmware.rb[427-444]
- app/models/flash_layout.rb[27-33]
- test/models/firmware_test.rb[133-166]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

3. Refusal recommends wrong hardware ✓ Resolved 🐞 Bug ≡ Correctness
Description
The new layout-based Ultimate check also reaches 16MB/32MB chips using the 8MB layout, but the
no-Lite branch still says the camera has an 8MB chip and needs a larger chip. For these newly
supported combinations the chip is already large enough; selecting the 16MB layout is the required
remedy.
Code

app/controllers/cameras/socs_controller.rb[333]

+      return unless eight_meg_rootfs_with_ultimate?
Evidence
The changed guard is true whenever the selected layout is nor8m, regardless of chip size. Its
no-Lite branch still explicitly claims an 8MB flash chip and says a larger chip is needed, whereas
the adjacent updated warning correctly tells larger-chip users to choose the 16MB layout.

app/controllers/cameras/socs_controller.rb[332-355]
app/controllers/cameras/socs_controller.rb[358-366]
app/models/camera.rb[225-232]
app/views/cameras/socs/show.html.erb[138-146]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
When Ultimate is the only published NOR release and a larger chip uses the 8MB layout, the refusal message incorrectly identifies the physical chip as 8MB and advises replacing it with a larger one. The message must distinguish an actual 8MB chip from an 8MB layout on a larger chip and recommend selecting the 16MB layout in the latter case.
## Issue Context
Changing the guard to `eight_meg_rootfs_with_ultimate?` correctly makes the partition layout the constraint, but only the downgrade warning was updated to distinguish chip and layout. The no-Lite alert remains hard-coded to the old chip-based condition.
## Fix Focus Areas
- app/controllers/cameras/socs_controller.rb[332-367]
- test/controllers/socs_controller_test.rb[262-273]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can group findings by type and pick your Finding display, from Minimal to Full

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread app/views/cameras/socs/show.html.erb Outdated
Comment thread app/models/firmware.rb
Comment thread app/controllers/cameras/socs_controller.rb
Qodo found three, and driving the wizard script through a DOM stub found a
fourth that was mine.

**The layout stopped following the chip.** The form opens on nor8m, and nor8m
is a legal layout on a larger part, so `checkPartitionLayout` never took it
away: picking NOR 16M left 8MB partitions on a 16MB camera and the edition
limiter took Ultimate off with them. It now follows the chip until the visitor
settles it themselves -- by using the menu, or by arriving on a permanent link
that carries a `part` that is not the chip's own, which is as deliberate as
clicking.

**The rootfs was bounded by the image, not by its partition.** `layout` gave
the rootfs part the whole chip-sized image as its limit. That was near enough
while the layout was the chip -- an 8MB image ends 0xb0000 past the rootfs
partition -- and is not now: a 16MB image laid out the 8MB way ends 0x8b0000
past it, and `download_full_image` takes the edition and the layout straight
from the query string. `?fw_release=ultimate&flash_size=16&layout=8` would have
written a 7MB rootfs from 0x250000 clean through rootfs_data. It is bounded by
the overlay offset now, which is what the mtdparts says the partition is.

**"This SoC needs a larger chip" reached chips that are large enough.** The
Ultimate guard became the layout's, so the no-Lite branch under it now sees
16MB and 32MB parts, where the advice is not to buy a chip but to choose the
other layout. It tells the two apart, like the warning beside it already did.

**NAND lost Ultimate.** The layout menu keeps its value while it is hidden, so
`checkPartitionLayout` handed back nor8m on a NAND part and the 8MB rootfs rule
disabled an edition that eleven of the sixteen NAND boards are published as and
nothing else. It answers with no layout where there is none to choose.
@openipc-ai
openipc-ai merged commit 7f83c0b into master Aug 29, 2026
2 checks passed
@openipc-ai
openipc-ai deleted the flash-chip-and-partition-layout branch August 29, 2026 06:33
openipc-ai pushed a commit that referenced this pull request Aug 29, 2026
* Give SigmaStar and Ingenic the layout their bootloader has

`FlashLayout::EIGHT_MEG_LAYOUT_VENDORS = %w[SigmaStar Ingenic]` was removed in
#78 on the grounds that their bootloaders define mtdpartsnor16m and the
uknor16m/urnor16m pair "down to the byte", the same as the HiSilicon and Goke
ones. The repositories that carry those definitions -- u-boot-msc313e,
u-boot-t20, u-boot-t40 -- are real, and no released binary is built from any of
them, so no camera runs them.

OpenIPC/firmware's .github/workflows/uboot.yml is what builds the bootloaders
this site links. It clones openipc/u-boot-sigmastar and openipc/u-boot-ingenic
and runs their build.sh; the SigmaStar one's spinor loop is `ssc377 ssc377d
ssc377de ssc377qe ssc378de ssc378qe` against include/configs/infinity6c.h,
which includes configs/sstar-common.h. That header is the whole environment
every SSC3xx NOR camera boots with:

    kernaddr=0x50000   kernsize=0x200000
    rootaddr=0x250000  rootsize=0x500000   rootmtd=5120k
    uknor / urnor / ubnor
    CONFIG_BOOTARGS "... mtdparts=NOR_FLASH:256k(boot),64k(env),2048k(kernel),
                         ${rootmtd}(rootfs),-(rootfs_data) ..."

A repo-wide grep for uknor8m|uknor16m|urnor8m|urnor16m|setnor8m|setnor16m|
mtdpartsnor returns nothing in either repo. u-boot-ingenic's isvp_common.h
carries the same unsuffixed uknor/urnor.

**The 16MB image cannot boot.** One mtdparts string, a 2048k kernel inside it,
and only ${rootmtd} varying: the rootfs starts at 0x250000 at every chip size.
Handing these two NOR[16] put it at 0x350000. The image openipc.org serves for
ssc377qe today has "hsqs" at 0x350000 and 0xff at 0x250000, and its env region
at 0x40000 is blank -- a full-image flash erases the env, so the camera comes up
on the compiled-in bootargs, looks for the rootfs at 0x250000, finds erased
flash and panics on root mount. The 8MB-layout image of the same page has it at
0x250000 and boots. Since #130 the 16MB layout is what a 16MB chip selects by
default, so this is what a SigmaStar visitor gets by pressing the button.

**None of the macros exist.** `run setnor16m`, `run uknor16m; run urnor16m` and
the printenv hint under them name variables no SigmaStar or Ingenic camera has.
U-Boot answers `## Error: "uknor16m" not defined` and flashes nothing, with no
failure the reader can see. This predates #78 -- the expert section has emitted
suffixed macros for these vendors since it existed.

So the pin was right for these two, even though its own comment gave a wrong
reason for it. What replaces it is the same fact stated properly: they have one
mtdparts, so 16MB means rootmtd=10240k at the same offset rather than a rootfs
1MB further up, and their macros are `uknor`/`urnor` with nothing after them.

That keeps Ultimate reachable, which a flat pin to the 8MB numbers would not:
its rootfs is 7832KB on ssc338q, 7252KB on ssc30kq and 6772KB on t31 against the
5120KB rootmtd defaults to. rootmtd is a plain variable, saved unexpanded and
expanded at boot by `cmdnor`, so the step that a macro does elsewhere is a
setenv here:

    setenv rootmtd 10240k; setenv rootsize 0xA00000
    saveenv
    reset

and where the layout is already the bootloader's own, that step is dropped
rather than rendered as a `run setnor8m` under a heading calling it required.

@flash_type_command is gone with it. It was the layout name, which is the macro
suffix only for the bootloaders that name their macros after the layout, and the
view read it in four places -- so the offsets and the commands could disagree.
Camera answers both now, off the same suffix.

* Assert the 16MB agreement per vendor rather than once for everybody

The test that pinned every vendor to 0x350000 was written when every vendor
had the same answer. It still checks the thing it was for -- that the image and
the page cannot describe different layouts -- but reads the offset each
bootloader is actually on.

A model of its own per vendor with it: the cache filename carries the model and
not the vendor, so one model built under three vendors has the second and third
read back the first's image. Nothing in production can hit that, a model
belongs to one vendor, but a test that did would pass on a stale file.
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.

1 participant