Giant match statements in flash xtask are fragile, error-prone #1886

cbiffle · 2024-10-01T17:11:09Z

Currently in build/xtask/src/flash.rs we have giant match statements that derive properties of boards based on hardcoded names. They look like this:

    let mut flash = match board {
        "lpcxpresso55s69" | "rot-carrier-1" | "rot-carrier-2"
        | "oxide-rot-1" => {
           // stuff
        }

        "stm32f3-discovery" | "stm32f4-discovery" | "nucleo-h743zi2"
        | "nucleo-h753zi" | "gemini-bu-1" | "gimletlet-1" | "gimletlet-2"
        | "gimlet-b" | "gimlet-c" | "gimlet-d" | "gimlet-e" | "psc-b"
        | "psc-c" | "sidecar-b" | "sidecar-c" | "sidecar-d"
        | "stm32g031-nucleo" | "donglet-g030" | "donglet-g031"
        | "oxcon2023g0" | "stm32g070-nucleo" | "stm32g0b1-nucleo"
        | "medusa-a" | "grapefruit" => {
           // things

This is inflexible and hard to maintain in ways that are actively causing us trouble. Case in point: in #1806 we accidentally removed "gimlet-f" from the match (when resolving a merge conflict with the change that added it) and neither Aaron nor I noticed, because honestly how would you. In #1885 we're restoring it.

While I was getting #1774 in over the course of about five months, I had repeated merge conflicts on this specific code, and spent more time than I'd like staring at it to make sure I didn't resolve them wrong.

Fortunately, in #1774 I also introduced the boards/ directory containing TOML board definitions. These files are currently empty. We should move the information being derived by these horrible match statements into the TOML files.

The text was updated successfully, but these errors were encountered:

Humility has historically had support for a --force-openocd flag on the flash subcommand, as a hedge against probe-rs having bugs or limited chip support. In practice, as far as I can tell, we never use it. This support required one of the three giant merge-conflict-prone match statements on which I declared war in #1886. The second such match statement was providing similar support for PyOCD, which Humility appears to have never actually implemented! So this change removes both. Archives built at or after this change can only be flashed (by Humility) using probe-rs. One giant scary match statement remains, but it's in my sights. I have left the openocd.cfg file in the archive because it's useful for debugging, even if we don't use it for _flashing._ (My workflow if I need gdb is typically to flash with Humility via probe-rs, and _then_ fire up openocd.) I've bumped the archive version here so that using such an archive with an older Humility gets you a cogent error. (Otherwise, you get a weird crash about loading flash.ron, no matter what you're trying to do.) Note that this requires a corresponding change in Humility, both to tolerate the absence of the removed fields, and to handle the new archive version. oxidecomputer/humility#514

Historically, the build system's flash support has contained a big scary match mapping board names to probe-rs chip names. This is super error prone, as I ranted about at length over in #1886. I recently introduced per-board configuration files as a way to (1) check that board names used in apps are not mispeld, and (2) store configuration like this. This change switches the build system to look in the toml files for the chip name, removing the final big scary match statement. Note that this information is _more specific_ than the information in chip.toml, because probe-rs is weirdly fanatical about specifying chip type (up to and including the package!), whereas Hubris's peripheral maps don't care. Fixes #1886.

Humility has historically had support for a --force-openocd flag on the flash subcommand, as a hedge against probe-rs having bugs or limited chip support. In practice, as far as I can tell, we never use it. This support required one of the three giant merge-conflict-prone match statements on which I declared war in #1886. The second such match statement was providing similar support for PyOCD, which Humility appears to have never actually implemented! So this change removes both. Archives built at or after this change can only be flashed (by Humility) using probe-rs. One giant scary match statement remains, but it's in my sights. I have left the openocd.cfg file in the archive because it's useful for debugging, even if we don't use it for _flashing._ (My workflow if I need gdb is typically to flash with Humility via probe-rs, and _then_ fire up openocd.) I've bumped the archive version here so that using such an archive with an older Humility gets you a cogent error. (Otherwise, you get a weird crash about loading flash.ron, no matter what you're trying to do.) Note that this requires a corresponding change in Humility, both to tolerate the absence of the removed fields, and to handle the new archive version. oxidecomputer/humility#514

Historically, the build system's flash support has contained a big scary match mapping board names to probe-rs chip names. This is super error prone, as I ranted about at length over in #1886. I recently introduced per-board configuration files as a way to (1) check that board names used in apps are not mispeld, and (2) store configuration like this. This change switches the build system to look in the toml files for the chip name, removing the final big scary match statement. Note that this information is _more specific_ than the information in chip.toml, because probe-rs is weirdly fanatical about specifying chip type (up to and including the package!), whereas Hubris's peripheral maps don't care. Fixes #1886.

Humility has historically had support for a --force-openocd flag on the flash subcommand, as a hedge against probe-rs having bugs or limited chip support. In practice, as far as I can tell, we never use it. This support required one of the three giant merge-conflict-prone match statements on which I declared war in #1886. The second such match statement was providing similar support for PyOCD, which Humility appears to have never actually implemented! So this change removes both. Archives built at or after this change can only be flashed (by Humility) using probe-rs. One giant scary match statement remains, but it's in my sights. I have left the openocd.cfg file in the archive because it's useful for debugging, even if we don't use it for _flashing._ (My workflow if I need gdb is typically to flash with Humility via probe-rs, and _then_ fire up openocd.) I've bumped the archive version here so that using such an archive with an older Humility gets you a cogent error. (Otherwise, you get a weird crash about loading flash.ron, no matter what you're trying to do.) Note that this requires a corresponding change in Humility, both to tolerate the absence of the removed fields, and to handle the new archive version. oxidecomputer/humility#514

Historically, the build system's flash support has contained a big scary match mapping board names to probe-rs chip names. This is super error prone, as I ranted about at length over in #1886. I recently introduced per-board configuration files as a way to (1) check that board names used in apps are not mispeld, and (2) store configuration like this. This change switches the build system to look in the toml files for the chip name, removing the final big scary match statement. Note that this information is _more specific_ than the information in chip.toml, because probe-rs is weirdly fanatical about specifying chip type (up to and including the package!), whereas Hubris's peripheral maps don't care. Fixes #1886.

Humility has historically had support for a --force-openocd flag on the flash subcommand, as a hedge against probe-rs having bugs or limited chip support. In practice, as far as I can tell, we never use it. This support required one of the three giant merge-conflict-prone match statements on which I declared war in #1886. The second such match statement was providing similar support for PyOCD, which Humility appears to have never actually implemented! So this change removes both. Archives built at or after this change can only be flashed (by Humility) using probe-rs. One giant scary match statement remains, but it's in my sights. I have left the openocd.cfg file in the archive because it's useful for debugging, even if we don't use it for _flashing._ (My workflow if I need gdb is typically to flash with Humility via probe-rs, and _then_ fire up openocd.) I've bumped the archive version here so that using such an archive with an older Humility gets you a cogent error. (Otherwise, you get a weird crash about loading flash.ron, no matter what you're trying to do.) Note that this requires a corresponding change in Humility, both to tolerate the absence of the removed fields, and to handle the new archive version. oxidecomputer/humility#514

cbiffle added developer-experience Fixing this would have a positive impact on developer experience build Affects or requires changes in the build system labels Oct 1, 2024

cbiffle closed this as completed in 7819a76 Oct 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Giant match statements in flash xtask are fragile, error-prone #1886

Giant match statements in flash xtask are fragile, error-prone #1886

cbiffle commented Oct 1, 2024

Giant match statements in flash xtask are fragile, error-prone #1886

Giant match statements in flash xtask are fragile, error-prone #1886

Comments

cbiffle commented Oct 1, 2024