feat: add client tests, status FIFO, and evaluation docs
This commit is contained in:
Loic Coenen
committed by
Loic Coenen (aider)
Loic Coenen (aider)
parent
998406616a
commit
791744beeb
@@ -1,74 +0,0 @@
|
||||
# Code Evaluation
|
||||
|
||||
## Summary Table
|
||||
|
||||
| Category | Rating | Remarks |
|
||||
|--------------------------|---------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
|
||||
| **Mocked / Left Undone** | ✅ Complete | All features are implemented: audio/MIDI looping, dynamic channels, bind/unbind, FIFO pipe, MIDI control with note 66 for MIDI channel creation, FIFO `add_midi` command. Integration tests cover MIDI channel creation, FIFO stop/bind/unbind, and all previously missing functionality. No placeholder code remains. |
|
||||
| **Potential Segfaults** | ✅ Good | Every `jack_port_get_buffer()` call is null‑checked based on channel type. Array accesses bounded by `channel_capacity`. No use‑after‑free – deferred cleanup ensures RT thread has finished with old resources. The only unprotected call is in `midi_handle_events`, but the caller has already verified the buffer. |
|
||||
| **Memory Safety** | ✅ Good | Dynamic channel array allocated with `calloc`, freed exactly once after one RT cycle via deferred free. No leaks. Integration tests do not leak JACK clients or file descriptors. All other buffers are stack‑allocated or static. |
|
||||
| **Thread Safety / Race** | ✅ Good | Three SPSC queues with correct atomic memory ordering (`acquire`/`release`). Shared state uses atomics. Deferred port/array cleanup uses `global_rt_cycles` with release‑acquire synchronisation. Channel `type` is written before `active=1` (release), RT thread reads `type` only after confirming `active==1` (acquire). No data races. |
|
||||
| **Performance** | ✅ Good | RT callback has no syscalls, locks, or allocations. Linear per‑channel processing. Main loop sleeps 50 ms – negligible overhead. Integration tests are slow (~25 s total) due to fixed `usleep()` waits; this is acceptable for an integration suite. |
|
||||
| **Architectural Soundness** | ✅ Good | Clean command‑driven design; per‑source input queues; RCU‑like deferred cleanup; extensible. Integration tests are well‑structured (per‑test looper process, real JACK connections, helpers). Missing test coverage has been addressed (MIDI channel creation, FIFO stop/bind/unbind). |
|
||||
|
||||
## Detailed Remarks
|
||||
|
||||
### 1. Mocked / Left Undone
|
||||
- **Nothing remains.**
|
||||
- `CMD_ADD_MIDI_CHANNEL` is triggered by MIDI note 66 (under control key) and by FIFO command `"add_midi"`.
|
||||
- `CMD_STOP` is sent from MIDI (note 65 under control key) and from FIFO (`"stop"`).
|
||||
- `CMD_BIND_CHANNEL`, `CMD_UNBIND`, `CMD_CYCLE`, `CMD_ADD_CHANNEL`, `CMD_REMOVE_CHANNEL` are all wired.
|
||||
- The integration test suite now includes `test_fifo_stop_bind_unbind()` and `test_midi_channel_add()`.
|
||||
- The FIFO pipe reader handles `"stop"`, `"bind <ch>"`, `"unbind"`, and `"add_midi"`.
|
||||
- **Note:** The separate test files in `tests/` (`test_audio.c`, `test_channel.c`, `test_fifo.c`, `test_loop.c`, `main.c`) are not compiled by the makefile and require a missing `test_common.h`. They are not part of the build – they do not affect functionality and may be removed in a future cleanup.
|
||||
|
||||
### 2. Potential Segfaults
|
||||
- **Audio channels:** `audio_in`/`audio_out` are checked for NULL before use.
|
||||
- **MIDI channels:** `midi_in`/`midi_out` are checked before use.
|
||||
- All `jack_port_get_buffer()` calls are inside guarded blocks.
|
||||
- Array indices are validated: `cap = atomic_load(&channel_capacity); idx < cap`.
|
||||
- The only unguarded call is in `midi_handle_events`, but its caller (`process_callback`) has already verified the port buffer pointer.
|
||||
|
||||
### 3. Memory Safety
|
||||
- The channel array is grown via `calloc` + memcpy + atomic exchange. The old pointer is freed only after at least one RT cycle has passed (`pending_old_cycle` vs `global_rt_cycles`).
|
||||
- No dynamic allocation occurs in the RT callback.
|
||||
- The FIFO pipe thread uses a stack‑allocated buffer (`char line[LINE_MAX]`).
|
||||
- No memory leaks: every `calloc` is eventually freed, and JACK ports are unregistered in deferred cleanup.
|
||||
|
||||
### 4. Thread Safety / Race Conditions
|
||||
- **Three SPSC queues:**
|
||||
- `cmd_queue` – producer = RT callback, consumer = same RT (no race).
|
||||
- `cmd_queue_main_midi` – producer = RT callback, consumer = main loop.
|
||||
- `cmd_queue_main_fifo` – producer = FIFO thread, consumer = main loop.
|
||||
- All queues use correct `memory_order_acquire`/`release` for head/tail.
|
||||
- `global_rt_cycles` is incremented with `memory_order_release` at the end of every RT cycle.
|
||||
- Deferred port unregistration and array free both wait for `current_cycle - pending_cycle >= 1`, guaranteeing the RT thread has seen the change.
|
||||
- `prev_state` is a plain `int` but only accessed from the RT thread – safe.
|
||||
- No data races detected.
|
||||
|
||||
### 5. Performance
|
||||
- RT callback per frame:
|
||||
1. MIDI event scan (may push to queues).
|
||||
2. Drain `cmd_queue` (usually 0–2 commands).
|
||||
3. Per‑channel processing – linear audio or MIDI event copy/playback.
|
||||
4. MIDI clock events (rare).
|
||||
5. Increment `global_rt_cycles`.
|
||||
- No syscalls, locks, or heap operations.
|
||||
- Main loop sleeps 50 ms; draining two queues adds negligible overhead.
|
||||
|
||||
### 6. Architectural Soundness
|
||||
- **Command‑driven design** – all state changes are explicit `command_t` structs.
|
||||
- **Input source isolation** – each source (MIDI, FIFO) has its own queue for main‑loop commands. RT‑safe commands go to `cmd_queue`.
|
||||
- **Deferred cleanup** – RCU‑like pattern for port unregistration and array deallocation ensures no use‑after‑free.
|
||||
- **Extensibility** – adding a new control input requires only a new SPSC queue, a producer thread, and a drain loop in `looper_process_commands()`.
|
||||
- Integration tests cover all major control paths.
|
||||
|
||||
## Overall Verdict
|
||||
|
||||
The code is **complete, race‑free, memory‑safe, and architecturally sound**.
|
||||
|
||||
- All intended features are implemented and tested.
|
||||
- No segfault or memory corruption is possible under normal operation.
|
||||
- Thread safety is correctly handled with atomic variables and deferred cleanup.
|
||||
- Performance is suitable for real‑time audio.
|
||||
- The architecture is clean and extensible.
|
||||
89
engine/tests/test_status_fifo.c
Normal file
89
engine/tests/test_status_fifo.c
Normal file
@@ -0,0 +1,89 @@
|
||||
#include <stdio.h>
|
||||
#include <stdlib.h>
|
||||
#include <unistd.h>
|
||||
#include <sys/wait.h>
|
||||
#include <sys/stat.h>
|
||||
#include <fcntl.h>
|
||||
#include <string.h>
|
||||
#include <errno.h>
|
||||
|
||||
#define STATUS_FIFO "/tmp/looper_status"
|
||||
#define CMD_FIFO "/tmp/looper_cmd"
|
||||
|
||||
static int write_cmd(const char *cmd) {
|
||||
int fd = open(CMD_FIFO, O_WRONLY | O_NONBLOCK);
|
||||
if (fd < 0) return -1;
|
||||
size_t len = strlen(cmd);
|
||||
int n = write(fd, cmd, len);
|
||||
if (n == (int)len && cmd[len-1] != '\n')
|
||||
write(fd, "\n", 1);
|
||||
close(fd);
|
||||
return (n >= 0) ? 0 : -1;
|
||||
}
|
||||
|
||||
static int read_status_line(char *buf, size_t bufsize) {
|
||||
int fd = open(STATUS_FIFO, O_RDONLY | O_NONBLOCK);
|
||||
if (fd < 0) return -1;
|
||||
FILE *f = fdopen(fd, "r");
|
||||
if (!f) { close(fd); return -1; }
|
||||
if (fgets(buf, bufsize, f) == NULL) {
|
||||
fclose(f);
|
||||
return -1;
|
||||
}
|
||||
fclose(f);
|
||||
return 0;
|
||||
}
|
||||
|
||||
static pid_t start_looper(void) {
|
||||
pid_t pid = fork();
|
||||
if (pid < 0) { perror("fork"); return -1; }
|
||||
if (pid == 0) {
|
||||
close(2);
|
||||
open("/dev/null", O_WRONLY);
|
||||
execl("./looper", "looper", NULL);
|
||||
perror("execl");
|
||||
_exit(1);
|
||||
}
|
||||
return pid;
|
||||
}
|
||||
|
||||
static int test_status_after_record(void) {
|
||||
printf("Test: status FIFO reports recording state\n");
|
||||
pid_t pid = start_looper();
|
||||
if (pid < 0) return 1;
|
||||
usleep(200000);
|
||||
if (write_cmd("record 0") != 0) {
|
||||
fprintf(stderr, " FAIL: cannot write record command\n");
|
||||
kill(pid, SIGTERM); waitpid(pid, NULL, 0);
|
||||
return 1;
|
||||
}
|
||||
usleep(500000);
|
||||
char line[256];
|
||||
if (read_status_line(line, sizeof(line)) != 0) {
|
||||
fprintf(stderr, " FAIL: cannot read status line\n");
|
||||
kill(pid, SIGTERM); waitpid(pid, NULL, 0);
|
||||
return 1;
|
||||
}
|
||||
int ch, sc;
|
||||
char state[32];
|
||||
if (sscanf(line, "CH=%d SC=%d STATE=%31s", &ch, &sc, state) != 3) {
|
||||
fprintf(stderr, " FAIL: malformed status line: %s\n", line);
|
||||
kill(pid, SIGTERM); waitpid(pid, NULL, 0);
|
||||
return 1;
|
||||
}
|
||||
if (ch != 0 || sc != 0 || strcmp(state, "RECORD") != 0) {
|
||||
fprintf(stderr, " FAIL: expected CH=0 SC=0 STATE=RECORD, got: CH=%d SC=%d STATE=%s\n",
|
||||
ch, sc, state);
|
||||
kill(pid, SIGTERM); waitpid(pid, NULL, 0);
|
||||
return 1;
|
||||
}
|
||||
printf(" PASS\n");
|
||||
kill(pid, SIGTERM); waitpid(pid, NULL, 0);
|
||||
return 0;
|
||||
}
|
||||
|
||||
int main(void) {
|
||||
int fail = 0;
|
||||
fail += test_status_after_record();
|
||||
return fail;
|
||||
}
|
||||
Reference in New Issue
Block a user