added interface to cpp LiquidCan #2

Z3r0z0 · 2026-01-23T17:54:34Z

Added header for the LiquidCan protocol and process handling

raffael0 · 2026-01-23T19:13:40Z

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:

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?
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
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?
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!

carofcons · 2026-01-23T23:11:13Z

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 · 2026-01-24T13:24:39Z

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 · 2026-01-24T15:24:43Z

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 · 2026-01-24T16:35:09Z

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

raffael0

Hi,
thank you for implementing the suggestions. It already looks much better. However I still have some things to note:

What you did so far looks good, but the most important parts are still missing. This repo should focus on the actual bits that get sent over the wire. So what we need here are packed structs such like we previously had in can houbolt . It's important to put these structs in this repo because the rust/cpp representations have to match. So please add the structs (You can basically copy them from the pdf) into a new header file. The Models/Infrastructure code you have here is also important, but should only belong in this repo if you also plan on doing the actual byte stream-> CanMessage parsing here. Is this something you want to do in this PR? Because my worry is that it might get a bit out of hand and we would end up implementing the rest of the protocol implementation here.
Is there a reason why you named your private variables with a trailing _? imo it's a bit ugly
You sometimes include the field value lengths in the can messages. However, when parsing out an individual message you can't infer it directly from it's contents. You would need the previous state messages to get that information.
You don't really need to store the string lengths, since all strings must be null terminated in the liquidCAN protocol anyways.

Thank you for all of your work! Please let me know if you get overwhelmed or you want some help here. I'm sure on of us could help you out. How is it going so far?

raffael0 · 2026-02-03T10:30:47Z

Cpp-Implementation/Infrastructure/CanMessage.h

+public:
+    virtual ~Can_message() = default;
+protected:
+    Can_message() = default;
+};
+


Can you explain why you explicitly defined the default constructor/destructor?
I think we should enforce the rule of 5/ rule of 0. C.20-C.22 under this link provide a good description.

raffael0 · 2026-02-03T10:35:45Z

Cpp-Implementation/Infrastructure/CanMessageType.h

+#ifndef MESSAGETYPE_H
+#define MESSAGETYPE_H
+
+enum Can_message_type


Could you change all of the enums/class names to be In PascalCase?

the enum attributes are fine as is

raffael0 · 2026-02-03T10:52:28Z

Cpp-Implementation/Models/FieldGetResult.h

+class Field_get_result : public Can_message
+{
+public:
+    Field_get_result();


I think we should remove the default constructor here (and in the other messages), as we don't want messages that are in an inconsistant state(i.e. some fields are defined and some are not). Maybe we should also delete it? What do you think?

raffael0 · 2026-02-03T10:55:32Z

Cpp-Implementation/Models/FieldGetResult.h

+{
+public:
+    Field_get_result();
+    Field_get_result(uint8_t field_id, const uint8_t* value, uint8_t length);


I'm not sure if we should use basic uint8 pointers here. I think a std::array might be more appropriate (@carofcons you probably disagree, right?). I guess it depends on how it is used. An advantage of std::array would be that we could enforce the length (In some cases) at compile time with std::size.

Also: how do you plan on getting the field length? In the message you only receive the fieldID and the individual field id-> datatype mappings won't be stored here

raffael0 · 2026-02-03T11:00:53Z

Cpp-Implementation/Models/NodeInfoRequest.h

+#include <cstdint>
+#include <Infrastructure/CanMessage.h>
+
+class Node_info_request : public Can_message


This should be the NodeInfoAnnouncement and not the request. The request doesn't have any payload

raffael0 · 2026-02-03T11:07:31Z

Cpp-Implementation/Models/NodeInfoRequest.h

+{
+public:
+    Node_info_request();
+    Node_info_request(uint8_t tel_cnt,


Length isn't required here as well

raffael0 · 2026-02-03T11:11:13Z

Cpp-Implementation/Models/ParameterSetLock.h

+{
+public:
+    Parameter_set_lock();
+    Parameter_set_lock(uint8_t parameter_id, uint8_t lock_status);


maybe a LockStatus enum would be nice here instead of the uint8 value

raffael0 · 2026-02-03T11:16:16Z

Cpp-Implementation/Models/TelemetryGroupUpdate.h

+{
+public:
+    Telemetry_group_update();
+    Telemetry_group_update(uint8_t group_id, const uint8_t* values, uint8_t value_count);


Same issue as with the Field Lengths. How do you plan on parsing out the value count of the message without the groupID mapping?

raffael0 · 2026-02-03T13:16:47Z

Cpp-Implementation/Models/FieldIdLookupRequest.h

+{
+public:
+    Field_id_lookup_request();
+    Field_id_lookup_request(const uint8_t* field_name, uint8_t length);


Same issue as with the field_get result. The length is not needed as it can be inferred from the length field.

raffael0 · 2026-02-03T13:17:45Z

Cpp-Implementation/Models/HeartBeat.h

+    uint32_t set_counter(uint32_t value);
+    uint32_t get_counter() const;
+
+    Heart_beat& operator++();


I'd remove this as it adds some unnecessary functionality that might be confusing. Messages should be value types and IMO not reused

added interface fo cpp LiquidCan

2c7288f

Z3r0z0 changed the title ~~added interface fo cpp LiquidCan~~ added interface to cpp LiquidCan Jan 23, 2026

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

Implement the Cpp Header Version #1

Open

Z3r0z0 added 2 commits January 24, 2026 22:39

added models (wip)

8871034

added models

9ae2bdd

raffael0 requested changes Feb 3, 2026

View reviewed changes

added interface to cpp LiquidCan #2

Are you sure you want to change the base?

added interface to cpp LiquidCan #2

Uh oh!

Conversation

Z3r0z0 commented Jan 23, 2026

Uh oh!

raffael0 commented Jan 23, 2026

Uh oh!

carofcons commented Jan 23, 2026

Uh oh!

raffael0 commented Jan 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carofcons commented Jan 24, 2026

Uh oh!

raffael0 commented Jan 24, 2026

Uh oh!

raffael0 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!

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!

4 participants

raffael0 commented Jan 24, 2026 •

edited

Loading