feat: Foxglove WebSocket bridge + CI/CD - #1
Conversation
- Add pure-C Foxglove WebSocket server (src/foxglove/) - SHA-1 + base64 for WebSocket handshake - RFC 6455 WebSocket framing (send/recv) - Foxglove WebSocket protocol v1 (serverInfo, advertise, messageData) - JSON serializers for all 6 sensor types - poll()-based event loop with multi-client support - Renoir topic subscriber to Foxglove channel forwarding - Integrate into build system - ENABLE_FOXGLOVE CMake option (OFF by default) - Conditional compilation in main.c - Add GitHub Actions CI/CD (.github/workflows/ci.yml) - Release, ASan, and TSan build matrix - clang-tidy static analysis job - MuJoCo and Renoir dependency caching - Update .clang-tidy: disable hicpp-signed-bitwise for crypto code
There was a problem hiding this comment.
Pull request overview
Adds an optional Foxglove WebSocket bridge for streaming sensor data over the Foxglove WS protocol, and introduces a GitHub Actions CI pipeline to build and lint the project.
Changes:
- Add
ENABLE_FOXGLOVE-gated Foxglove WebSocket bridge implementation and wire it into the IPC-enabled runtime. - Add GitHub Actions workflow with build matrix (Release/ASan/TSan) and a clang-tidy lint job.
- Update
.clang-tidyconfiguration to disablehicpp-signed-bitwise.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/main.c |
Starts/stops the Foxglove bridge when IPC is enabled. |
src/foxglove/foxglove.h |
Public API for creating/starting/destroying the bridge. |
src/foxglove/foxglove.c |
WebSocket server + Foxglove protocol v1 implementation and sensor JSON serializers. |
CMakeLists.txt |
Adds ENABLE_FOXGLOVE option, builds foxglove source, links pthread, defines macro. |
.github/workflows/ci.yml |
New CI workflow for build matrix + lint job with caching. |
.clang-tidy |
Disables hicpp-signed-bitwise. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| struct iovec iov[2] = { | ||
| { .iov_base = hdr, .iov_len = hdr_len }, | ||
| { .iov_base = (void*)data, .iov_len = len }, | ||
| }; | ||
| ssize_t total = (ssize_t)(hdr_len + len); | ||
| ssize_t sent = writev(fd, iov, data ? 2 : 1); | ||
| return (sent == total) ? 0 : -1; |
There was a problem hiding this comment.
ws_send_frame() assumes writev() writes the entire header+payload in one call. On stream sockets, partial writes are allowed, so this can silently drop data and corrupt the WebSocket stream. Please loop until all bytes are written (handling EINTR/EAGAIN) or use a helper that reliably sends the full buffer.
| } else if (strstr(buf, "\"unsubscribe\"")) { | ||
| uint32_t sub_id = parse_uint(buf, "\"subscriptionIds\""); | ||
| for (int i = 0; i < client->num_subs; i++) { |
There was a problem hiding this comment.
Foxglove v1 unsubscribe messages use "subscriptionIds" as an array (e.g., [1,2]), but parse_uint() expects a scalar number and will return 0 when the value starts with '['. As a result, unsubscribe requests won’t remove subscriptions. Please parse the array and remove all listed IDs (or switch to a minimal JSON parser).
| if (br->listen_fd >= 0) { | ||
| close(br->listen_fd); | ||
| br->listen_fd = -1; | ||
| } | ||
|
|
||
| pthread_join(br->thread, NULL); |
There was a problem hiding this comment.
foxglove_destroy() unconditionally calls pthread_join(br->thread, NULL). If foxglove_start() was never called or pthread_create() failed, br->thread is uninitialized/invalid and pthread_join() can fail (or worse). Please track whether the thread was started and only join when it is valid (and consider skipping join when called from the bridge thread itself).
| if (br->listen_fd >= 0) { | |
| close(br->listen_fd); | |
| br->listen_fd = -1; | |
| } | |
| pthread_join(br->thread, NULL); | |
| int had_listen_fd = (br->listen_fd >= 0); | |
| if (br->listen_fd >= 0) { | |
| close(br->listen_fd); | |
| br->listen_fd = -1; | |
| } | |
| if (had_listen_fd && !pthread_equal(pthread_self(), br->thread)) { | |
| pthread_join(br->thread, NULL); | |
| } |
| foxglove_bridge_t *fg = NULL; | ||
| if (sim.ipc_enabled) { | ||
| fg = foxglove_create(&sim.transport, FG_DEFAULT_PORT); | ||
| if (fg) foxglove_start(fg); | ||
| } |
There was a problem hiding this comment.
foxglove_start() return value is ignored. If foxglove_start() fails after foxglove_create() succeeds (e.g., bind() fails or pthread_create() fails), main will still call foxglove_destroy(fg) later; foxglove_destroy() currently unconditionally pthread_join()s, which can fail/UB when the thread was never created. Please handle the foxglove_start() error path here (e.g., destroy and set fg=NULL, or track a started flag).
| if (rc != 0 || out_len == 0) continue; | ||
|
|
||
| int jlen = serializers[ch](msg_buf, json_buf, sizeof(json_buf)); | ||
| if (jlen <= 0) continue; |
There was a problem hiding this comment.
serializersch returns the snprintf() return value (bytes that would have been written). If json_buf is too small, jlen will be > sizeof(json_buf) and the subsequent fg_send_message(..., (size_t)jlen) will read past json_buf (OOB read) and may allocate/send a huge frame. Please treat truncation as an error (jlen >= sizeof(json_buf)) or clamp to the actual bytes in the buffer.
| if (jlen <= 0) continue; | |
| if (jlen <= 0 || (size_t)jlen >= sizeof(json_buf)) continue; |
| int off = 0; | ||
| off += snprintf(buf + off, sizeof(buf) - (size_t)off, | ||
| "{\"op\":\"advertise\",\"channels\":["); | ||
|
|
||
| for (int i = 0; i < n; i++) { | ||
| if (i > 0) buf[off++] = ','; | ||
| off += snprintf(buf + off, sizeof(buf) - (size_t)off, | ||
| "{\"id\":%u," | ||
| "\"topic\":\"%s\"," | ||
| "\"encoding\":\"json\"," | ||
| "\"schemaName\":\"%s\"," | ||
| "\"schema\":\"\"}", | ||
| channels[i].id, | ||
| channels[i].topic, | ||
| channels[i].schema_name); | ||
| } | ||
| off += snprintf(buf + off, sizeof(buf) - (size_t)off, "]}"); | ||
|
|
||
| return ws_send_frame(fd, WS_OP_TEXT, buf, (size_t)off); |
There was a problem hiding this comment.
fg_send_advertise() builds JSON into a fixed 4096-byte buffer but doesn’t guard against snprintf() truncation or off running past sizeof(buf). If snprintf() truncates, it returns the would-be length, causing off to grow beyond the buffer and subsequent writes (including buf[off++]) to go out of bounds. Please add explicit bounds checks after each snprintf()/write and fail/trim safely when the buffer is too small.
| int off = 0; | |
| off += snprintf(buf + off, sizeof(buf) - (size_t)off, | |
| "{\"op\":\"advertise\",\"channels\":["); | |
| for (int i = 0; i < n; i++) { | |
| if (i > 0) buf[off++] = ','; | |
| off += snprintf(buf + off, sizeof(buf) - (size_t)off, | |
| "{\"id\":%u," | |
| "\"topic\":\"%s\"," | |
| "\"encoding\":\"json\"," | |
| "\"schemaName\":\"%s\"," | |
| "\"schema\":\"\"}", | |
| channels[i].id, | |
| channels[i].topic, | |
| channels[i].schema_name); | |
| } | |
| off += snprintf(buf + off, sizeof(buf) - (size_t)off, "]}"); | |
| return ws_send_frame(fd, WS_OP_TEXT, buf, (size_t)off); | |
| size_t off = 0; | |
| int written = snprintf(buf + off, sizeof(buf) - off, | |
| "{\"op\":\"advertise\",\"channels\":["); | |
| if (written < 0) { | |
| return -1; | |
| } | |
| if ((size_t)written >= sizeof(buf) - off) { | |
| /* Truncation would occur */ | |
| return -1; | |
| } | |
| off += (size_t)written; | |
| for (int i = 0; i < n; i++) { | |
| if (i > 0) { | |
| if (off + 1 >= sizeof(buf)) { | |
| /* No space left for comma */ | |
| return -1; | |
| } | |
| buf[off++] = ','; | |
| } | |
| written = snprintf(buf + off, sizeof(buf) - off, | |
| "{\"id\":%u," | |
| "\"topic\":\"%s\"," | |
| "\"encoding\":\"json\"," | |
| "\"schemaName\":\"%s\"," | |
| "\"schema\":\"\"}", | |
| channels[i].id, | |
| channels[i].topic, | |
| channels[i].schema_name); | |
| if (written < 0) { | |
| return -1; | |
| } | |
| if ((size_t)written >= sizeof(buf) - off) { | |
| /* Truncation would occur */ | |
| return -1; | |
| } | |
| off += (size_t)written; | |
| } | |
| written = snprintf(buf + off, sizeof(buf) - off, "]}"); | |
| if (written < 0) { | |
| return -1; | |
| } | |
| if ((size_t)written >= sizeof(buf) - off) { | |
| /* Truncation would occur */ | |
| return -1; | |
| } | |
| off += (size_t)written; | |
| return ws_send_frame(fd, WS_OP_TEXT, buf, off); |
| ssize_t n = recv(fd, req, sizeof(req) - 1, 0); | ||
| if (n <= 0) return -1; | ||
| req[n] = '\0'; | ||
|
|
There was a problem hiding this comment.
ws_accept_handshake() reads the HTTP request with a single recv() and assumes it contains the full headers. TCP can split the request across packets, so this can intermittently fail to find Sec-WebSocket-Key and reject valid clients. Please read until the end of headers (\r\n\r\n) or use a small loop/timeout to accumulate the request before parsing.
| ssize_t n = recv(fd, req, sizeof(req) - 1, 0); | |
| if (n <= 0) return -1; | |
| req[n] = '\0'; | |
| size_t total = 0; | |
| /* Read HTTP request headers until end-of-headers marker or buffer is full */ | |
| while (total < sizeof(req) - 1) { | |
| ssize_t n = recv(fd, req + total, sizeof(req) - 1 - total, 0); | |
| if (n <= 0) { | |
| return -1; | |
| } | |
| total += (size_t)n; | |
| req[total] = '\0'; | |
| if (strstr(req, "\r\n\r\n") != NULL) { | |
| break; | |
| } | |
| } | |
| /* If we filled the buffer without seeing end-of-headers, treat as error */ | |
| if (strstr(req, "\r\n\r\n") == NULL) { | |
| return -1; | |
| } |
| uint8_t hdr[2]; | ||
| if (recv(fd, hdr, 2, MSG_WAITALL) != 2) return -1; | ||
|
|
||
| *opcode = hdr[0] & 0x0FU; | ||
| bool masked = (hdr[1] & 0x80U) != 0; | ||
| uint64_t plen = hdr[1] & 0x7FU; | ||
|
|
||
| if (plen == 126) { | ||
| uint8_t ext[2]; | ||
| if (recv(fd, ext, 2, MSG_WAITALL) != 2) return -1; |
There was a problem hiding this comment.
ws_recv_frame() uses recv(..., MSG_WAITALL) for the header/extended length/mask. Even if poll() indicates readability, MSG_WAITALL can still block indefinitely if a client sends a partial frame and stalls, potentially hanging the entire bridge thread. Consider using non-blocking sockets (or timeouts) and reading incrementally without MSG_WAITALL, or disconnecting clients that don’t complete a frame in time.
| if(ENABLE_FOXGLOVE) | ||
| target_link_libraries(hummingbird PRIVATE pthread) | ||
| target_compile_definitions(hummingbird PRIVATE ENABLE_FOXGLOVE) | ||
| message(STATUS "Foxglove WebSocket bridge enabled") |
There was a problem hiding this comment.
CMake links pthread directly when ENABLE_FOXGLOVE is on. On many toolchains the correct flags are provided via find_package(Threads) + Threads::Threads (which may add -pthread to both compile and link, and is more portable than -lpthread). Please switch to the CMake Threads package to avoid platform/toolchain-specific failures.
Changes
Foxglove WebSocket Bridge
src/foxglove/)ENABLE_FOXGLOVECMake option (OFF by default, not part of CI)CI/CD
.github/workflows/ci.yml)WarningsAsErrors: *Other
.clang-tidy: disabledhicpp-signed-bitwise(false positives on unsigned crypto code)Testing
make -j$(nproc)— compiles with-Werrorand 15+ warning flags (zero warnings)make lint— passes clang-tidy with all warnings as errors