68 lines
7.2 KiB
Markdown
68 lines
7.2 KiB
Markdown
# Code Evaluation
|
||
|
||
## 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. |
|
||
|
||
## 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.
|
||
|
||
### 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.
|
||
|
||
### 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.
|
||
|
||
### 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.
|
||
|
||
### 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.
|
||
|
||
### 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.
|
||
|
||
## 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.
|
||
|
||
**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.
|