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

Done, opted to fold 1978 into this. I opted to prefix our types and functions with viam_ rather than viam_rust_utils_. My reasoning was 1) this is a lot less verbose, and 2) the viam_ prefix probably doesn't create a significantly higher risk of collision than the viam_rust_utils_ prefix. Happy to revisit if either of you disagree with point 2, or think there's some other reason to prefer the fuller prefix.

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.

stuqdog · 2025-11-12T18:15:17Z

viam_rust_utils.h

+ * the caller must remember to free the rotation matrix memory using the
+ * free_rotation_matrix_memory FFI function
+ */
+nalgebra_rotation3_f64 *viam_new_rotation_matrix(const double (*elements)[9]);


This is the one function that hasn't really translated well. The argument in rust is named elements and of type *const [f64; 9]. I'm not sure precisely what the appropriate way to typedef this in C++ would be. Is the best argument here const *double[9] elements? @acmorrow @lia-viam

I think this translation is pretty much correct. Using the ping-pong rule for C declaration parsing, we get that elements is a pointer to an array of size 9 of const double which I think is a faithful translation. I guess we weren't calling this particular function anywhere else before in the code?

As an aside, famously in C you can't actually pass arrays of fixed length as an argument, or rather they just decay to a pointer. See eg
https://stackoverflow.com/questions/77914509/c-passing-an-array-of-fixed-size-to-function-vs-passing-pointer
where they observe that int pipe(int pipefd[2]) is just written that way for documentation purposes, but you can pass any int* or int arr[20] to it.

I was surprised to find however that the C type system does enforce this if you add a level of pointer indirection, see godbolt

oh okay, great! clearly my understanding of the nuances of C syntax are not up to snuff 😂

stuqdog · 2025-11-12T18:35:26Z

viam_rust_utils.h

+/// @deprecated please use `viam_`-prefixed function instead
+void free_axis_angles_memory(struct viam_axis_angle *ptr);


@acmorrow @lia-viam With all the ffi functions, we're now exporting the new viam_-prefixed version and also a version with the old name and a deprecation warning. My intuition is this isn't ideal; I'd like to not advertise the existence of the non-prefixed functions. This should be fine with version compatibility; old SDK and module versions will continue to access these functions as they used to, while new ones that want to use the headers will be making an update anyway so they might as well update to the proper function name.

Unfortunately, initial digging didn't turn up a good way to not include extern "C" rust functions in this header based on deprecated status. We could do some sort of sed tooling to delete these ourselves, but that feels a little error prone. I'm happy to dig into this more, but wanted to get feedback on the our general design preferences and confirm my intuition before doing so.

I see what you mean. I think it's probably fine, even if we get people using the old ad-hoc declarations to switch to this header, that would be an improvement because they'd be getting deprecation warnings. And then I guess in the future the crate just won't declare the non-prefixed versions as part of the FFI?

Well I think long-term what we'd like is to get rid of the non-prefixed versions entirely. If we put this out now and tell people they have 6 months to update, then I think that's a reasonable timeframe to then get rid of the deprecated methods.

Ideally I'd like to keep them out of the header immediately though, since their existing there now still creates some risk of collision I expect? And even if they're not in the header, they're still part of the FFI so existing modules will be able to point to the deprecated versions as they always have. But, doing so is kinda tricky!

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

stuqdog added 4 commits November 12, 2025 11:40

pr comments

cc48a24

fix yml syntax

6a38358

fix yml syntax2

9672ff9

fix lingering mangling issues

d9ff509

stuqdog commented Nov 12, 2025

View reviewed changes

stuqdog requested a review from acmorrow November 12, 2025 18:39

		/// @deprecated please use `viam_`-prefixed function instead
		void free_axis_angles_memory(struct viam_axis_angle *ptr);

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!

stuqdog Nov 12, 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!

Choose a reason for hiding this comment

Uh oh!

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

stuqdog Nov 12, 2025 •

edited

Loading