Make oni_frame_t use void* for data

[ximion]

Hi!

Just a small follow-up to the previous patch: ctx->shared_rbuf->read_pos is uint8_t, while iframe->private.f.data is char. The latter can have any signedness, the C compiler decides, but often it's signed, so we get this warning.

This patch just silences the warning with no functional change. I looked into using uint8_t everywhere for data, since that is the more modern way to do things in C (distinguishes from printable characters, has a defined width and signedness on every compiler, etc), but that would be a larger, API-breaking change, so I went with the most minimal change instead.

This cast should be fine on most compilers, but it is a bit iffy because char will most likely range from [-128, 127], and uint8_t is 0 - 255. So we do rely on the compiler to not do crazy things, and for us to not interpret the value. All compilers I know will work just fine here and wrap around, but technically the result would be undefined.

If needed, I can split the whitespace changes into their own commit.
Cheers,
Matthias

[jonnew]

See this for relevant discussion

#60 (comment)

[ximion]

Claude is actually correct here 😅

Using char isn't technically wrong, although unsigned char to represent random byte arrays is a bit more common in C. But the more modern thing to do for new code is to just use uint8_t (for C) or std::byte if you have C++.

I could rework the code to use uint8_t everywhere and make it warning/error-free on Linux if you would like that. This would be a much bigger patch though and may require some downstream changes.

[jonnew]

Yes, thanks. We are aware of what a char and uint8_t are. We choose not to fix this because the type of this particular char * vs uint8_t * not really ever relevant. Its an opaque blob made of 8-bit long units.

• In hardware we manipulate bits.
• At API consumption level, raw frames are cast or memcpyed into other types.

The only thing that's used here is the width and that's the same for both. We deemed in not worth the effort to change everywhere. I guess if @ximion wants to do this, then we can. @aacuevas are their any hidden issues we would want to consider?

My feeling is that it touch enough files that its probably not worth it, but maybe Im wrong.

Edit:

Also, at least for

// Frame type
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

} oni_frame_t;

it seems like data should actually be void * since it really represents heterogenous data composed of heterogeneous types and void * is explicit about this and forces the consumer to deal with this explicitly. I dunno.

[ximion]

Yeah, it honestly would be a change to be "technically better" but without any actual real-world benefit unless you are using a really ancient C compiler. It is also quite a common pattern.

it seems like data should actually be void * since it really represents heterogenous data composed of heterogeneous types and void * is explicit about this and forces the consumer to deal with this explicitly. I dunno.

Using void* would make sense, but you use it to do pointer arithmetic in the code to move between bytes, so, doing that would mean you'd have to cast into the correct view first (which honestly might be the right thing to do, for explicitness).

What would help me more than changing pointer types would be to do #63 (comment) - so, get rid of the static library BLOB, add Meson support and a bit of automation on GitHub, so binaries are created automatically. That way, I could just directly pull the repository as a subproject, instead of fetching it as a submodule and wrapping some code around it :-)

But, that change would be larger and a fair bit of work, so I wouldn't do it if it doesn't match the project's goals, as I am not familiar with how you prefer the workflow to be :-)

[ximion]

Using void* would make sense, but you use it to do pointer arithmetic in the code to move between bytes, so, doing that would mean you'd have to cast into the correct view first (which honestly might be the right thing to do, for explicitness).

I think this might actually be better and solves the warnings as well. We now need to cast it explicitly if we want to do arithmetic with it (which I think is actually alright).
I adjusted the patch, let me know if this makes sense to you!

Important note: This is an API break now, and will require adjustments of code downstream.

[jonnew]

Right, yes it will require a bit of thought. I'm going to mark it as a draft for now. We can consider the implications for the next major release which targets V2 of the ONI specification. This will have major performance implications and also breaks the API so it will be a good time to consider it.

You're right that there are definitely a lot of things that can be done with respect to CI here (#23). The big one is that we need to break out the bindings into their own repos and reduce this one to the core c library and maybe oni-repl, although that maybe should also go to its own repo.

I'm unsure what meson buys on for liboni though. This is tiny library with few lines of code and very few dependencies. build speed is not an issue and I personally disagree that make syntax is unpleasant as indicated on the meson/ninja FAQ page. Feel free to specify what kind of CI you envision over on #23 though I might be wrong.

Thanks for your input its great to have community contributions and we are glad that you are adopting the library for your acquisition software.

[ximion]

Okay, I will override the error for this component in Syntalos for now then :-)

Splitting the bindings may make sense, and possibly even resetting the branch, because there are some pretty large Git LFS objects in there apparently, some of which fail to resolve for some reason...

As for Meson: The speed bonus doesn't matter that much here, what it buys you is mainly composability, abstraction and simplicity. With the first, it's easy to embed the project into other projects that use Meson or CMake, like Syntalos. The abstraction makes it a lot easier to support multiple platforms. For example, by using Meson, I can trivially cross-compile to different architectures, could create Windows builds with a different compiler, and generally abstract away a lot of differences between compilers/linkers/OSes and find the right dependencies everywhere. Install support comes as well as a bonus.
Once you do want those features, it is also a lot easier to write than a Makefile. For example, the Meson snippet to build liboni and the ft600 bckend is just 55 lines for Syntalos :-)

What I was suggesting wasn't to drop Makefiles, but to just add the Meson stuff in addition. Having multiple buildsystems is totally fine, as long as there's a benefit.

Syntalos uses CI and automated testing and deployment quite excessively, a tiny version of that would work here too (of course, with the added caveat that liboni supports more platforms than Syntalos does).

[jonnew]

@ximion can you move this content to #23? That way when we get to tackling the CI stuff, your opinions will be taken into consideration.

With respect the large objects: yes, its an know issue: #17.

Both of these issues will be addressed with the next major version increment as we target the V2 ONI spec. We are at the tail end of the hardware side of that implementation, and the API will be the next step.