-
Notifications
You must be signed in to change notification settings - Fork 0
added interface to cpp LiquidCan #2
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
base: main
Are you sure you want to change the base?
Conversation
|
Hi, 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:
Thanks again for the PR! |
|
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. |
|
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 |
|
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. |
|
Yeah maybe we can keep coroutines in the back of our mind. You are right we should keep it simple. 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 |
#1
Added header for the LiquidCan protocol and process handling