schemas icon indicating copy to clipboard operation
schemas copied to clipboard

C/C++: Add support for websocket parameters

Open gasmith opened this issue 8 months ago • 2 comments

Description

This is most of the work towards supporting parameters in C/C++. Things that are here:

  • Lots of fun new types and a billion ways to construct them
  • Callbacks for on_get_parameters and on_set_parameters
  • Tests for parameter types in C & C++
  • Tests for on_get_parameters and on_set_parameters in C++
  • A param-server example, like rust's
  • So much SAFETY

What's missing:

  • Callbacks for on_parameters_subscribe and on_parameters_unsubscribe
  • Plumbing for WebSocketServerHandle::publish_parameter_values
  • C++ docs

Ownership

Ownership is a big deal in this change, because parameters need to be allocated in both rust and C/C++, and ownership needs to be transferred as a return value for the get/set callbacks.

To accommodate that:

  • Expose create/free functions for each parameter type, plus some convenience constructors.
  • get/set callbacks must use the provided allocators to construct their return value.
  • Avoid copies by moving owned values (conventionally *mut) in constructors.
  • Expose clone APIs for cloning shared references (conventionally *const) into owned values (*mut).
  • In C++, use separate types to distinguish between owned values (Parameter) and shared references (ParameterView).

Naming

It's so much fun, you'll want to claw your eyes out:

  • foxglove_parameter_create_dict: Parameter convenience constructor for a dict value.
  • foxglove_parameter_value_create_dict: ParameterValue constructor, for the dict variant.
  • foxglove_parameter_value_dict_create: ParameterValueDict constructor.

Container Types

There are three container types here:

  • FoxgloveParameterArray (for Vec<Parameter>)
  • FoxgloveParameterValueArray (for Vec<ParameterValue>)
  • FoxgloveParameterValueDict (for BTreeMap<String, ParameterValue>)

They're all pretty similar. But I don't think we can use generics, because we need a C-transparent struct layout. There might be a clever way to make it work, or a way to extract common functionality, but I was worried that doing so would make it more difficult to reason about safety, because the logic would be scattered all over the place.

Owned FoxgloveStrings

Unfortunately we now have two types of FoxgloveString: the shared references we had before, plus a new owned version, created with FoxgloveString::from_string, that gets stashed within the parameter types. For now, we deal with this by convention alone, and say that:

  • A FoxgloveString as an API argument is never owned.
  • A FoxgloveString in a parameter struct is always owned.

If we wanted to put a few more guardrails on the rust side, we could perhaps introduce a new type (FoxgloveOwnedString?) with the same C layout (but different methods) and alias it as a foxglove_string so that the C API doesn't become cluttered with redundant types.

FoxgloveBytes

I needed a new struct for a byte array, and so took inspiration from FoxgloveString. I put it in its own module, because I feel like it's going to go through some iterations as we work on service and fetch-asset handlers.

C callback API

  • I think we should standardize on void *context as the first argument; that's pretty typical in C.
  • I'm using CString for some arguments; do we want FoxgloveString instead?

C++ implementation

Thankfully this is the smallest part of the change.

Despite the proliferation of types (for T and TView) and crazy identifier verbosity, I think it's fairly straightforward. I have no idea how idiomatic it is, but I'll say that Claude helped somewhat, so perhaps it's more idiomatic than I would've mustered on my first attempt.

Keep an eye out for the .release() method, which is how we're transferring ownership of the unique_ptr referrent, either to move values into C constructors, or to return the FoxgloveParameterArray value from the get/set callbacks.

C++ websocket tests

I spent some time building an RAII wrapper around the websocket client which ensures that the client is properly shut down, and that the thread is properly joined. This way, when you have some random exception in the middle of your test, you get to actually see it in the ctest output, rather than some cryptic error.

It also turns out that throwing exceptions (e.g., via REQUIRE) from callback handlers is a bad idea, because the exception gets all gummed up by the rust stack frames, and you end up with yet another useless SIGABRT. For now, I'm just punting callback values over to the main thread for validation. But perhaps we ought to consider an exception-handling shim layer around C++ callbacks. Might be useful in production.

Part of: FG-10680

gasmith avatar Apr 23 '25 20:04 gasmith

Is it feasible to summarize the approach to some of the types in the C++ header files? e.g., unowned views are mostly relevant to callbacks, it looks like? I think the complexity here is probably warranted, but we can provide some context in docs. It's likely not where we'd go with a pure C++ implementation.

Honestly, I'm hoping a C++ practitioner comes along and knocks some sense into me, because these types are a little painful to work with.

Writing unit tests, I noticed that (a) forcing move semantics on people is no fun, at least partially because (b) clangd doesn't give you any IDE warnings when you screw it up and (c) clang's compiler errors (about missing copy constructors &c.) are extremely baroque and unhelpful.

Maybe the right thing to do is to write a native C++ version of params, and then do the conversion at the last minute at the C interface boundary, so that C++ SDK users get more friendly types. The downside of doing it that way is that we'll need to write a bunch more C++, but maybe it's worth it.

I played around with the example a little bit and realized I can write different types from the app into state (e.g., insert a string into a float array, or replace it with true). I didn't dig in, but I assume it's unrelated to this PR; I'm wondering if the core SDK should prevent this kind of thing, or leave it up to a param server implementation. (Arguably this is a client bug.)

Yeah, the rust example behaves the same way. Hard to say whether that ball belongs in the SDK's court. I'm inclined to think it's the responsibility of the onSetParameters callback to decide whether a particular parameter is strongly typed.

While playing around with this, I noticed that the app will (a) allow you to create heterogeneous arrays and (b) seems to latch the parameter type - so that even if you replace a f64[] with a str, it'll still say type: float64_array in its setParameters request.

Do you plan to merge this prior to the subscription support or are you going to stack PRs? I think it probably makes sense to add them in the same release, so we'll just need to coordinate.

Agreed, they should merge together. I'll stack the subscription support on top of this PR for review, and we'll merge them both once everybody's happy.

gasmith avatar Apr 24 '25 22:04 gasmith