Skip to content
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.

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

RFE: Support for VSS structs #124

Open
engelmi opened this issue Jan 3, 2025 · 9 comments
Open

RFE: Support for VSS structs #124

engelmi opened this issue Jan 3, 2025 · 9 comments

Comments

@engelmi
Copy link
Contributor

engelmi commented Jan 3, 2025

Please describe what you would like to see

Currently, VSS supports the definition of custom structs. Kuksa, however, does not support those at the moment according to #14.
If possible, I'd like the struct support being implemented in Kuksa.

If struct support in Kuksa makes sense and is desired, I can give it a try and implement it (with a bit of guidance since I am not familiar with the project code, yet).

Please describe your use case

I'd like to integrate BlueChi with Kuksa. Specifically, the goal is to propagate all state changes observed by BlueChi of ECUs (online/offline) and applications (running, stopping, etc.) to Kuksa. This would allow VSS to be used as a standardized data format also for software state changes across the vehicle and Kuksa would be the centralized broker for it.

This requires a translation between BlueChi, VSS and Kuksa - which I started working on here and defined an initial VSS model for BlueChi. Using VSS structs for this use case seems more appropriate than generating a .vspec (with primitive types) on the fly when starting BlueChi and passing that to Kuksa. But maybe there is different, more simple solution, so feedback is highly appreciated.

@SebastianSchildt
Copy link
Contributor

SebastianSchildt commented Jan 7, 2025

Hi @engelmi
Having struct support in KUKSA would be great. It does come with a bunch of decisions/tradeoffs to be made.

So currently as you have seen KUKSA is based on GRPC (protobuf) definition of data structures. While that of course WOULD work fine with vspec structs, the catch 22 is, taht those structs would be avaulable at "compile time" for KUKSA and its clients, however we try to keek KUKSA (the "binary") seperate from from the VSS model (config). I see two potetnial "easy"? options to add structs without breaking the API too mich

    1. Add a new datatype "struct" (in addtion ot the in, string, etc.)
    1. Either
      • encode the structs as JSON in in string (maybe have a fancy way to also provide JSON schema to clients). Disadvantage: You get JSON in the mix in your formerly purely proto world
      • Actually do use some form of proto model for the structs, but keep KUKSA oblivious to it: Stuff it into a char array proto wise as fast as as KUKSA is concerned. Just also flag it as datatype struct instead of uint8[]. Disadvantage: Not much discoverability, and you do need to play/compile protos on your client side.

There may be other options, but those I can remember from discussions in the past. Happy to hear other ideas take PRs :D

Basically, as today structs are not supported we would be relatively open wrt to how exactly this is solved, in case a PR is provided, if it does not break the current API and code paths for the primitive types (too much)

@argerus
Copy link
Contributor

argerus commented Jan 14, 2025

Using VSS structs for this use case seems more appropriate than generating a .vspec (with primitive types) on the fly when starting BlueChi and passing that to Kuksa.

Adding / removing signals requires databroker to reload its configuration, but it's not necessarily a bad way of accomplishing what you want to do.

    1. Add a new datatype "struct" (in addtion ot the in, string, etc.)
    1. Either
      * encode the structs as JSON in in string (maybe have a fancy way to also provide JSON schema to clients). Disadvantage: You get JSON in the mix in your formerly purely proto world
      * Actually do use some form of proto model for the structs, but keep KUKSA oblivious to it: Stuff it into a char array proto wise as fast as as KUKSA is concerned. Just also flag it as datatype struct instead of uint8[]. Disadvantage: Not much discoverability, and you do need to play/compile protos on your client side.

Adding a new data type (struct) seems like a given if we want to support this new data type, but wouldn't it make more sense to define it the same way that all the other data types are defined, i.e. in the databroker proto definitions, reusing the already defined Value message.

I.e. adding something like this:

message Struct {
  map<string, Value> fields = 1;
}

message Value {
  oneof typed_value {
    string string            = 11;
    bool bool                = 12;
    ...
    DoubleArray double_array = 28;
    Struct struct = 30;
  }
}

enum DataType {
  DATA_TYPE_STRUCT     = 40;
}

There are perhaps other ways to define the Struct in order to make it a bit more efficient, at the cost of some convenience / ease of use. For example:

message Field {
  FieldId id = 1;
  Value value = 2;
}

message Struct {
  repeated Fields fields = 1;
}

Introducing JSON as a parallel serialization inside of the already existing protobuf serialization seems totally inappropriate for a multitude of reasons (I can elaborate, but I'm not sure it's needed). Having a bespoke .proto for each struct would probably introduce a lot of complexity for uncertain benefits in my opinion, but I could be wrong..

@SebastianSchildt
Copy link
Contributor

I think we agree that in any case, this needs to be added as "DataType

enum DataType {
 ...
  DATA_TYPE_STRUCT     = 40;
}

The other questions is then how it is encoded. I was not aware of the struct in proto and I have not seen how it looks like when using this in the different languages. A challenge would still be the "convention" how to map a structure defined in vspec to a proto "Struct" (for example: Can we loose the fine distinction in vspec between (u)int8/16/32/ which .proto does not support, or would one still "annotate" that in the struct somehow (and then also taking in mind that VSS structs can be nested).

When using .proto files directly to describe vss structs (especially since there is at least some code around in vss-tools doing that already I think), has the advantage that on the "provider" and "subscriber" side you would get a (depending on language) dataclass from your code generator to work with that might be easier than using the dictionary-like generic structs (where you would need logic like "if field xyz exists: access it). But of course you need to deal with those proto files.

It hink databroker-wise the effort is almost similar, either DATA_TYPE_STRUCT is treated as transporting a proto struct or as a proto bytes array.

No need to elaborate why JSON(schema) is the worse generic solution, adding yet another required techstack. For individual projects that are living in the Python/JS something world it might be easier than dealing with proto. But then maybe for those use cases just use a VSS String where the understanding is "This contains JSON" (Similar to how existing "string" Signals in VSS such as DTCs or ISO timestamps already expect a certain format of the string)

@argerus
Copy link
Contributor

argerus commented Jan 15, 2025

I was not aware of the struct in proto and I have not seen how it looks like when using this in the different languages.

I think you misunderstood me as there is no such thing as a struct in protobuf. When I said struct I was referring to the new VSS data type (compared to say uint8).

But yes, the question is how this should be encoded.

@SebastianSchildt
Copy link
Contributor

SebastianSchildt commented Jan 15, 2025

Ah I was thinking (googling) you were referring maybe referring to https://protobuf.dev/reference/protobuf/google.protobuf/#struct

This is I think basically what you proposed, but it also seems already "built-in"?

Which might be one feasible way to support "unknown/dynamic" data structures in a protobuf way. It is basically a nestable Map (key,value) of proto types If I understood it correctly

@argerus
Copy link
Contributor

argerus commented Jan 15, 2025

Ah I was thinking (googling) you were referring maybe referring to https://protobuf.dev/reference/protobuf/google.protobuf/#struct

This is I think basically what you proposed, but it also seems already "built-in"?

Haha 😄
Yes, that's exactly the same as what I proposed. I didn't know that it was also defined as a "well known type" in protobuf.

Which might be one feasible way to support "unknown/dynamic" data structures in a protobuf way. It is basically a nestable Map (key,value) of proto types If I understood it correctly

There's also the Any type for this purpose.

@argerus
Copy link
Contributor

argerus commented Jan 15, 2025

Yes, that's exactly the same as what I proposed. I didn't know that it was also defined as a "well known type" in protobuf.

But a fundamental difference is that the Value referred to (by me) was the Value defined in the databroker protos. Reusing this Value (and restricting the fields to only be of this type) seems to make a lot of sense to me..

@SebastianSchildt
Copy link
Contributor

Seems we have everything in the toolbox to build a "not-dumb" solution for that. Not sure yet how exactly it would look like but going down the rabbit whole stumbled across this blog, that lines out the tradeoffs between any and struct-style appriaches quite well: https://blog.envoyproxy.io/dynamic-extensibility-and-protocol-buffers-dcd0bf0b8801

@engelmi
Copy link
Contributor Author

engelmi commented Jan 29, 2025

Thank you for the positive feedback and your discussion to this RFE @argerus @SebastianSchildt
I'll try to propose a PR in the next weeks, depending on how much time I can spend on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants