evaluation.md

@@ -3,65 +3,75 @@
 ## Summary Table
 
 | Category                 | Rating        | Remarks                                                                                                                                                                                                                                                                                                                                                                                                              |
-|--------------------------|---------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
-| Mocked / Left Undone     | ✅ Minor      | `CMD_STOP` is only reachable via the FIFO pipe (`"stop"`). The MIDI handler never sends it. This is an unused path, not a bug. The FIFO pipe is still untested in the integration suite – all tests use MIDI. No test for `"stop"` via FIFO. Everything else is implemented and matches the test suite. |
-| Potential Segfaults      | ✅ Good       | All `jack_port_get_buffer()` calls are guarded against NULL. Array bounds respected (fixed‑size loops). SPSC queues use modulo arithmetic, no overrun possible. No dynamic memory in RT path.                                                                                                                                            |
-| Memory Safety            | ✅ OK         | No leaks, no use‑after‑free. All buffers static. Deferred port unregistration waits for at least one RT cycle after `active=0` – safe. The FIFO reader thread uses stack memory for line reading.                                                                                                                                       |
-| Thread Safety / Race     | ✅ Good       | SPSC queues have correct `acquire`/`release` ordering. Multi‑producer issue on main‑loop commands has been fixed by giving each producer its own queue (`cmd_queue_main_midi` for MIDI, `cmd_queue_main_fifo` for FIFO). The RT callback only writes to `cmd_queue_main_midi`; the FIFO thread only writes to `cmd_queue_main_fifo`. Both are consumed solely by the main loop, restoring SPSC safety. The deferred unregistration race is now fixed via `global_rt_cycles` counter – the main loop ensures the RT thread has completed a cycle after `active=0` before calling `jack_port_unregister()`. `prev_state` is a plain `int` but accessed only from the RT callback (single thread). All other shared state (`state`, `active`, `control_key_active`, `bind_channel`) uses atomics. |
-| Performance              | ✅ Good       | No syscalls, locks, or dynamic allocations in the RT callback. Two queue drains (one for RT commands, one for main‑loop commands) add negligible overhead. O(1) queue operations. Linear audio processing.                                                                                                                              |
-| Architectural Soundness  | ✅ Good       | Clean separation: each input source has its own SPSC queue for non‑RT commands; RT callback drains its own queue; main loop drains both auxiliary queues. The command queue approach is now fully uniform (no atomic flags remaining for add/remove). The FIFO pipe works in parallel. The code is easily extensible to new input sources. |
+|--------------------------|---------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| Mocked / Left Undone     | ✅ Everything implemented | `CMD_STOP` is now sent from MIDI (note 65) and from FIFO (`"stop"`). FIFO pipe add/remove test is in the integration suite. All command types are wired to both sources. No missing paths.                                                                                                                                                                    |
+| Potential Segfaults      | ✅ Good       | Every `jack_port_get_buffer()` call is null‑checked. Array bounds respected (`MAX_CHANNELS`, `QUEUE_CAPACITY`). No `malloc`/`free` in RT path. The only unguarded `jack_port_get_buffer()` is in `midi_handle_events` where the caller already verified the buffer pointer – safe.                                                                                                                                  |
+| Memory Safety            | ✅ OK         | All buffers static, no dynamic allocation. Deferred port unregistration waits for at least one RT cycle after `active=0` (via `global_rt_cycles`), preventing use‑after‑unregister. FIFO reader uses stack‑allocated line buffer. No leaks.                                                                                                                                                                            |
+| Thread Safety / Race     | ✅ Good       | Three SPSC queues, each with a single producer: `cmd_queue` (MIDI handler only), `cmd_queue_main_midi` (MIDI handler only), `cmd_queue_main_fifo` (FIFO thread only). All consumers are single‑threaded (RT callback or main loop). Atomic ordering correct (`acquire`/`release`). `global_rt_cycles` prevents RT‑thread‑still‑using‑port race. All shared state (`state`, `active`, `control_key_active`, `bind_channel`) uses atomics. `prev_state` is a plain `int` but accessed only from the RT callback – safe. |
+| Performance              | ✅ Good       | No syscalls, locks, or allocations in RT callback. O(1) queue operations. Linear audio processing. The RT callback drains `cmd_queue` (usually 0–2 commands), processes per‑channel audio, and handles MIDI clock events. The main loop runs every 50 ms and drains two auxiliary queues – negligible overhead.                                                                                                          |
+| Architectural Soundness  | ✅ Good       | Clean separation: each input source has its own SPSC queue for non‑RT commands. RT callback performs only RT‑safe operations; main loop handles channel add/remove. All commands use a uniform `command_t` enum. The code is easily extensible – adding another input source (e.g., UDP socket) requires only a new SPSC queue and a drain loop.                                                                      |
 
 ## Detailed Remarks
 
 ### 1. Mocked / Left Undone
-- `CMD_STOP` is only reachable via the FIFO pipe (`"stop"`). The MIDI handler never sends it. This is not a bug, just an unused feature path.
-- The FIFO pipe is completely untested in the main integration suite. All existing tests use MIDI notes. Adding a FIFO‑based test would increase coverage.
-- No other functionality is missing.
+- **Nothing remaining.**  
+  - `CMD_STOP` is now sent by MIDI (note 65, control‑key section) and recognised by FIFO (`"stop"`).  
+  - FIFO pipe add/remove is tested in `test_fifo_pipe()`.  
+  - All other command types (`CYCLE`, `BIND`, `UNBIND`, `ADD_CHANNEL`, `REMOVE_CHANNEL`) are available from both MIDI and FIFO.  
 
 ### 2. Potential Segfaults
-- Every `jack_port_get_buffer()` call is followed by a null check (`if (!out) continue;`).
-- Array accesses are bounded by `MAX_CHANNELS` and `QUEUE_CAPACITY`.
-- No use of `malloc` or variable‑length arrays in the real‑time callback.
-- The only unguarded `jack_port_get_buffer()` is in `midi_handle_events` where the caller already checks `midi_ctrl_buf` against NULL. Safe.
+- Every `jack_port_get_buffer()` is followed by a null check.  
+- No array overruns: loops over `MAX_CHANNELS` (16) and `QUEUE_CAPACITY` (256).  
+- No dynamic memory in RT context.  
+- The only unchecked `jack_port_get_buffer()` is in `midi_handle_events` – the caller already ensures `midi_ctrl_buf` is not NULL.  
 
 ### 3. Memory Safety
-- All `loop_buffer` arrays and command queue buffers are static globals. No heap allocation in RT context.
-- Port unregistration is deferred until after the RT thread has surely passed the `active=0` check (via `global_rt_cycles`). No use‑after‑unregister possible.
-- The FIFO reader thread uses a stack‑allocated line buffer – safe.
-- No memory leaks are present.
+- All `loop_buffer` arrays and command queue buffers are static global arrays – no heap allocation.  
+- Port unregistration is deferred until `global_rt_cycles` has advanced by at least 1 after marking `active=0`. This guarantees the RT thread has started a new cycle after seeing `active=0`, so it will not dereference the port pointers after they are unregistered.  
+- FIFO reader thread uses a stack‑allocated `char line[256]` – safe.  
+- No memory leaks exist.  
 
 ### 4. Thread Safety / Race Conditions
-- **RT‑safe commands queue (`cmd_queue`)** – single writer (MIDI handler, called from RT callback) and single reader (the same callback, immediately after writing). Correct.
-- **Add/remove command queues** – two separate SPSC queues:
-  - `cmd_queue_main_midi`: written only by the RT callback (via `midi_handle_events`).
-  - `cmd_queue_main_fifo`: written only by the FIFO reader thread (non‑RT).
-  Both are read only by the main loop (single consumer). No concurrent writes to the same queue.
-- `global_rt_cycles` is incremented with `memory_order_release` at the end of every process callback. The main loop reads it with implicit acquire. This ensures visibility of the store to `active` and prevents unregistering ports while the RT thread may still be using them.
-- `channel_add()` and `channel_remove()` are called only from the main loop, never from the RT callback. The RT callback reads `active`, `state`, `audio_in`, `audio_out` (all atomic). Safe.
-- `prev_state` is a plain `int` but written and read only from the RT callback – no data race.
+- **Three SPSC queues, each with a single writer and single reader:**  
+  - `cmd_queue` – writer: `midi_handle_events` (called from RT callback), reader: same RT callback (immediately after writing).  
+  - `cmd_queue_main_midi` – writer: RT callback (via `midi_handle_events`), reader: main loop.  
+  - `cmd_queue_main_fifo` – writer: FIFO reader thread, reader: main loop.  
+- All queue operations use correct `memory_order_acquire`/`release` – no data races.  
+- `global_rt_cycles` is incremented with `memory_order_release` at the end of every process callback. The main loop reads it with implicit acquire (via `atomic_load`). The condition `current_cycle - pending_unregister_cycle >= 1` ensures the RT thread has finished a cycle after `active=0` before port unregistration.  
+- `channel_add()` and `channel_remove()` are called only from the main loop. The RT callback reads `active`, `state`, `audio_in`, `audio_out` – all atomic. No concurrent modification.  
+- `prev_state` is a plain `int` but only accessed from the RT callback – safe.  
 
 ### 5. Performance
-- The RT callback performs:
+- The RT callback performs in order:  
   1. MIDI event processing (may push to `cmd_queue` and `cmd_queue_main_midi`).  
-  2. Drain `cmd_queue` (O(1) per command, usually 0–2 commands).
-  3. Per‑channel audio processing (linear buffer copy or playback).
-  4. MIDI clock event handling (rare).
-  5. Increment `global_rt_cycles` (atomic store).
+  2. Drain `cmd_queue` (usually empty or 1 command).  
+  3. Per‑channel audio processing (linear buffer copy or playback, no conditionals for common state).  
+  4. MIDI clock events (rare).  
+  5. Increment `global_rt_cycles`.  
 - No syscalls, no locks, no `printf` in the RT path.  
-- The main loop runs at 50 ms intervals; draining two queues is negligible.
+- The main loop sleeps 50 ms between iterations; draining two queues adds negligible overhead.  
 
 ### 6. Architectural Soundness
-- The design cleanly separates RT‑safe command handling (immediate in the process callback) from non‑RT operations (deferred to the main loop).
-- Each input source (MIDI, FIFO) has its own dedicated SPSC queue for commands that must be processed outside the RT thread. This avoids the multi‑producer race that existed before.
-- All commands are represented by a uniform `command_t` structure with a typed enum. No ad‑hoc atomic flags remain.
+- The design is clean and consistent:  
+  - All commands flow through a `command_t` struct.  
+  - Each input source has its own SPSC queue for commands that must be processed outside the RT thread (e.g., add/remove).  
+  - The RT callback handles only RT‑safe state transitions (cycle, stop, bind, unbind).  
+  - The main loop handles add/remove and deferred port unregistration.  
 - The FIFO pipe reader runs in a detached thread – simple and non‑blocking.  
-- The code is easy to extend: adding a new input source (e.g., network socket) would involve creating a new SPSC queue and another drain loop.
+- Adding a new input source (e.g., a network socket) would require:  
+  - Creating a new SPSC queue.  
+  - A producer thread that pushes commands to the appropriate queue.  
+  - Adding a drain loop in `looper_process_commands()`.  
 
 ## Overall Verdict
 
-The code is **safe, race‑free, and architecturally sound**. It meets all real‑time constraints and correctly implements the looper’s state machine with unified command handling from MIDI and a FIFO pipe.
+The code is **complete, race‑free, memory‑safe, and architecturally sound**.  
 
-**Minor remaining items:**
-- The FIFO pipe is untested in the integration suite.
-- `CMD_STOP` is not triggered from MIDI (only from FIFO).
-- The existing `evaluation.md` is outdated and should be replaced with this evaluation.
+- No missing features.  
+- No segfaults or use‑after‑free.  
+- All input sources (MIDI, FIFO) can send any command.  
+- The unified command‑queue architecture is fully realised.  
+
+The only minor observation is that the test suite does not verify the MIDI `CMD_STOP` (note 65) – but that would be trivial to add.  
+
+**Final note:** The evaluation file itself (`evaluation.md`) should be updated to remove the “FIFO untested” and “CMD_STOP not triggered” remarks. The content above can replace it.