Revamp how attaching works

[mkeeter]

(Staged on #651)

Note: A bunch of the LOC changes are in the test suite, so you may want to just look at the first commit when reviewing. The logic changes are a significant net code reduction (+616/-1144), but a few of the test suite commands now successfully run where they previously exited with an error (+1147/-358), which accounts for the overall LOC increase.

Overview

The CommandKind type has confusing, non-orthogonal behavior.

It combines two ideas:

• Do we need a HubrisArchive, and if so, does it need to be fully loaded ("cooked") or do we just need the raw ZIP data?
• Do we need to attach to a system (called a "core")?
  • If so, what kind of attachments are valid?
    • Live system (debugger)
    • Live system (network)
    • Dump (RAM)
    • Archive (flash only)
  • How should we validate any attached system?

These two big ideas ("do we need an archive" and "do we need a core") are combined in unintuitive ways. For example, CommandKind::Unattached { archive: Archive::Required } tries to attach to both a dump and an archive, but does not fail if the archive is absent (see #564 (comment)).

It's also unfortunate that it's defined at the Command level, because commands may not have consistent behavior across all of their subcommands. For example, humility hiffy -l just lists Hiffy APIs from an archive, and does not require attaching to a system. Indeed, if you run humility hiffy -l with an archive and you have a probe attached to an SP running a different image, it will fail:

$ humility --archive=build-cosmo-b-dev-image-default.zip hiffy -l
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.15s
     Running `target/debug/humility hiffy -l`
humility: attached via ST-Link V3
humility hiffy failed: image ID in archive ([bd, 1b, fb, 72, 35, 4b, a7, 48]) does not equal ID at 0x8004f50 ([7c, 56, 0, 8, 38, 54, 0, 8])
# this is an attached Grapefruit, why is it interfering with hiffy -l?

Some commands work around this by doing an extra parse of the arguments in their init function, then return a different Command depending on arguments:

humility/cmd/monorail/src/lib.rs

Lines 1186 to 1221 in 27d1f15

	// We do a bonus parse of the command-line arguments here to see if we're
	// doing a `monorail info` subcommand, which doesn't require a Hubris image
	// or attached device; skipping those steps improves runtime (especially
	// in debug builds)
	let subcmd_attached = humility_cmd::Command {
	app: MonorailArgs::command(),
	name: "monorail",
	run: monorail,
	kind: CommandKind::Attached {
	archive: Archive::Required,
	attach: Attach::LiveOnly,
	validate: Validate::Booted,
	},
	};
	let subcmd_unattached = humility_cmd::Command {
	app: MonorailArgs::command(),
	name: "monorail",
	run: monorail_get_info,
	kind: CommandKind::Unattached { archive: Archive::Ignored },
	};
	// If there's a `monorail` subcommand, then attempt to parse the subcmd
	let mut args = std::env::args().skip_while(|a| a != "monorail").peekable();
	if args.peek().is_some() {
	if let Ok(args) = MonorailArgs::try_parse_from(args) {
	if !args.requires_target() {
	return subcmd_unattached;
	}
	} else {
	// If the argument parse failed, then return the faster subcommand
	// so that we don't have to attach to a device then fail parsing
	// again.
	return subcmd_unattached;
	}

This works, but is annoying to have to do!

I have one more complaint, which is improved but not entirely fixed in this PR: the HubrisArchive is default-constructed, then actually populated based on archive or dump arguments; if neither is present, then archive is not valid! Commands have to work around this (with varying degrees of incredulity), e.g.

humility/cmd/reset/src/lib.rs

Lines 33 to 37 in 27d1f15

	// `context.archive` is always `Some(..)` even if we have not specified
	// anything on the command line or through environment flags. However, if no
	// archive is specified, then the actual data in the archive is default
	// constructed (??!).
	let chip = context.archive.as_mut().unwrap().chip();

So, what's to be done?

This PR removes CommandKind entirely. It is replaced with a set of functions on Cli to build archives and cores, which can be called by commands when they are needed (instead of before the command is invoked).

Archive handling

• archive returns the archive (if available); it does not ever return a default-constructed archive
• try_archive returns an optional archive
• raw_archive returns the bytes of the ZIP file

All of these return a full HubrisArchive; it is the caller's responsibility to store it somewhere and hand out references.

Attachment

• attach_live attaches to a live system (debugger or net-based)
• attach_live_or_dump attaches to a live system or dump
• attach_archive attaches to an archive

There are also additional small wrapper functions with suffixes for validation, e.g. attach_live_booted checks that the live system has booted. Each of these functions returns a Box<dyn Core> directly, instead of stashing it in the ExecutionContext.

Changes to commands

Each command is updated to construct its archive and/or core on-demand. It's mostly mechanical porting, e.g.

CommandKind::Attached {
    archive: Archive::Required,
    attach: Attach::LiveOnly,
    validate: Validate::Booted,
}

becomes

    let hubris = &context.cli.archive()?;
    let core = &mut *context.cli.attach_live_booted(hubris)?;

...but there are a few weird edge cases – for example, humility probe now uses attach_probe directly.

[mkeeter]

Do we care about the ability to dump from a dump? Probably not, but it was previously possible.

[labbott]

I feel like if we actually want that we're probably going to want other tooling

[labbott]

I think this looks okay I want to take another look in a bit

[labbott]

I guess these were vestigial op_start/op_done?

[mkeeter]

Yeah, and even more vestigal now that we're reading from the archive directly (which doesn't have a core to start and stop).