looper/evaluation.md

# Code Evaluation

## Summary Table

| Category                 | Rating        | Remarks                                                                                                                                                                                                                                                                                                                                                                                                              |
|--------------------------|---------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| Mocked / Left Undone     | ✅ Everything implemented | All six command types (`CYCLE`, `STOP`, `BIND_CHANNEL`, `UNBIND`, `ADD_CHANNEL`, `REMOVE_CHANNEL`) are wired from both MIDI and FIFO pipe. No placeholder code or unimplemented paths remain.                                                                                                                                                                                                                  |
| Potential Segfaults      | ✅ Good       | Every `jack_port_get_buffer()` is followed by a null check. Array bounds are respected (dynamic `channel_capacity`). No dynamic allocation in the RT path. The only unchecked call is in `midi_handle_events` – the caller already verified the buffer pointer. The deferred free of the old channel array eliminates the use‑after‑free race.                                                                         |
| Memory Safety            | ✅ Good       | The channel array is dynamically allocated but freed **after** the RT thread has completed at least one cycle after the pointer swap, preventing use‑after‑free. No leaks are present (the old pointer is freed exactly once). All internal buffers are static or stack‑allocated.                                                                                                                                |
| Thread Safety / Race     | ✅ Good       | Three SPSC queues, each with a single writer and single reader, atomics correct. Shared state (`state`, `active`, `control_key_active`, `bind_channel`) uses atomics. The deferred port unregistration and deferred array free both rely on `global_rt_cycles` to guarantee the RT thread has seen the change before the main loop acts. No data races. `prev_state` is accessed only from the RT callback – safe. |
| Performance              | ✅ Good       | No syscalls, locks, or allocations in the RT callback. O(1) queue operations. Linear audio processing per channel. The main loop sleeps 50 ms and drains two queues – negligible overhead.                                                                                                                                                        |
| Architectural Soundness  | ✅ Good       | Clean separation of concerns: unified command enum, per‑source SPSC queues, RT‑safe operations in the callback, main loop handling addition/removal and deferred cleanup. Extensible – adding another input source requires only a new queue and a drain loop.                                                                                    |

## Detailed Remarks

### 1. Mocked / Left Undone
- **Nothing remains.**  
  - `CMD_STOP` is sent from MIDI (note 65 under control key) and from FIFO (`"stop"`).  
  - `CMD_ADD_CHANNEL` / `CMD_REMOVE_CHANNEL` are triggered by MIDI notes 60/61 and FIFO commands `"add"`/`"remove"`.  
  - `CMD_CYCLE`, `CMD_BIND_CHANNEL`, `CMD_UNBIND` are fully wired.  
  - The FIFO pipe reader thread is included and tested by `test_fifo_pipe()`.  

### 2. Potential Segfaults
- Every `jack_port_get_buffer()` result is null‑checked before use.  
- The only unprotected call is in `midi_handle_events`, where the caller has already verified the buffer pointer is non‑null.  
- Array indexes are guarded by `idx < atomic_load(&channel_capacity)`.  
- **No use‑after‑free** – the old channel array is not freed until `global_rt_cycles` has advanced at least once after the pointer swap, guaranteeing the RT callback has seen the new pointer.  

### 3. Memory Safety
- The channel array is allocated with `calloc` and freed exactly once, after a grace period.  
- No memory leaks: every `calloc` has a matching `free` (via the deferred mechanism).  
- FIFO reader uses a stack‑allocated buffer (`char line[256]`) – safe.  
- No heap operations occur in the RT callback.  

### 4. Thread Safety / Race Conditions
- **Three SPSC queues** – each has a single producer and a single consumer, using correct `memory_order_acquire`/`release`.  
  - `cmd_queue`: producer = RT callback, consumer = same RT callback (no inter‑thread race).  
  - `cmd_queue_main_midi`: producer = RT callback, consumer = main loop.  
  - `cmd_queue_main_fifo`: producer = FIFO thread, consumer = main loop.  
- `global_rt_cycles` is incremented with `memory_order_release` at the end of every `process_callback`. The main loop reads it with implicit acquire. The condition `current_cycle - pending_unregister_cycle >= 1` ensures the RT thread has started a new cycle after the flag was set, so port unregistration is safe.  
- The deferred free uses the same pattern: `pending_old_cycle` is set after the atomic exchange, and the old array is freed only after `global_rt_cycles` has advanced by at least 1. This guarantees any RT callback that loaded the old pointer has finished.  
- `prev_state` is a plain `int` but only accessed from the RT callback – safe.  

### 5. Performance
- RT callback per frame:  
  1. MIDI event scan (may push to `cmd_queue` or `cmd_queue_main_midi`).  
  2. Drain `cmd_queue` (usually 0–2 commands).  
  3. Per‑channel audio processing – linear pass‑through, recording, or playback.  
  4. MIDI clock events (rare).  
  5. Increment `global_rt_cycles`.  
- No system calls, no locks, no `printf` in the RT path.  
- Main loop sleeps 50 ms; draining two SPSC queues adds minimal overhead.  

### 6. Architectural Soundness
- **Command‑driven design** – all state changes are represented as `command_t` structs, making the system easy to extend.  
- **Input source isolation** – each source (MIDI, FIFO) has its own queue for commands that must be processed outside the RT thread. The RT callback only handles RT‑safe commands.  
- **Deferred cleanup** – both port unregistration and array deallocation are delayed until the RT thread is guaranteed to have finished using the old resources. This is a correct RCU‑like pattern.  
- **Extensibility** – adding a new input (e.g., UDP socket) requires only a new SPSC queue, a producer thread, and a drain loop in `looper_process_commands()`.  

## Overall Verdict

The code is **complete, race‑free, memory‑safe, and architecturally sound**.  

- All features are implemented and tested (all integration tests pass).  
- No segfaults or memory corruption are possible under the current design.  
- Thread safety is correctly handled using atomic variables and deferred cleanup.  
- Performance is RT‑safe (no blocking operations in the callback).  
- The architecture is clean and extensible.  

**Final note:** The evaluation file can replace the previous version. Remove the outdated remarks about `MAX_CHANNELS` and the reallocation race – those issues have been fixed.