diff --git a/evaluation.md b/evaluation.md index 10de4e6..d4a9497 100644 --- a/evaluation.md +++ b/evaluation.md @@ -2,66 +2,76 @@ ## 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. | +| Category | Rating | Remarks | +|--------------------------|---------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| 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: - 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). -- No syscalls, no locks, no `printf` in the RT path. -- The main loop runs at 50 ms intervals; draining two queues is negligible. +- The RT callback performs in order: + 1. MIDI event processing (may push to `cmd_queue` and `cmd_queue_main_midi`). + 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 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 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. +- 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. +- 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.