Skip to content

Conversation

@Z3r0z0
Copy link

@Z3r0z0 Z3r0z0 commented Jan 23, 2026

#1

Added header for the LiquidCan protocol and process handling

@Z3r0z0 Z3r0z0 changed the title added interface fo cpp LiquidCan added interface to cpp LiquidCan Jan 23, 2026
@raffael0
Copy link
Member

Hi,
Thank you for the PR! There is some really nice stuff here ^^
I have a few notes/questions:

I think I didn't properly explain what this repo is about. The goal here is to collect all protocol level structs/data formats which are transferred on the bus in one location, so that changes are made to the protocol are propagated to both ends of the system. That means that the focus shouldn't be on individual functions or classes (I'd argue they belong in the firmware repo, feel free to disagree though!) but instead the structs that are sent (eg. what a parameter_set_request look like). Can you add them from the protocol pdf? TLDR: just add the structs from the pdf

With that in mind I think what you have is a pretty solid base for the firmware part to build on. Some pointers though:

  1. I didn't really have a plan for how to manage the req -> res flow in the firmware, so thank you for thinking about that. Have you thought about how to implement this in the firmware? Do you want to add a callback to each function that is looking for a response? We could also look into cpp coroutines, but that would add a lot of complexity for a feature which we barely use (node-to-node paramter sets + paramter locking probably won't be used very often). Maybe @miDeb @carofcons also have an opinion here?
  2. It would be nice if we could restrict the templates for the subtype in the final implementation to only be one of the defined types
  3. Additionally it would be nice if we could restrict the strings to be of a certain size to fit in the message. Is that possible in virtual functions? probably not right?
  4. I've noticed that there are a few typos in the comments. I'd really recommend a spellchecker for your IDE. Mine saves me all the time ^^. If you'd like I can also go through and quickly fix them

Thanks again for the PR!

@raffael0 raffael0 linked an issue Jan 23, 2026 that may be closed by this pull request
@carofcons
Copy link

Coroutines are definitely out of the questions, since our firmware system doesn't have the supporting infrastructure in place. In the actual firmware code we would also have to use C-style pointers to characters instead of C++-style strings. The copy constructor and copy assignment also should be explicitly, publicly marked as deleted instead of just being privated.

@raffael0
Copy link
Member

raffael0 commented Jan 24, 2026

I don't think we have to directly rule out coroutines. The short reading I did suggested that it is possible to do it in embedded applications, specifically allocations can be adapted for embedded needs.

For the strings I think it would be nice if we would move away from c style cpp and use a bit more cpp features. For strings specifically I think std::string_view would be a nice option, although we have to keep in mind there that this doesn't enforce null termination (However we could add that in). Also because we could then enforce the string lengths at compile time since string_view is constexpr 'able

@carofcons
Copy link

For coroutines you would have to do some kind of dynamic memory management, and they would also enable very complex control flow. For both of those reasons, I think they are not really suitable for our firmware. It would be possible to implement them, but that by itself would also be a very non-trivial task.
std::string_view would be an option for representing strings, but I don't see a big reason to use it. In any case, we would have to restrict its usage, since some of its methods rely on dynamic memory allocation.
In general, I think that we should use all available C++ features for supporting software, but for the firmware, a more C-style C++ is appropriate.

@raffael0
Copy link
Member

Yeah maybe we can keep coroutines in the back of our mind. You are right we should keep it simple.
I want to use std::string_view because then we can restrict the strings to be of a certain size, fitting into the liquidCAN packages, at compile time.

I disagree. I think we should really consider using more modern cpp features where applicable. But I think the discussion is out of scope for this PR

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

Successfully merging this pull request may close these issues.

Implement the Cpp Header Version

4 participants