Post-Merge Cleanup for Machine Code Monitor

[chrisgleissner]

Machine Monitor — Post-merge Cleanup & Architecture Follow-ups

Follow-up issue tracking review feedback from @GideonZ on the machine monitor PR (merged into test-merge, not yet on master).

To avoid merge conflicts and to keep things simple, it is probably best to address these issues as follows:

1. Merge test-merge into master, possibly after addressing the most important issues shown below.
2. Branch off a new branch from master and fix remaining issues mentioned below.

---

1. UIPathPicker should be folded into TreeBrowser

File: software/userinterface/path_picker.h / path_picker.cc

The path picker is a thin wrapper around TreeBrowser that re-implements window setup, key handling and a text-entry fallback. TreeBrowser is already abstract enough to host this — it isn't file-specific despite its name.

Recommended solution

• Delete UIPathPicker as a separate UIObject.
• Express "pick a path" as a configuration of TreeBrowser itself (e.g. a PICK_PATH extension of the existing PickMode enum, plus the text-entry overlay as an internal mode of the browser).
• Callers (FTP Move/Copy-to, monitor Save dialog, future consumers) construct a TreeBrowser directly with that mode rather than instantiating a parallel picker class.

This removes the duplicated init/poll/keymap plumbing and keeps a single authoritative entry point for tree-based selection.

---

2. Browsable::getFileInfo() does not belong on the abstract base

File: software/userinterface/browsable.h

Browsable represents any line in the line-based browser — not necessarily a file. virtual FileInfo *getFileInfo() therefore leaks a file concept into the base class, and isPathPickerWrapper() is a similar abstraction leak
added by the picker work.

Recommended solution

• Move getFileInfo() off Browsable and onto a dedicated interface/subclass (e.g. BrowsableFileEntry) implemented only by browsables that actually back a FileInfo.
• Callers that currently call entry->getFileInfo() perform a typed downcast / dynamic_cast-style check instead, so the type system enforces the distinction.
• Remove isPathPickerWrapper() once the picker is folded into TreeBrowser (item 1) — the wrapper class disappears with it.

@GideonZ offered to take this on; documenting here so it is tracked either way.

---

3. Context menu should not reach into TreeBrowser state

File: software/userinterface/context_menu.cc:55-62

The context menu currently inspects state->browser->pick_mode, state->browser->can_pick(...) and contextable->pickAsCurrentPath() to decide whether to inject a synthetic "Select" action. This couples the context menu to the browser's pick state, when by design it should depend only on the Browsable under the cursor.

Recommended solution

• Introduce a BrowsablePickEntry (or similar) type that exposes its own fetch_context_items(...) returning the appropriate "Select" action.
• The context menu calls contextable->fetch_context_items(actions) and is done — no knowledge of pick_mode or the browser.
• The picker (item 1) wraps/promotes entries into BrowsablePickEntry while building the list, so the action only appears when picking is actually active.

Result: ContextMenu :: get_items() collapses back to a single contextable->fetch_context_items(actions) call.

---

4. machine_monitor_actions[256] registry is the wrong shape

Files: software/monitor/monitor_init.cc:20-65, monitor_init.h, software/test/monitor/machine_monitor_test.cc:82-101

The current implementation is a 256-entry Action * array indexed by subsys_id, with register_machine_monitor_task / get_machine_monitor_task guarded by __attribute__((weak)) so the test build can shadow them.

@GideonZ's concern: this isn't congruent with how subsys_ids are intended to be used. SubSystem already maintains a registry (SubSystem::getSubSystems()), and ids are sparse and few (currently 12).

A 256-slot sidecar array indexed by the same ids is essentially a parallel registry that bypasses the existing SubSystem machinery.

Recommended solution

• Replace the sidecar array with a small, fixed lookup that mirrors the shape of the subsystem registry — e.g. a single Action * per machine type (C64 / U64), stored where the machine itself is constructed, and resolved through C64::getMachine() rather than a numeric id.
• The caller in tree_browser.cc:447-459 then asks the current machine for "your monitor action" instead of probing SUBSYSID_U64 then falling back to SUBSYSID_C64.
• This also removes the need for __attribute__((weak)) and the duplicated test stub in machine_monitor_test.cc.

---

5. Test stub string_box overloads have unused parameters

File: software/test/stubs/userinterface.h:65-80

int  string_box(const char *msg, char *buffer, int maxlen, bool)         { ... }
int  string_box(const char *msg, char *buffer, int maxlen, bool, bool)   { ... }

The stubs match the real overload set (template_mode, uppercase) but silently discard the flags. That's misleading: tests that exercise template or uppercase behavior pass through unchanged.

Recommended solution

• Either implement minimal honest semantics in the stub (e.g. uppercase input when uppercase=true), or
• Remove the extra overloads and have the production code that needs them use a single signature. The stub then matches that signature directly.

The unused-parameter overloads should not stay as-is.

---

6. U64 PLA / memory-mapping emulation is too clunky (architectural)

File: software/u64/u64_machine.cc

@GideonZ's summary of the current arbitration / decode pipeline:

 CPU -> PIO ---------------------------\
       CPU -> bus -> |\                 |
       VIC -> bus -> | \                v
 DirectDMA -> bus -> | arbitration -> decode (PLA) -> CPU view
 ExternDMA -> bus -> |/                 ^
 External Pins (EXROM/GAME) ------------/

DirectDMA has a flag that forces the decode block to end up in RAM; what actually puts the system "in the game" is mostly the external EXROM/GAME pins.

To peek/poke under the CPU's view, the monitor today emulates the PLA in software (see read_cpu_mapped_byte / write_cpu_mapped_byte in u64_machine.cc) — duplicating banking logic that already exists in hardware. This is hard to keep correct and complete.

Recommended solution (Gideon's proposal)

Pull the external pins (EXROM/GAME) and the PIO pins before arbitration and make them programmable on the DirectDMA port. Each bus agent then gets its own view of memory, interleaved cleanly:

• The application drives "virtual" EXROM/GAME/PIO levels on the DirectDMA port, independent of the real system-level pins.
• The existing decode (PLA) block then produces the correct view for that agent automatically — no software re-implementation of banking.
• read_cpu_block / read_cpu_mapped_byte / write_cpu_mapped_byte and the cpu_port plumbing in U64Machine collapse to "set virtual pins, do a regular DMA read/write".

This is a gateware change (with a software follow-up to remove the PLA emulation), so it is scoped as its own piece of work. Filing here for tracking.

---

7. override_cpu_port does not work via DMA

File: software/u64/u64_machine.cc:37-47

override_cpu_port writes $0000 / $0001 to swing the CPU port over the DMA bus before a peek/poke. Per @GideonZ: the CPU port is not accessible through DMA. The current sequence reads back whatever is in zero-page RAM and writes there, but the CPU port itself is unchanged.

Recommended solution

• Remove override_cpu_port / restore_cpu_port and the cpu_port parameter on peek_cpu / poke_cpu / read_cpu_block in their current form.
• Once item 6 lands, the CPU view is selected by feeding virtual PIO pins into the bus subsystem at DirectDMA — no zero-page mutation required.
• Until then, document the limitation in the monitor (the "CPU view" peek/poke produces the value the PLA emulator computes, not what the real CPU sees after toggling the port).

---

8. C64_DMA_MEMONLY is not readable

File: software/u64/u64_machine.cc — every saved_memonly = C64_DMA_MEMONLY site

The register is write-only. The sequence

uint8_t saved_memonly = C64_DMA_MEMONLY;   // always 0
C64_DMA_MEMONLY = 0;
...
C64_DMA_MEMONLY = saved_memonly;           // always restores 0

works by accident today because callers always want it back at 0 after the access. It is, however, misleading and brittle.

Recommended solution

• Drop the read-back entirely. before_memory_access / after_memory_access already own the lifetime (C64_DMA_MEMONLY = ... on entry, = 0 on exit) - let them be the single source of truth.
• Remove the unused saved_memonly locals in read_visible_block, peek_visible, write_visible_byte, and read_visible_byte.

---

9. C64_MODE_NMI jump won't work in External Slot mode

File: software/monitor/monitor_file_io.cc:230-272 (monitor_io::jump_to)

@GideonZ noted that using C64_MODE_NMI to trigger the jump trampoline won't work when the machine is in External Slot mode. Today the monitor "jump to" feature is fine for normal/cartridge modes but will silently misbehave with an external cartridge.

Recommended solution

• Short-term: detect External Slot mode in jump_to and either refuse the operation with a clear message, or fall back to the SUBSYSID_C64 / C64_DMA_BUFFER / RUNCODE_DMALOAD_JUMP path that the non-U64 branch already uses.
• Long-term: revisit alongside breakpoint support — at that point a more general "run from address" primitive should subsume both paths.

---

Suggested ordering

1. Items 5, 8, 9 — small, local, no design dependencies.
2. Item 4 — registry shape; clears __attribute__((weak)) plumbing.
3. Items 1 + 2 + 3 — done together; the Browsable / context-menu cleanup makes the picker fold-in straightforward.
4. Items 6 + 7 — gateware-led; software cleanup follows.

[chrisgleissner]

Hi @GideonZ ,

this is a summary of PR comments you left on #358 so we can track it and implement it in future.

In case an wording needs to be improved or in case you prefer a different implementation, please edit as desired.

If Github for some reason prevents edits of the original issue, please comment below and I will incorporate it into the issue.

Thanks
Christian