[1.4] Fix Navigator not being detected on the first boot of a Bookworm install

.github/workflows/test-and-deploy.yml

@@ -303,6 +303,20 @@ jobs:
 
       - name: Add /boot additions
         run: |
+          # configure_board.sh dispatches on the host device tree, so the board configuration the image
+          # must end up with is decided by the runner's own hardware, not by the image it builds.
+          case "${{ matrix.runner }}" in
+            pi5-builder)
+              EXPECTED=("[pi5]" "dtoverlay=i2c3-pi5,baudrate=400000" "dtoverlay=spi1-3cs")
+              ;;
+            blueos-ci|pi4-builder2)
+              EXPECTED=("[pi4]" "dtoverlay=i2c4,pins_6_7,baudrate=1000000" "dtoverlay=spi1-3cs")
+              ;;
+            *)
+              echo "Unknown runner ${{ matrix.runner }}, add the board configuration its image must contain."
+              exit 1
+              ;;
+          esac
           sudo apt-get update && sudo apt-get install -y parted kpartx
           # Create mount point if it doesn't exist
           sudo mkdir -p /mnt/piboot
@@ -312,8 +326,27 @@ jobs:
           sudo mount "/dev/mapper/${LOOP_DEVICE}p1" /mnt/piboot
           # Create ssh and userconf files
           sudo cp install/boards/config.toml /mnt/piboot/custom.toml
+          # A board install script writing to the /boot stub instead of the boot partition still exits 0,
+          # so the image itself is the only place where a missing Navigator configuration can be caught.
+          MISSING=()
+          for LINE in "${EXPECTED[@]}"; do
+            sudo grep -qxF "$LINE" /mnt/piboot/config.txt || MISSING+=("config.txt: $LINE")
+          done
+          # cmdline.txt is reached through its own path and can be left behind on its own. Whatever
+          # board this is, the memory cgroup docker needs is enabled and the serial console that
+          # would hold the autopilot port is dropped.
+          sudo grep -q "cgroup_enable=memory" /mnt/piboot/cmdline.txt || MISSING+=("cmdline.txt: cgroup_enable=memory")
+          if sudo grep -q "console=serial" /mnt/piboot/cmdline.txt; then
+            MISSING+=("cmdline.txt: console=serial is still there")
+          fi
           sudo umount /mnt/piboot
           sudo kpartx -d deploy/pimod/blueos.img
+          if [ ${#MISSING[@]} -ne 0 ]; then
+            echo "Missing from the boot partition of the ${{ matrix.os }} ${{ matrix.platform }} image built on ${{ matrix.runner }}:"
+            printf '  %s\n' "${MISSING[@]}"
+            echo "The board install script did not configure the image's boot partition."
+            exit 1
+          fi
           echo "Boot partition updated successfully."
 
       - name: Sanitize platform name

core/tools/blueos_startup_update/blueos_startup_update.py

@@ -7,7 +7,7 @@
 import time
 import subprocess
 from pathlib import Path
-from typing import List, Tuple
+from typing import List, Optional, Tuple
 import configparser
 
 import appdirs
@@ -23,6 +23,10 @@
 
 BOOT_LOOP_DETECTOR = "/root/.config/.boot_loop_detector"
 
+# First line of the plain text files Bookworm leaves at /boot once the boot partition moved to
+# /boot/firmware. The install scripts match the same line, see install/boards/bcm_27xx.sh
+BOOT_STUB_MARKER = "DO NOT EDIT THIS FILE"
+
 # Any change made in this DELTA_JSON dict should be also made
 # into /bootstrap/startup.json.default too!
 DELTA_JSON = {
@@ -99,8 +103,8 @@ def boot_config_get_or_append_section(config_content: List[str], section_name: s
         (i for (i, line) in enumerate(config_content) if re.match(section_match_pattern, line, regex_flags)), None
     )
     if section_start_line_number is None:
-        config_content.append(f"\n[{section_name}]")
-        section_start_line_number = len(config_content)
+        config_content.extend(["", f"[{section_name}]"])
+        section_start_line_number = len(config_content) - 1
 
     any_section_match_pattern = r"^\[.*\].*$"
     section_end_line_number = next(
@@ -117,20 +121,62 @@ def boot_config_get_or_append_section(config_content: List[str], section_name: s
     return (section_start_line_number, section_end_line_number)
 
 
+def boot_config_merge_duplicated_sections(config_content: List[str], section_name: str) -> None:
+    regex_flags = re.IGNORECASE | re.DOTALL | re.MULTILINE
+
+    section_match_pattern = r"^\[" + re.escape(section_name) + r"\].*$"
+    any_section_match_pattern = r"^\[.*\].*$"
+
+    # A board filter is applied until the next one, so config.txt is a run of sections, each starting
+    # at its header, with the configuration that applies to every board ahead of the first header
+    sections: List[List[str]] = [[]]
+    for line in config_content:
+        if re.match(any_section_match_pattern, line, regex_flags):
+            sections.append([])
+        sections[-1].append(line)
+
+    duplicated = [
+        index
+        for (index, section) in enumerate(sections[1:], start=1)
+        if re.match(section_match_pattern, section[0], regex_flags)
+    ]
+    if len(duplicated) < 2:
+        return
+
+    # A duplicate header just reopens the same filter, so every stray's configuration is already in
+    # force, and the firmware applies whatever comes last. The strays are emptied into the end of the
+    # first section's run of non-blank lines, which keeps that order and keeps them where the helpers
+    # above, that read a section as ending at the first blank line, will find them
+    (survivor, *strays) = duplicated
+    body_end = next((index for index, line in enumerate(sections[survivor]) if index > 0 and line == ""), None)
+    if body_end is None:
+        body_end = len(sections[survivor])
+    sections[survivor][body_end:body_end] = [line for index in strays for line in sections[index][1:] if line != ""]
+    for index in strays:
+        sections[index] = []
+
+    config_content[:] = [line for section in sections for line in section]
+
+
 def boot_config_add_configuration_at_section(config_content: List[str], config: str, section_name: str) -> None:
     regex_flags = re.IGNORECASE | re.DOTALL | re.MULTILINE
 
     (section_start, section_end) = boot_config_get_or_append_section(config_content, section_name)
 
     section_content = config_content[section_start:section_end]
     config_already_exists = any(
-        section_content for section_content in section_content if re.match(config, section_content, regex_flags)
+        section_content
+        for section_content in section_content
+        if re.match(re.escape(config), section_content, regex_flags)
     )
     if not config_already_exists:
         config_content.insert(section_start + 1, config)
 
 
 def boot_config_remove_section(config_content: List[str], section_name: str) -> None:
+    if section_name not in boot_config_get_available_section(config_content):
+        return
+
     (section_start, section_end) = boot_config_get_or_append_section(config_content, section_name)
     del config_content[section_start:section_end]
 
@@ -168,7 +214,7 @@ def boot_config_filter_conflicting_configuration_at_section(
                 # ...except...
                 and not (
                     # ...if it's the correct one....
-                    re.match(f"^{config}.*$", line, regex_flags)
+                    re.match(f"^{re.escape(config)}.*$", line, regex_flags)
                     # ...and lives inside the correct section.
                     and (section_start < i < section_end)
                 )
@@ -206,7 +252,7 @@ def boot_cmdline_add_modules(cmdline_content: List[str], config_key: str, desire
             cmdline_content.remove(cmdline_content[config_index])
 
     # Replace the first configs line with the combined, append if none
-    if first_config_line:
+    if first_config_line is not None:
         cmdline_content[first_config_line] = f"{config_key}=" + ",".join(desired_config)
     else:
         config_line = f"{config_key}=" + ",".join(desired_config)
@@ -354,7 +400,16 @@ def update_dwc2() -> bool:
 
     # Add dwc2 overlay in pi4 section if it doesn't exist
     dwc2_overlay_config = "dtoverlay=dwc2,dr_mode=otg"
-    section_name = "pi4" if get_cpu_type() == CpuType.PI4 else "pi5"
+    cpu_type = get_cpu_type()
+    if cpu_type == CpuType.PI4:
+        section_name = "pi4"
+    elif cpu_type == CpuType.PI5:
+        section_name = "pi5"
+    else:
+        # A board filter the firmware does not know is applied instead of ignored, so a [pi5]
+        # section would reach the pins of every board older than the Pi5
+        logger.error("Unsupported CPU type for dwc2 update")
+        return False
     boot_config_add_configuration_at_section(config_content, dwc2_overlay_config, section_name)
 
     # Remove any unprotected and conflicting dwc2 overlay configuration
@@ -443,6 +498,10 @@ def update_navigator_overlays() -> bool:
         logger.error("Unsupported CPU type for navigator overlays update")
         return False
 
+    # Devices patched by a release that appended a board section on every boot accumulated strays,
+    # and only the first of them would ever be configured
+    boot_config_merge_duplicated_sections(config_content, section_name)
+
     navigator_configs_with_match_patterns.reverse()
 
     for (config, config_match_pattern) in navigator_configs_with_match_patterns:
@@ -742,6 +801,24 @@ def setup_ssh() -> bool:
         return False
 
 
+def locate_boot_file(candidates: List[str]) -> Optional[str]:
+    """Locate a boot file, refusing the plain text stub Bookworm leaves at /boot once the boot
+    partition moved to /boot/firmware. Writing to that stub configures nothing, so the install
+    scripts refuse it by the same marker and the startup patches have to refuse it the same way,
+    or they write the board section into a file the firmware never reads and reboot into it."""
+    located = locate_file(candidates)
+    if located is None:
+        return None
+    # The marker is the stub's whole first line, and only that line, so a real config.txt that happens
+    # to carry the phrase in a comment keeps being patched. Read it with run_command rather than
+    # load_file: this runs before the run_command_is_working() check below, and load_file raises when
+    # the host connection is not up yet, which would take every patch down with it
+    if run_command(f'head -n 1 "{located}"', False).stdout.strip() == BOOT_STUB_MARKER:
+        logger.error(f"{located} is the Bookworm boot stub, the boot partition is not mounted")
+        return None
+    return located
+
+
 def main() -> int:
     start = time.time()
     # check if boot_loop_detector exists
@@ -755,9 +832,9 @@ def main() -> int:
     # pylint: disable=global-statement
     global config_file
     global cmdline_file
-    config_file = locate_file(["/boot/firmware/config.txt", "/boot/config.txt"])
+    config_file = locate_boot_file(["/boot/firmware/config.txt", "/boot/config.txt"])
     logger.info(f"config.txt found at {config_file}")
-    cmdline_file = locate_file(["/boot/firmware/cmdline.txt", "/boot/cmdline.txt"])
+    cmdline_file = locate_boot_file(["/boot/firmware/cmdline.txt", "/boot/cmdline.txt"])
     logger.info(f"cmdline.txt found at {cmdline_file}")
 
     if not run_command_is_working():