Skip to content

Address issues revealed during claude code review #60

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jonnew wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Address issues revealed during claude code review #60

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d51512
Address issues revealed during claude code review
jonnew Apr 24, 2026
b799fc9
Prevent race between `WaitForSingleObject` and `oni_read_frame`
jonnew Apr 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions api/liboni/drivers/test/onidriver_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,10 @@

#define UNUSED(x) (void)(x)

#define ONI_RFRAMEHEADERSZ sizeof(oni_fifo_time_t) + 2 * sizeof(oni_fifo_dat_t) // [time, dev_idx, data_sz]

// NB: To save some repetition
#define CTX_CAST const oni_test_ctx ctx = (oni_test_ctx)driver_ctx

#define MIN(a,b) ((a<b) ? a : b)

const oni_driver_info_t driverInfo
Expand Down Expand Up @@ -183,7 +184,7 @@ oni_driver_ctx oni_driver_create_ctx()
ctx->sig_queue = queue_u8_create(1024);

// Buffer for creating frames
ctx->max_frame_size += ONI_FRAMEHEADERSZ;
ctx->max_frame_size += ONI_RFRAMEHEADERSZ;
ctx->buff_pos = 0;
ctx->read_buff = malloc(ctx->block_read_size + ctx->max_frame_size);

Expand Down Expand Up @@ -548,7 +549,8 @@ static void _fill_read_buffer(oni_test_ctx ctx, void *data, size_t size)
int d = ctx->enabled_idx[rand() % ctx->num_enabled]; // Select a random device (that is generating data)
for (; // static i
ctx->buff_pos < size;
ctx->buff_pos += (ONI_FRAMEHEADERSZ + ctx->dev_table[d].dev.read_size),
ctx->buff_pos
+= (ONI_RFRAMEHEADERSZ + ctx->dev_table[d].dev.read_size),
d = ctx->enabled_idx[rand() % ctx->num_enabled]) {

// Header
Expand Down
22 changes: 0 additions & 22 deletions api/liboni/liboni.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,6 @@ Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "oni-repl", "oni-repl\oni-re
EndProject
Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "test", "test\test.vcxproj", "{F2118152-414B-401A-A5F0-964598B8D167}"
EndProject
Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "reg_bench", "..\reg_bench\reg_bench.vcxproj", "{9D715FCD-0681-48F2-8378-A9C83BCBCB42}"
ProjectSection(ProjectDependencies) = postProject
{0B6B9ACA-75DE-4776-A6CA-B4123ED59F1D} = {0B6B9ACA-75DE-4776-A6CA-B4123ED59F1D}
{127796BA-A009-43DD-9F0B-295BE0D422B9} = {127796BA-A009-43DD-9F0B-295BE0D422B9}
EndProjectSection
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|x64 = Debug|x64
Expand Down Expand Up @@ -194,22 +188,6 @@ Global
{F2118152-414B-401A-A5F0-964598B8D167}.ReleaseStatic|x64.Build.0 = Release|x64
{F2118152-414B-401A-A5F0-964598B8D167}.ReleaseStatic|x86.ActiveCfg = Release|Win32
{F2118152-414B-401A-A5F0-964598B8D167}.ReleaseStatic|x86.Build.0 = Release|Win32
{9D715FCD-0681-48F2-8378-A9C83BCBCB42}.Debug|x64.ActiveCfg = Debug|x64
{9D715FCD-0681-48F2-8378-A9C83BCBCB42}.Debug|x64.Build.0 = Debug|x64
{9D715FCD-0681-48F2-8378-A9C83BCBCB42}.Debug|x86.ActiveCfg = Debug|Win32
{9D715FCD-0681-48F2-8378-A9C83BCBCB42}.Debug|x86.Build.0 = Debug|Win32
{9D715FCD-0681-48F2-8378-A9C83BCBCB42}.DebugStatic|x64.ActiveCfg = Debug|x64
{9D715FCD-0681-48F2-8378-A9C83BCBCB42}.DebugStatic|x64.Build.0 = Debug|x64
{9D715FCD-0681-48F2-8378-A9C83BCBCB42}.DebugStatic|x86.ActiveCfg = Debug|Win32
{9D715FCD-0681-48F2-8378-A9C83BCBCB42}.DebugStatic|x86.Build.0 = Debug|Win32
{9D715FCD-0681-48F2-8378-A9C83BCBCB42}.Release|x64.ActiveCfg = Release|x64
{9D715FCD-0681-48F2-8378-A9C83BCBCB42}.Release|x64.Build.0 = Release|x64
{9D715FCD-0681-48F2-8378-A9C83BCBCB42}.Release|x86.ActiveCfg = Release|Win32
{9D715FCD-0681-48F2-8378-A9C83BCBCB42}.Release|x86.Build.0 = Release|Win32
{9D715FCD-0681-48F2-8378-A9C83BCBCB42}.ReleaseStatic|x64.ActiveCfg = Release|x64
{9D715FCD-0681-48F2-8378-A9C83BCBCB42}.ReleaseStatic|x64.Build.0 = Release|x64
{9D715FCD-0681-48F2-8378-A9C83BCBCB42}.ReleaseStatic|x86.ActiveCfg = Release|Win32
{9D715FCD-0681-48F2-8378-A9C83BCBCB42}.ReleaseStatic|x86.Build.0 = Release|Win32
EndGlobalSection
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
Expand Down
4 changes: 2 additions & 2 deletions api/liboni/oni-repl/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ int write_reg_file(FILE *file) {
}

// Simple & slow device lookup
int find_dev(oni_dev_idx_t idx)
static int find_dev(oni_dev_idx_t idx)
{
for (size_t i = 0; i < num_devs; i++)
if (devices[i].idx == idx)
Expand Down Expand Up @@ -301,7 +301,7 @@ static void stop_threads()

#ifdef _WIN32

WaitForSingleObject(read_thread, 500); // INFINITE);
WaitForSingleObject(read_thread, INFINITE);
CloseHandle(read_thread);

//WaitForSingleObject(write_thread, 200);
Expand Down
82 changes: 54 additions & 28 deletions api/liboni/oni.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,8 @@ static int _oni_alloc_write_buffer(oni_ctx ctx, void **data, size_t size);
static int _oni_read_buffer(oni_ctx ctx, void **data, size_t size, int);
static void _oni_dump_buffers(oni_ctx ctx);
static void _oni_destroy_buffer(const struct ref *ref);
static inline void _ref_inc(const struct ref *ref);
static inline void _ref_dec(const struct ref *ref);
static inline void _ref_inc(struct ref *ref);
static inline void _ref_dec(struct ref *ref);

oni_ctx oni_create_ctx(const char* drv_name)
{
Expand Down Expand Up @@ -173,8 +173,7 @@ int oni_init_ctx(oni_ctx ctx, int host_idx)
int oni_destroy_ctx(oni_ctx ctx)
{
assert(ctx != NULL && "Context is NULL");

int rc = ctx->driver.destroy_ctx(ctx->driver.ctx);
int rc = oni_destroy_driver(&ctx->driver);
if (rc) return rc;

// NB: _ref_dec is only called when a new shared buffer is created. We must
Expand Down Expand Up @@ -292,8 +291,7 @@ int oni_get_opt(const oni_ctx ctx, int ctx_opt, void *value, size_t *option_len)
}
case ONI_OPT_MAXREADFRAMESIZE: {

assert(ctx->run_state > UNINITIALIZED
&& "Context state must be IDLE or RUNNING.");
assert(ctx->run_state > UNINITIALIZED && "Context state must be IDLE or RUNNING.");
if (ctx->run_state < IDLE)
return ONI_EINVALSTATE;

Expand All @@ -307,8 +305,7 @@ int oni_get_opt(const oni_ctx ctx, int ctx_opt, void *value, size_t *option_len)
}
case ONI_OPT_MAXWRITEFRAMESIZE: {

assert(ctx->run_state > UNINITIALIZED
&& "Context state must be IDLE or RUNNING.");
assert(ctx->run_state > UNINITIALIZED && "Context state must be IDLE or RUNNING.");
if (ctx->run_state < IDLE)
return ONI_EINVALSTATE;

Expand All @@ -322,9 +319,9 @@ int oni_get_opt(const oni_ctx ctx, int ctx_opt, void *value, size_t *option_len)
}
case ONI_OPT_BLOCKREADSIZE: {

assert(ctx->run_state > UNINITIALIZED
&& "Context state must be IDLE or RUNNING.");
assert(ctx->run_state > UNINITIALIZED && "Context state must be IDLE or RUNNING.");
if (ctx->run_state < IDLE)
return ONI_EINVALSTATE;

if (*option_len < ONI_REGSZ)
return ONI_EBUFFERSIZE;
Expand All @@ -335,9 +332,9 @@ int oni_get_opt(const oni_ctx ctx, int ctx_opt, void *value, size_t *option_len)
}
case ONI_OPT_BLOCKWRITESIZE: {

assert(ctx->run_state > UNINITIALIZED
&& "Context state must be IDLE or RUNNING.");
assert(ctx->run_state > UNINITIALIZED && "Context state must be IDLE or RUNNING.");
if (ctx->run_state < IDLE)
return ONI_EINVALSTATE;

if (*option_len < ONI_REGSZ)
return ONI_EBUFFERSIZE;
Expand Down Expand Up @@ -365,8 +362,7 @@ int oni_get_opt(const oni_ctx ctx, int ctx_opt, void *value, size_t *option_len)
return ONI_EBUFFERSIZE;

int rc = _oni_read_config(ctx,
ONI_CONFIG_CUSTOMBEGIN
+ (ctx_opt - ONI_OPT_CUSTOMBEGIN),
ONI_CONFIG_CUSTOMBEGIN + (ctx_opt - ONI_OPT_CUSTOMBEGIN),
value);
if (rc) return rc;

Expand Down Expand Up @@ -423,7 +419,8 @@ int oni_set_opt(oni_ctx ctx, int ctx_opt, const void *value, size_t option_len)
if (rc) return rc;

// Get device table etc
_oni_reset_routine(ctx);
rc = _oni_reset_routine(ctx);
if (rc) return rc;
}

break;
Expand Down Expand Up @@ -668,6 +665,8 @@ int oni_read_frame(const oni_ctx ctx, oni_frame_t **frame)

// Allocate frame and buffer
oni_frame_impl_t *iframe = malloc(sizeof(oni_frame_impl_t));
if (!iframe)
return ONI_EBADALLOC;

// Total frame size
int total_size = sizeof(oni_frame_t);
Expand Down Expand Up @@ -735,6 +734,8 @@ int oni_create_frame(const oni_ctx ctx,

// Allocate frame and buffer
oni_frame_impl_t *iframe = malloc(sizeof(oni_frame_impl_t));
if (!iframe)
return ONI_EBADALLOC;

// Total frame size
int total_size = sizeof(oni_frame_t);
Expand Down Expand Up @@ -777,6 +778,12 @@ int oni_create_frame(const oni_ctx ctx,

int oni_write_frame(const oni_ctx ctx, const oni_frame_t *frame)
{
assert(ctx != NULL && "Context is NULL");

// NB: We don't need run_state == RUNNING because this could be changed in
// a different thread
assert(ctx->run_state >= IDLE && "Context is not acquiring.");

// Write the frame
oni_frame_impl_t *iframe = (oni_frame_impl_t *)frame;

Expand Down Expand Up @@ -871,7 +878,7 @@ const char *oni_error_str(int err)
return "Badly formatted device table supplied by firmware";
}
case ONI_EBADALLOC: {
return "Bad dynamic memory allocation";
return "A memory allocation failed";
}
case ONI_ECLOSEFAIL: {
return "File descriptor close failure (check errno)";
Expand Down Expand Up @@ -915,7 +922,7 @@ const char *oni_error_str(int err)
{
return "Received malformed frame";
}
case ONI_EBADCONTROLLER :
case ONI_EBADCONTROLLER :
{
return "ONI Controller is not compatible with driver translator";
}
Expand Down Expand Up @@ -1048,11 +1055,12 @@ static int _oni_reset_routine(oni_ctx ctx)
// NB: Default the block read size to a single max sized frame. This is bad
// for high bandwidth performance and good for closed-loop delay. The opposite is true
// for write frames (to an extent) so this is defaulted to something large.
ctx->block_read_size = ctx->max_read_frame_size
+ ctx->max_read_frame_size % sizeof(oni_fifo_dat_t);
ctx->block_write_size = ctx->max_write_frame_size
+ ctx->max_write_frame_size % sizeof(oni_fifo_dat_t);
ctx->block_write_size = ctx->block_write_size < 4096 ? 4096 : ctx->block_write_size;
size_t align = sizeof(oni_fifo_dat_t);
size_t r = ctx->max_read_frame_size % align;
ctx->block_read_size = ctx->max_read_frame_size + (r ? align - r : 0);
Copy link
Copy Markdown
Collaborator

@aacuevas aacuevas Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my ONIv2 implementation I'm using

ctx->block_read_size = (ctx->max_read_frame_size + align - 1) & ~(align - 1);

which basically does the same for power of 2 sizes in a very fast way

here is not critical but I'll be using it on some other more critical places, so it could be a good moment to standardize

Copy link
Copy Markdown
Member Author

@jonnew jonnew Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good. i can change that.


r = ctx->max_write_frame_size % align;
ctx->block_write_size = ctx->max_write_frame_size + (r ? align - r : 0);

// Set the block read size in the driver, in case it needs it
ctx->driver.set_opt_callback(ctx->driver.ctx,
Expand Down Expand Up @@ -1245,9 +1253,18 @@ static int _oni_read_buffer(oni_ctx ctx, void **data, size_t size, int allow_ref
// New buffer allocated, old_buffer saved
struct oni_buf_impl *old_buffer = ctx->shared_rbuf;
ctx->shared_rbuf = malloc(sizeof(struct oni_buf_impl));
if (!ctx->shared_rbuf) {
ctx->shared_rbuf = old_buffer;
return ONI_EBADALLOC;
}

// Allocate data block in buffer
ctx->shared_rbuf->buffer = malloc(remaining + ctx->block_read_size);
if (!ctx->shared_rbuf->buffer) {
free(ctx->shared_rbuf);
ctx->shared_rbuf = old_buffer;
return ONI_EBADALLOC;
}

// Transfer remaining data to new buffer
if (old_buffer != NULL) {
Expand Down Expand Up @@ -1299,9 +1316,18 @@ static int _oni_alloc_write_buffer(oni_ctx ctx, void **data, size_t size)
// New buffer allocated, old_buffer saved
struct oni_buf_impl *old_buffer = ctx->shared_wbuf;
ctx->shared_wbuf = malloc(sizeof(struct oni_buf_impl));
if (!ctx->shared_wbuf) {
ctx->shared_wbuf = old_buffer;
return ONI_EBADALLOC;
}

// Allocate data block in buffer
ctx->shared_wbuf->buffer = malloc(ctx->block_write_size);
if (!ctx->shared_wbuf->buffer) {
free(ctx->shared_wbuf);
ctx->shared_wbuf = old_buffer;
return ONI_EBADALLOC;
}

// Context releases control of old buffer
if (old_buffer != NULL)
Expand Down Expand Up @@ -1345,21 +1371,21 @@ static void _oni_destroy_buffer(const struct ref *ref)
free(buf);
}

static inline void _ref_inc(const struct ref *ref)
static inline void _ref_inc(struct ref *ref)
{
#ifdef _WIN32
_InterlockedIncrement((int *)&ref->count);
_InterlockedIncrement(&ref->count);
#else
__sync_add_and_fetch((int *)&ref->count, 1);
__sync_add_and_fetch(&ref->count, 1);
#endif
}

static inline void _ref_dec(const struct ref *ref)
static inline void _ref_dec(struct ref *ref)
{
#ifdef _WIN32
if (_InterlockedDecrement((int *)&ref->count) == 0)
if (_InterlockedDecrement(&ref->count) == 0)
#else
if (__sync_sub_and_fetch((int *)&ref->count, 1) == 0)
if (__sync_sub_and_fetch(&ref->count, 1) == 0)
#endif
ref->free(ref);
}
6 changes: 3 additions & 3 deletions api/liboni/oni.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
// Version macros for compile-time API version detection
// NB: see https://semver.org/
#define ONI_VERSION_MAJOR 4
#define ONI_VERSION_MINOR 5
#define ONI_VERSION_PATCH 1
#define ONI_VERSION_MINOR 6
#define ONI_VERSION_PATCH 0

#define ONI_MAKE_VERSION(major, minor, patch) \
((major) * 10000 + (minor) * 100 + (patch))
Expand Down Expand Up @@ -51,7 +51,7 @@ typedef struct {
const oni_fifo_time_t time; // Frame time (ACQCLKHZ)
const oni_fifo_dat_t dev_idx; // Device index that produced or accepts the frame
const oni_fifo_dat_t data_sz; // Size in bytes of data buffer
char *data; // Raw data block
uint8_t *data; // Raw data block
Copy link
Copy Markdown
Collaborator

@aacuevas aacuevas Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is causing some warnings (which on our linux build are treated as errors) because now we are mixing this uint8_t* with calls and buffers declared as char* (so there is a sign mismatch).

Specifically, the issues are in

liboni/api/liboni/oni.c

Line 757 in b799fc9

iframe->private.f.data = buffer_start + ONI_WFRAMEHEADERSZ;

liboni/api/liboni/oni.c

Line 792 in b799fc9

int rc = _oni_write(ctx, ONI_WRITE_STREAM_DATA, iframe->private.f.data - ONI_WFRAMEHEADERSZ, wsize);

function signatures and allocated buffers should be all changed to match this new type

Copy link
Copy Markdown
Member Author

@jonnew jonnew Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or we could revert back to char since this is how its been done for centuries anyway. I'm fine with this.


} oni_frame_t;

Expand Down
8 changes: 2 additions & 6 deletions api/liboni/onidefs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,6 @@
#define __ONI_DEFS_H__
#include <stdint.h>

// Frame header size in bytes
// NB: Header is [dev_idx, data_sz, time]
#define ONI_FRAMEHEADERSZ 2 * sizeof(oni_fifo_dat_t) + sizeof(oni_fifo_time_t)

// Context options
enum {
ONI_OPT_DEVICETABLE = 0,
Expand Down Expand Up @@ -41,7 +37,7 @@ enum {
ONI_ERETRIG = -13, // Attempted to perform a hardware operation before a previous call to the same operation has completed
ONI_EBUFFERSIZE = -14, // Supplied buffer is too small
ONI_EBADDEVTABLE = -15, // Badly formatted device table supplied by firmware
ONI_EBADALLOC = -16, // Bad dynamic memory allocation
ONI_EBADALLOC = -16, // A memory allocation failed
ONI_ECLOSEFAIL = -17, // File descriptor close failure, check errno
ONI_EREADONLY = -18, // Attempted write to read only object (register, context option, etc)
ONI_EUNIMPL = -19, // Specified, but unimplemented, feature
Expand Down Expand Up @@ -88,7 +84,7 @@ typedef uint32_t oni_reg_val_t; // Registers have 32-bit values
typedef uint32_t oni_fifo_dat_t; // FIFOs use 32-bit words; // TODO: find a way to remove
typedef uint64_t oni_fifo_time_t; // FIFO bound timers use 64-bit words; // TODO: find a way to remove

#define BYTE_TO_FIFO_SHIFT 2; // TODO: find a way to remove
#define BYTE_TO_FIFO_SHIFT 2 // TODO: find a way to remove

// Register size
#define ONI_REGSZ sizeof(oni_reg_val_t)
Expand Down
2 changes: 1 addition & 1 deletion api/liboni/onidriverloader.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ static inline void* get_driver_function(lib_handle_t handle, const char* functio
#ifndef NDEBUG
char *e = dlerror();
if (e != NULL)
fprintf(stderr, "%s\n", dlerror());
fprintf(stderr, "%s\n", e);
#endif
return f;
#endif
Expand Down