looper/evaluation.md

Code Evaluation

Summary Table

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

• 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() 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 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

• 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 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 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 complete, race‑free, memory‑safe, and architecturally sound.

• 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.