Feature/bms command refactor #14
Conversation
| /* Formula Slug - Sam Ritzo */ | ||
|
|
||
| #pragma once | ||
|
|
There was a problem hiding this comment.
You need #include <stdint.h> for uint8_t, uint16_t. (include-what-you-use)
bms/src/LTC68xxCommands.h
Outdated
| // Start GPIOs ADC Conversion and Poll Status | ||
| class ADAX : public LTC68xxCommandCode { | ||
| public: | ||
| ADAX(MD md, CHG chg) {} |
There was a problem hiding this comment.
You forgot to make member variables to store the ctor args in. This class probably doesn't even compile since none of the variables in toValue() are in scope (unless the base class defines them, but exposing variables directly from base classes is poor practice).
There was a problem hiding this comment.
Yeah I'm not sure how this successfully compiled...
There was a problem hiding this comment.
Oh... because it didn't get compiled. Not included anywhere. My bad
There was a problem hiding this comment.
Yup, I'm dumb and removed them by mistake
| public: | ||
| virtual uint16_t toValue() const = 0; | ||
|
|
||
| enum class MD : uint8_t { |
There was a problem hiding this comment.
Not 100% convinced on using the shortened names from the datasheet. It might be easier/cleaner to expand them.
Either way we should add some documentation here on what these keywords mean.
There was a problem hiding this comment.
This file should end up being low-level commands only, I want it to reflect exactly what's going on in the datasheet
There was a problem hiding this comment.
A comment should be added at the top of the file describing that design intent, so future maintainers don't mess that up during modifications. Design rationale is good to document somewhere in general.
Spamandritz
force-pushed
the
feature/bms-command-refactor
branch
from
b7fd9b3 to
231d0dd
Compare
No description provided.