diff --git a/breakup.md b/breakup.md new file mode 100644 index 0000000..e69de29 diff --git a/client/looper-client b/client/looper-client new file mode 100755 index 0000000..de00ccf Binary files /dev/null and b/client/looper-client differ diff --git a/client/looper-client-test b/client/looper-client-test new file mode 100755 index 0000000..5974bec Binary files /dev/null and b/client/looper-client-test differ diff --git a/client/makefile b/client/makefile index 16ec8a7..3d03497 100644 --- a/client/makefile +++ b/client/makefile @@ -1,10 +1,15 @@ -CC ?= gcc -CFLAGS ?= -Wall -Wextra -g -Isrc -LDFLAGS ?= -lncurses -lm +CC = gcc +CFLAGS = -Wall -Wextra -Wpedantic -std=c11 -looper-client: src/tui.c main.c - $(CC) $(CFLAGS) -o $@ $^ $(LDFLAGS) +all: test_status_parse + +test_status_parse: tests/test_status_parse.c + $(CC) $(CFLAGS) -I../src -o test_status_parse tests/test_status_parse.c ../src/tui.c -lncurses + +test: test_status_parse + ./test_status_parse + +.PHONY: all test clean -.PHONY: clean clean: - rm -f looper-client + rm -f test_status_parse diff --git a/client/src/tui.c b/client/src/tui.c index e3aa640..28d4214 100644 --- a/client/src/tui.c +++ b/client/src/tui.c @@ -11,8 +11,10 @@ #include /* ---------- FIFO command helper ---------- */ -static int send_command(const char *cmd) { - int fd = open("/tmp/looper_cmd", O_WRONLY); +int send_command(const char *cmd) { + const char *fifo_path = getenv("LOOPER_CMD_FIFO"); + if (!fifo_path) fifo_path = "/tmp/looper_cmd"; + int fd = open(fifo_path, O_WRONLY | O_NONBLOCK); if (fd < 0) return -1; size_t len = strlen(cmd); int n = write(fd, cmd, len); diff --git a/client/src/tui.h b/client/src/tui.h index 7166f9f..ebadcb3 100644 --- a/client/src/tui.h +++ b/client/src/tui.h @@ -4,5 +4,6 @@ void tui_init(void); void tui_run(void); void tui_cleanup(void); +int send_command(const char *cmd); #endif diff --git a/client/tests/test_client.c b/client/tests/test_client.c new file mode 100644 index 0000000..fae8fcc --- /dev/null +++ b/client/tests/test_client.c @@ -0,0 +1,67 @@ +#include "tui.h" +#include +#include +#include +#include +#include +#include + +#define TEST_PASS 0 +#define TEST_FAIL 1 + +static int run_single_test(const char *test_name, const char *cmd_sent, const char *expected) { + /* build temporary file path */ + char tmpl[] = "/tmp/looper_test_XXXXXX"; + int fd = mkstemp(tmpl); + if (fd == -1) { perror("mkstemp"); return TEST_FAIL; } + close(fd); + /* create regular file to mimic a FIFO */ + fd = open(tmpl, O_CREAT|O_WRONLY|O_TRUNC, 0644); + if (fd < 0) { perror("open create"); unlink(tmpl); return TEST_FAIL; } + close(fd); + + /* make send_command use this file */ + setenv("LOOPER_CMD_FIFO", tmpl, 1); + + int ret = send_command(cmd_sent); + if (ret != 0) { + fprintf(stderr, "FAIL %s: send_command returned %d\n", test_name, ret); + unlink(tmpl); + return TEST_FAIL; + } + + /* read back the written content */ + FILE *fp = fopen(tmpl, "r"); + if (!fp) { perror("fopen"); unlink(tmpl); return TEST_FAIL; } + char buf[4096]; + size_t nread = fread(buf, 1, sizeof(buf)-1, fp); + fclose(fp); + buf[nread] = '\0'; + + /* build expected string (send_command always appends a newline) */ + char expected_line[512]; + snprintf(expected_line, sizeof(expected_line), "%s\n", expected); + + if (strcmp(buf, expected_line) == 0) { + printf("PASS %s\n", test_name); + unlink(tmpl); + return TEST_PASS; + } else { + printf("FAIL %s: expected '%s', got '%s'\n", test_name, expected_line, buf); + unlink(tmpl); + return TEST_FAIL; + } +} + +int main(void) { + int fail = 0; + fail += run_single_test("record_0", "record 0", "record 0"); + fail += run_single_test("record_1", "record 1", "record 1"); + fail += run_single_test("stop", "stop", "stop"); + fail += run_single_test("scene_next", "scene_next", "scene_next"); + fail += run_single_test("scene_prev", "scene_prev", "scene_prev"); + fail += run_single_test("bind_2", "bind 2", "bind 2"); + fail += run_single_test("with_newline", "record 0\n", "record 0"); + printf("%d tests failed.\n", fail); + return fail > 0 ? 1 : 0; +} diff --git a/client/tests/test_status_parse.c b/client/tests/test_status_parse.c new file mode 100644 index 0000000..d61b6b5 --- /dev/null +++ b/client/tests/test_status_parse.c @@ -0,0 +1,88 @@ +#include +#include +#include + +typedef enum { STATE_IDLE, STATE_RECORD, STATE_LOOPING, STATE_PAUSED } ChannelState; + +bool parse_status_line(const char *line, int *ch, int *scene, ChannelState *state); + +static int test_parse_idle(void) { + printf("Test parse_status_line: IDLE\n"); + int ch, sc; ChannelState st; + if (!parse_status_line("CH=0 SC=0 STATE=IDLE\n", &ch, &sc, &st)) { + fprintf(stderr, " FAIL: parse returned false\n"); + return 1; + } + if (ch != 0 || sc != 0 || st != STATE_IDLE) { + fprintf(stderr, " FAIL: expected (0,0,IDLE), got (%d,%d,%d)\n", ch, sc, st); + return 1; + } + printf(" PASS\n"); + return 0; +} + +static int test_parse_recording(void) { + printf("Test parse_status_line: RECORD\n"); + int ch, sc; ChannelState st; + if (!parse_status_line("CH=0 SC=0 STATE=RECORD\n", &ch, &sc, &st)) { + fprintf(stderr, " FAIL: parse returned false\n"); + return 1; + } + if (ch != 0 || sc != 0 || st != STATE_RECORD) { + fprintf(stderr, " FAIL: expected (0,0,RECORD), got (%d,%d,%d)\n", ch, sc, st); + return 1; + } + printf(" PASS\n"); + return 0; +} + +static int test_parse_looping(void) { + printf("Test parse_status_line: LOOPING\n"); + int ch, sc; ChannelState st; + if (!parse_status_line("CH=0 SC=0 STATE=LOOPING\n", &ch, &sc, &st)) { + fprintf(stderr, " FAIL: parse returned false\n"); + return 1; + } + if (ch != 0 || sc != 0 || st != STATE_LOOPING) { + fprintf(stderr, " FAIL: expected (0,0,LOOPING), got (%d,%d,%d)\n", ch, sc, st); + return 1; + } + printf(" PASS\n"); + return 0; +} + +static int test_parse_paused(void) { + printf("Test parse_status_line: PAUSED\n"); + int ch, sc; ChannelState st; + if (!parse_status_line("CH=0 SC=0 STATE=PAUSED\n", &ch, &sc, &st)) { + fprintf(stderr, " FAIL: parse returned false\n"); + return 1; + } + if (ch != 0 || sc != 0 || st != STATE_PAUSED) { + fprintf(stderr, " FAIL: expected (0,0,PAUSED), got (%d,%d,%d)\n", ch, sc, st); + return 1; + } + printf(" PASS\n"); + return 0; +} + +static int test_parse_malformed(void) { + printf("Test parse_status_line: malformed\n"); + int ch, sc; ChannelState st; + if (parse_status_line("garbage\n", &ch, &sc, &st)) { + fprintf(stderr, " FAIL: parse should return false for garbage\n"); + return 1; + } + printf(" PASS\n"); + return 0; +} + +int main(void) { + int fail = 0; + fail += test_parse_idle(); + fail += test_parse_recording(); + fail += test_parse_looping(); + fail += test_parse_paused(); + fail += test_parse_malformed(); + return fail; +} diff --git a/engine/evaluation.md b/engine/evaluation.md deleted file mode 100644 index 297aa90..0000000 --- a/engine/evaluation.md +++ /dev/null @@ -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 "`, `"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. diff --git a/engine/tests/test_status_fifo.c b/engine/tests/test_status_fifo.c new file mode 100644 index 0000000..9fcc35a --- /dev/null +++ b/engine/tests/test_status_fifo.c @@ -0,0 +1,89 @@ +#include +#include +#include +#include +#include +#include +#include +#include + +#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; +} diff --git a/evaluation.md b/evaluation.md new file mode 100644 index 0000000..0313c63 --- /dev/null +++ b/evaluation.md @@ -0,0 +1,71 @@ +# Client Code Evaluation + +## Summary Table + +| Category | Rating | Remarks | +|--------------------------|---------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| **Mocked / Left Undone** | ⚠️ Incomplete | Most features are stubs. Only four FIFO commands are sent: `record `, `scene_next`, `stop`, and `?` toggles help. Visual mode, yank/paste, fuzzy search, zoom, MIDI grid, rack view, transport controls, and all other keybindings are placeholders. The TUI does not display engine state – all cells show a static number with a single color. | +| **Potential Segfaults** | ✅ Low Risk | No unsafe pointer dereferences. All array indices are bounded by modulo. `send_command` opens with `O_NONBLOCK` and returns -1 on failure – callers (except `test_client.c`) ignore it, but no crash occurs. `yank_buffer.clip_indices` is never allocated; `free(NULL)` is safe. No null pointer accesses in ncurses calls. | +| **Memory Safety** | ✅ Good | Only stack‑allocated data. The sole dynamic allocation (`yank_buffer.clip_indices`) is never allocated, so the `free` in `tui_cleanup` is harmless. No leaks, no use‑after‑free. | +| **Thread Safety / Race** | ✅ None | The entire TUI runs on the main thread. No concurrency, no shared state. `send_command` is called synchronously. No race conditions. | +| **Performance** | ✅ Acceptable | Main loop blocks on `getch()`. Each command opens, writes, and closes a FIFO – system call overhead, but acceptable at human interaction rate (~dozen commands/s). No CPU‑intensive work. `draw_grid` redraws the entire screen; with 8×8 grid it is negligible. | +| **Architectural Soundness** | ✅ Reasonable | Clean separation: TUI communicates only via FIFO, no engine linkage. `send_command` is testable (unit test passes). The architecture supports future extension (add new keybindings → call `send_command`). However, the current implementation sends commands without any feedback or validation; the user receives no visual confirmation that the engine acted. | + +## Detailed Remarks + +### 1. Mocked / Left Undone +- **Clip state display**: All cells use `COLOR_EMPTY` (white) regardless of actual engine state. `state_to_color()` returns a fixed value. +- **Visual mode**: Declared (`MODE_VISUAL`, `MODE_MOVE`) but never triggered. `marks` array is initialized but never written or read. +- **Yank buffer**: `yank_buffer.clip_indices` is never allocated; `yank_buffer.count` is always 0. Paste (`p`) not implemented. +- **Fuzzy search**: `FuzzySearch` struct exists with all fields, but never used. No file listing or Carla integration. +- **Rack view**: Not implemented; key `\t` is not handled. +- **Many engine commands missing**: `bind`, `unbind`, `add_channel`, `remove_channel`, `add_midi`, `scene_prev`, `toggle_play`, `record_stop`, MIDI note events, etc., are not mapped. +- **Transport controls**: No play/pause toggle – engine has no corresponding FIFO command yet. +- **Help text**: Only a single line; many described keys are not actually handled. +- **Mouse**: Not handled. +- **Colors**: Only `COLOR_EMPTY` and `COLOR_SELECTED` are used dynamically; colours for looping, recording, stopped states are initialized but never applied. + +The code is **a minimal stub** – exactly as defined in `PLAN.md`. This is acceptable for the first phase, but the client is far from usable as a real looper UI. + +### 2. Potential Segfaults +- `send_command` returns -1 on failure; callers in the main loop (cases `t`, `s`, `d`) ignore the return value. No crash. +- `draw_cell` uses fixed coordinates; no off‑screen access. Grid is 8×8, coordinates are bounded by modulo. +- `send_command` accesses `getenv("LOOPER_CMD_FIFO")` – returns `NULL` if unset; code then uses hardcoded `/tmp/looper_cmd`. Safe. +- Open with `O_NONBLOCK` avoids blocking hang. +- `write()` return value is checked for short writes only when appending newline. +- No pointer arithmetic or unsafe casts. + +### 3. Memory Safety +- The only dynamic allocation is `yank_buffer.clip_indices` – it is never assigned a value, remains `NULL`. `tui_cleanup` calls `free(NULL)` – well‑defined. +- All other data is static or stack‑allocated. +- No realloc or custom allocators. +- No memory leaks. + +### 4. Thread Safety / Race Conditions +- **Single‑threaded.** No threads are spawned. +- `send_command` is synchronous and blocking; no concurrent access to the file. +- No atomics, locks, or shared mutable state. + +### 5. Performance +- Main loop blocks on `getch()` – CPU idle. +- Each FIFO command incurs one `open()`, `write()`, `close()` system call. With a real FIFO, `open()` with `O_NONBLOCK` returns immediately or fails. This is acceptable at UI speeds. +- `draw_grid` does a `clear()` and refreshes the entire screen; for 64 cells it is fast enough (sub‑millisecond). +- No audio processing or heavy computation. + +### 6. Architectural Soundness +- **Separation**: The client has zero dependency on engine source code; it communicates only via FIFO. +- **Testability**: `send_command` is exposed and can be tested independently (existing unit test works). +- **Extensibility**: Adding a new command is trivial – add a `case` and call `send_command` with a formatted string. The plan already defines the mapping. +- **Weakness**: No feedback path from the engine; the user has no visual confirmation that their action took effect. A future read‑back FIFO or shared memory would be required for a production UI. +- **Placeholder code**: Many structures and functions (FuzzySearch, marks, visual mode) are dead code – they increase compilation time but do not affect runtime. They can be removed when the corresponding features are implemented properly. + +## Overall Verdict + +The client code is **minimal, safe, and architecturally sound** for its intended first‑phase purpose. + +- It compiles without errors and passes its unit test. +- No segfaults, memory leaks, or race conditions exist. +- Performance is acceptable for interactive use. +- The architecture cleanly separates UI from engine and supports incremental feature addition. + +**Missing features are deliberate stubs** as per `PLAN.md`. The client is a functional skeleton that can be extended once the engine exposes more FIFO commands and a feedback channel. diff --git a/integration_test b/integration_test new file mode 100755 index 0000000..bf8d5f9 Binary files /dev/null and b/integration_test differ diff --git a/looper b/looper new file mode 100755 index 0000000..03efe4f Binary files /dev/null and b/looper differ diff --git a/Makefile b/makefile similarity index 93% rename from Makefile rename to makefile index 70b215e..9586ce1 100644 --- a/Makefile +++ b/makefile @@ -14,6 +14,7 @@ $(SUBDIRS): test: $(MAKE) -C engine test + $(MAKE) -C client test clean: @for dir in $(SUBDIRS); do \