Answer the review: three ways the split was still wrong

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.

Author: openipc-ai

app/controllers/cameras/socs_controller.rb

@@ -342,10 +342,7 @@ def enforce_eight_meg_limit
         @camera.firmware_version = 'lite'
         flash.now[:warning] = eight_meg_warning
       else
-        flash.now[:alert] =
-          'The Ultimate edition does not fit an 8MB flash chip, and OpenIPC publishes no Lite build ' \
-          'for this SoC on NOR. These instructions cannot produce a working camera on 8MB flash -- ' \
-          'this SoC needs a larger chip.'
+        flash.now[:alert] = no_lite_for_eight_meg_alert
       end
     end
 
@@ -355,6 +352,24 @@ def eight_meg_rootfs_with_ultimate?
       @camera.partition_layout.eql?('nor8m') && @camera.firmware_version.eql?('ultimate')
     end
 
+    # "This SoC needs a larger chip" is the right advice for an 8MB part and the
+    # wrong advice for a 16MB one wearing the 8MB layout, where the chip is
+    # already big enough and the layout is the thing to change. The guard above
+    # reaches both since it became the layout's, so this has to tell them apart
+    # too -- it is the branch for a SoC published as Ultimate and nothing else,
+    # hi3516cv6xx and hi3519dv500, where there is no Lite to fall back to.
+    def no_lite_for_eight_meg_alert
+      unless @camera.flash_type.eql?('nor8m')
+        return 'The Ultimate edition does not fit the 8MB partition layout, and OpenIPC publishes ' \
+               'no Lite build for this SoC on NOR. Choose the 16MB layout, which this chip is big ' \
+               'enough for.'
+      end
+
+      'The Ultimate edition does not fit an 8MB flash chip, and OpenIPC publishes no Lite build ' \
+        'for this SoC on NOR. These instructions cannot produce a working camera on 8MB flash -- ' \
+        'this SoC needs a larger chip.'
+    end
+
     # The chip when the chip is what limits them, and the layout when it is the
     # layout: a 5120KB rootfs partition is a 5120KB rootfs partition whether the
     # part around it is 8MB or 32MB, and on the larger ones there is something

app/models/firmware.rb

@@ -233,10 +233,31 @@ def layout(uboot, kernel, rootfs, size)
     [
       Part.new('u-boot', uboot, 0, kernel_offset, 'the kernel offset'),
       Part.new('kernel', kernel, kernel_offset, rootfs_offset, 'the rootfs offset'),
-      Part.new('rootfs', rootfs, rootfs_offset, size, 'the end of the image')
+      Part.new('rootfs', rootfs, rootfs_offset, *rootfs_limit(size))
     ]
   end
 
+  # The rootfs may not run past its own partition, which on NOR is where the
+  # overlay starts and not the end of the image.
+  #
+  # The two were near enough the same number while the layout was the chip: an
+  # 8MB image laid out the 8MB way ends 0xb0000 past the rootfs partition, and
+  # the difference only mattered for a rootfs already too big for the partition
+  # to mount. 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 -- so `?fw_release=ultimate&flash_size=16&layout=8` would have
+  # written a 7MB rootfs from 0x250000 clean through rootfs_data, and the
+  # camera would have mounted a squashfs whose tail the overlay then formatted
+  # over.
+  #
+  # NAND keeps the end of the image, which is where its rootfs ends by
+  # construction: image_size is the rootfs offset plus the payload.
+  def rootfs_limit(size)
+    return [size, 'the end of the image'] if nand?
+
+    [nor_layout[:overlay_offset], 'the rootfs partition']
+  end
+
   # Build beside the destination and rename into place.
   #
   # This used to build in Dir.tmpdir and hand the result to FileUtils.mv. In

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

@@ -145,10 +145,25 @@
         return chip === 'nor8m' ? ['nor8m'] : ['nor8m', 'nor16m'];
     }
 
+    // The layout a chip wears unless it is told otherwise: the largest one it
+    // can hold, which is the last entry allowedLayouts returns.
+    function naturalLayout(chip) {
+        const allowed = allowedLayouts(chip);
+        return allowed.length ? allowed[allowed.length - 1] : '';
+    }
+
+    // Whether the visitor has settled the layout for themselves. Until they
+    // have, it follows the chip, so choosing NOR 16M gets the 16MB layout --
+    // which is what the single menu this replaced always produced.
+    //
+    // Without it the 8MB layout the form opens on survived every later chip
+    // change, because it is a legal choice on a larger part and so never
+    // tripped the not-allowed branch. Picking NOR 16M then quietly gave a 16MB
+    // camera 8MB partitions, and the edition limiter below took Ultimate away
+    // with it.
+    let layoutChosen = false;
+
     // Narrow the layout menu to the chip, and answer what is now selected.
-    // Landing on the last allowed entry rather than the first makes the
-    // chip's own layout the default, which is what this form produced when
-    // the two were a single field.
     function checkPartitionLayout() {
         const el = document.querySelector('#camera_partition_layout');
         const chip = document.querySelector('#camera_flash_type').value;
@@ -159,9 +174,17 @@
             const o = el.options.item(i);
             o.disabled = !allowed.includes(o.value);
         }
-        if (allowed.length && !allowed.includes(el.value)) el.value = allowed[allowed.length - 1];
+        if (allowed.length && !(layoutChosen && allowed.includes(el.value))) {
+            el.value = naturalLayout(chip);
+        }
 
-        return el.value;
+        // Nothing, rather than whatever the hidden menu happens to hold, when
+        // the chip has no NOR layout to choose. The menu keeps its value while
+        // it is hidden, so NAND was reading back `nor8m` and having Ultimate
+        // taken off it by a rootfs partition it does not have -- eleven of the
+        // sixteen boards with a NAND build are published as Ultimate and
+        // nothing else.
+        return allowed.length ? el.value : '';
     }
 
     function allowedEditions(chip, layout) {
@@ -232,6 +255,19 @@
 
     document.querySelector('#generate-mac-address').addEventListener('click', generateMacAddress);
     document.querySelector('#camera_flash_type').addEventListener('change', checkFlashSize);
-    document.querySelector('#camera_partition_layout').addEventListener('change', checkFlashSize);
+    document.querySelector('#camera_partition_layout').addEventListener('change', function () {
+        layoutChosen = true;
+        checkFlashSize();
+    });
+
+    // Settled before the first narrowing, and after the chip has: a page that
+    // opens on a layout that is not its chip's own got there from a permanent
+    // link carrying `part`, and that is as deliberate as using the menu. A link
+    // written before this field existed carries none, so it opens on the chip's
+    // own layout and means exactly what it always did.
+    useAnOfferedFlashType();
+    layoutChosen = allowedLayouts(document.querySelector('#camera_flash_type').value).length > 0 &&
+        document.querySelector('#camera_partition_layout').value
+            !== naturalLayout(document.querySelector('#camera_flash_type').value);
     checkFlashSize();
 </script>

test/controllers/socs_controller_test.rb

@@ -272,6 +272,49 @@ def submit(soc, flash_type, firmware_version: 'lite', partition_layout: nil, loc
     end
   end
 
+  # The other half of that rule, for a SoC with no Lite build to fall back to.
+  # "This SoC needs a larger chip" is right for an 8MB part and wrong here: the
+  # chip is 16MB and it is the layout on it that Ultimate does not fit.
+  test 'a SoC published only as Ultimate is told to change the layout, not the chip' do
+    soc = instructable_soc('TS3516EVE50')
+
+    with_release_index("openipc.#{soc.board}-nor-ultimate.tgz") do
+      submit(soc, 'nor16m', firmware_version: 'ultimate', partition_layout: 'nor8m')
+
+      assert_match 'Choose the 16MB layout, which this chip is big enough for', response.body
+      assert_no_match(/needs a larger chip/, response.body)
+    end
+  end
+
+  test 'an 8MB chip with no Lite build is still told it needs a larger one' do
+    soc = instructable_soc('TS3516EVE60')
+
+    with_release_index("openipc.#{soc.board}-nor-ultimate.tgz") do
+      submit(soc, 'nor8m', firmware_version: 'ultimate')
+
+      assert_match 'needs a larger chip', response.body
+    end
+  end
+
+  # The layout menu follows the chip until the visitor settles it themselves,
+  # so picking a larger chip does not leave the 8MB layout the form opens on
+  # sitting on it -- which would quietly hand a 16MB camera 8MB partitions and
+  # take Ultimate away with them.
+  test 'the layout menu follows the chip until the visitor picks one' do
+    soc = instructable_soc('TS3516EVE70')
+
+    with_release_index(*every_edition_for(soc)) do
+      get "/cameras/vendors/#{@vendor.to_param}/socs/#{soc.to_param}"
+
+      assert_response :success
+      assert_match(/el.value = naturalLayout\(chip\);/, response.body)
+      assert_match(/layoutChosen = true;/, response.body)
+      # ...and answers with no layout at all where there is none to choose, so
+      # a hidden menu still holding nor8m cannot take Ultimate off a NAND part.
+      assert_match(/return allowed.length \? el.value : '';/, response.body)
+    end
+  end
+
   test 'the permanent link carries the layout so it can be reopened' do
     soc = instructable_soc('TS3516EVE40')
 

test/models/firmware_test.rb

@@ -313,7 +313,7 @@ def leftover_temp_files(firmware)
 
   # --- a part too big for its slot ---
 
-  test 'a rootfs too large for the flash is refused rather than written past the end' do
+  test 'a rootfs too large for its partition is refused rather than written past the end' do
     # openipc-hi3516ev200-nor-ultimate-8mb.bin was found in the production
     # cache at 9,465,856 bytes -- 0x250000 plus a 7MB Ultimate rootfs, in a
     # file whose name promises 8MB. IO.binwrite past the end grows the file
@@ -327,15 +327,34 @@ def leftover_temp_files(firmware)
     assert_not File.exist?(fw.filepath), 'an image larger than its flash must not be left to be served'
   end
 
-  test 'a rootfs that exactly fills the flash is still built' do
-    exact = "\xC3".b * (8.megabytes - 0x250000)
+  test 'a rootfs that exactly fills its partition is still built' do
+    # 0x250000 to 0x750000 -- the 5120KB the 8MB mtdparts gives the rootfs, not
+    # the 0x5b0000 that is left before the end of an 8MB image. The last
+    # 0xb0000 of the chip is rootfs_data, and a squashfs written into it is one
+    # the kernel cannot mount anyway: root is mtdblock3, which is 5120KB long.
+    exact = "\xC3".b * (0x750000 - 0x250000)
     fw = build(model: 'hi3516ev200', vendor: 'HiSilicon', flash_type: 'nor', size: 8,
                members: { 'uImage.hi3516ev200' => KERNEL, 'rootfs.squashfs.hi3516ev200' => exact })
     fw.generate
 
     assert_equal 8.megabytes, File.size(fw.filepath)
   end
 
+  # Room in the image is not room in the partition. This one fits a 16MB chip
+  # twice over and does not fit the 8MB layout that was asked for with it --
+  # and download_full_image takes both straight from the query string, so
+  # nothing upstream of here refuses the combination.
+  test 'a rootfs that fits the chip but not its partition is refused' do
+    oversize = "\xC3".b * (0x750000 - 0x250000 + 1)
+    fw = build(model: 'hi3516ev204', vendor: 'HiSilicon', flash_type: 'nor', size: 16, layout: 8,
+               members: { 'uImage.hi3516ev204' => KERNEL, 'rootfs.squashfs.hi3516ev204' => oversize })
+
+    error = assert_raises(Firmware::PayloadTooLarge) { fw.generate }
+    assert_match(/rootfs/, error.message)
+    assert_match(/the rootfs partition/, error.message)
+    assert_not File.exist?(fw.filepath)
+  end
+
   test 'a kernel that would run into the rootfs is refused' do
     oversize = "\xA5".b * (0x250000 - 0x50000 + 1)
     fw = build(model: 'hi3516ev200', vendor: 'HiSilicon', flash_type: 'nor', size: 8,