RSDK-1975 - autogen header file #156

stuqdog · 2025-11-04T16:10:47Z

Adds a rust-utils header file, and includes it in releases. Consumers should be separately updated to ensure that they use the header file.

Testing: replaced the viam_rust_utils.h file in the C++ SDK with the one generated here, with a couple small adjustments elsewhere I was able to successfully build and connect to a robot via rust-utils.

stuqdog · 2025-11-04T16:22:56Z

Note to reviewers: I opted for A C-style header. The C++ style header allows us to include namespaces, which is nice, and was marginally more readable, but might not play nice in as many cases and looked like it would potentially not work as well (e.g., it tried to convert rust Box types which I wouldn't be confident in without more testing).

stuqdog · 2025-11-04T19:08:50Z

doc/dependency_decisions.yml

    :why:
    :versions: []
    :when: 2025-04-14 14:25:17.811988000 Z
+- - :permit


(flyby) license finder was failing because of a BSD-zero-clause license. since it's an extremely permissive license, it seemed fine to include here.

stuqdog · 2025-11-04T19:10:56Z

.github/workflows/release.yml

        run: |
          cp target/${{ matrix.target }}/release/libviam_rust_utils.dylib builds/libviam_rust_utils-${{ matrix.platform }}.dylib
          cp target/${{ matrix.target }}/release/libviam_rust_utils.a builds/libviam_rust_utils-${{ matrix.platform }}.a
+          cp viam_rust_utils.h builds/viam_rust_utils-${{ matrix.platform }}.h


possibly overly defensive but I opted for unique header files for each architecture in case they differ in includes or otherwise.

maybe cc @acmorrow but I think we're fine just using one header for all of them. i'm not sure the level of control you have over the generated code, but if there are indeed differing includes this is something you'd want to do with a single header file that has #ifdef blocks

Yes, client needs to be able to name one include and get the right things. If there are architecture dependencies, there should be a per-arch header and then they should be unified with a top-level header.

A suggestion if you are worried: generate for each platform, then checksum or diff them. If they compare identical, just ship the first one. If they differ, fail the build.

Then you don't need a wrapper header at all, unless and until a divergence emerges.

Hmm unfortunately it's a bit awkward to force in more complicated code blocks to the generated code, not impossible but might be awkward.

I do think though that having multiple headers isn't too bad (at least for the SDK), since we can just enforce that the right one is downloaded in CMakeLists.txt

edit: this comment was made before seeing Drew's reply above!)

Hmm, a question on that front just for my own understanding. Are we planning on checking in the header file to the repo? If so, what happens if someone has their own copy of the rust_utils binary that they want to use (a thing we currently support), but it differs in some way from the checked in header? And if we're not checking it in but rather downloading it when we download the rust_utils binary (or expecting a user to provide their own along with their own rust_utils binary) then what's the danger with having that download seek out the architecture-specific header and then save it as to the generic .../rust_utils.h path?

From my perspective, there really shouldn't be any architecture dependent code in this header. It should be identical across all. If architecture variation slipped in, I'd consider that an error.

Shifting the burden to the consumer to select the right header seems somewhat unfair.

Regarding whether we will check this in, my expectation is that no, we wouldn't. I'd instead expect to see the one header published along with the release.

That's why my suggestion was to verify at build time that the header is bit identical on every platform where we generate it. That way, we will notice if they ever diverge, and can publish any one of them as "the header".

lia-viam · 2025-11-05T14:21:43Z

A C-style header is definitely the right choice; C should be considered the lingua franca for making other languages talk to each other like this via their respective FFI interfaces and we could probably use this header in other SDKs.

edit: sorry deleted what I said below; the library is in fact putting conditional extern "C" blocks around the declarations

acmorrow

This looks great. Lots of small questions and comments.

Regarding a C++ header, I expect we will make our own for that, using type traits to make specialized unique_ptr and friends, much like I did here for the C API to the triton server: https://github.com/viam-modules/viam-mlmodelservice-triton/blob/main/src/viam_mlmodelservice_triton.hpp#L131-L152.

acmorrow · 2025-11-05T15:07:34Z

.github/workflows/release.yml

        run: |
          cp target/${{ matrix.target }}/release/libviam_rust_utils.dylib builds/libviam_rust_utils-${{ matrix.platform }}.dylib
          cp target/${{ matrix.target }}/release/libviam_rust_utils.a builds/libviam_rust_utils-${{ matrix.platform }}.a
+          cp viam_rust_utils.h builds/viam_rust_utils-${{ matrix.platform }}.h


Yes, client needs to be able to name one include and get the right things. If there are architecture dependencies, there should be a per-arch header and then they should be unified with a top-level header.

A suggestion if you are worried: generate for each platform, then checksum or diff them. If they compare identical, just ship the first one. If they differ, fail the build.

Then you don't need a wrapper header at all, unless and until a divergence emerges.

acmorrow · 2025-11-05T15:08:57Z

viam_rust_utils.h

+ */
+typedef struct Rotation_f64__3 Rotation3_f64;
+
+#ifdef __cplusplus


I think this shows that what @lia-viam was after is actually already present.

acmorrow · 2025-11-05T15:10:35Z

viam_rust_utils.h

+ * Initialize a tokio runtime to run a gRPC client/sever, user should call this function before trying to dial to a Robot
+ * Returns a pointer to a [`DialFfi`]
+ */
+struct DialFfi *init_rust_runtime(void);


Is there an option to have bindgen prefix these with a leading string to act as a namespace? So they come out as viam_rust_utils_init_rust_runtime or similar?

Yes, they are prolix, but dial and free_string and quaternion_add are very general names with a high chance of very annoying collisions.

unfortunately no, there is a prefix option but it prefixes the whole line so we'd end up with viam_rust_utils_ struct DialFfi *init_rust_runtime(void); which obviously doesn't compile.

I think the relatively straightforward fix is to just rename the methods on the rust side. I do want to think through how important this is, however, because I think this would be an extremely breaking change.

edit: actually we don't have to rename the rust functions themselves, we can have cbindgen do it for us through config options (though we have to maintain a list of every function we want to rename and we risk drift between the actual implementation and the config settings). I think this helps solve some of the breaking change issues for older SDK builds at least but I'll want to test and make sure it works.

So, it can't be a breaking change to do the renames in this header. I think you can use the same cbindgen renaming tricks to achieve that, and I think it is worth it.

Simultaneously, I also think the rust FFI functions should be renamed, per https://viam.atlassian.net/browse/RSDK-1978. The way to do that is probably to rename the functions, add forwarding functions under the old names, and attach deprecation attributes to the old names.

But I'd say we can start by correcting them here, and that'll inform the choices for 1978. Alternatively, just fold 1978 into this?

I would think ideally we'd want some kind of namespacing/name-prefixing happening in rust too. Otherwise even if we called it viam_dial in the header there would be no such symbol in the .a/.dylib file right?

Marking the old version as deprecated and making it call the new one seems like a good option

acmorrow · 2025-11-05T15:14:06Z

viam_rust_utils.h

+/**
+ * The DialFfi interface, returned as a pointer by init_rust_runtime. User should keep this pointer until freeing the runtime.
+ */
+typedef struct DialFfi DialFfi;


Is there a way to get these types to be snake_case instead?

There is, but we'll have to include each type and its remapping individually in a cbindgen config. Frustratingly, trying to set to snake_case seems to be a no-op, see https://github.com/mozilla/cbindgen/blob/ac824ecebc64c1b45720e324b4676eedcc2b177f/docs.md?plain=1#L769

I think it is worth it. snake case is idiomatic for modern C. A little annoying that we need to maintain the mapping, but honestly it will probably be only for a few types.

acmorrow · 2025-11-05T15:18:51Z

viam_rust_utils.h

+   * starts at 0 as you would expect.
+   */
+  ArrayStorage_f64__3__1 data;
+} Matrix_f64__U3__U1__ArrayStorage_f64__3__1;


So, clearly, based on the docstring, this type isn't ours. Per my request to get some namespacing going on, is there a way to get these adorned with some sort of package prefix?

Not only for collision avoidance, but to make it clearer in the header which parts are "ours" and which are from (what) package dependency

yeah, we'll need to individually declare a rename but we certainly can.

I think worth it, so we can tag these into nalgebra or something, as well as snake-ify.

acmorrow · 2025-11-05T15:20:41Z

viam_rust_utils.h

+typedef struct Matrix_f64__U3__U1__ArrayStorage_f64__3__1 Unit_Matrix_f64__U3__U1__ArrayStorage_f64__3__1;
+
+/**
+ * A stack-allocated, 3-dimensional unit vector.
+ */
+typedef Unit_Matrix_f64__U3__U1__ArrayStorage_f64__3__1 UnitVector3_f64;
+


Any idea why it wants the double typedef here? My plain C skills are rusty, but wouldn't typedef struct Unit_Matrix_f64__U3__U1__ArrayStorage_f64__3__1 UnitVector3_f64 work? I wonder why it does that.

Yeah good question! I think this isn't a C thing but actually a rust thing, the issue is that a UnitVector is a Unit<Matrix<...>>, where a Unit<T> is a struct with just a single field. This makes sense in rust because the value field has crate-limited visibility but when translating to C means we have a struct that's just a wrapper around a struct.

That said, I only just briefly looked at the code here so I could be off-base, but it would explain why we don't see a similar doubly nested typedef for the Vector3 type.

Also:

My plain C skills are rusty

heh

They really named the language well.

I missed the Unit floating around in there. This is fine.

that's interesting, I guess the typedefs here are similar to the mangled names you would see for a class template instantiation.

autogen header file

5328e2d

stuqdog requested a review from a team as a code owner November 4, 2025 16:10

stuqdog requested review from acmorrow, lia-viam and njooma November 4, 2025 16:10

add BSD-zero clause to license finder

2c19c29

stuqdog added 2 commits November 4, 2025 11:54

add cpp compatibility

2ca8592

architecture-specific header file

4b0ed04

stuqdog commented Nov 4, 2025

View reviewed changes

acmorrow requested changes Nov 5, 2025

View reviewed changes

RSDK-1975 - autogen header file #156

Are you sure you want to change the base?

RSDK-1975 - autogen header file #156

Uh oh!

Conversation

stuqdog commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuqdog commented Nov 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuqdog Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lia-viam commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acmorrow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuqdog Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stuqdog commented Nov 4, 2025 •

edited

Loading

stuqdog Nov 5, 2025 •

edited

Loading

lia-viam commented Nov 5, 2025 •

edited

Loading

stuqdog Nov 5, 2025 •

edited

Loading