-
Notifications
You must be signed in to change notification settings - Fork 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
SNS - Inverter Current #97
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very good start, a few things that can be nicer (I should have thought a bit harder about the adc mux tbh)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all good, one more thing to do is add a test command to the debug REPL. You need to get the i2c object, then construct the sensor, and create a command that reads from the sensor, as well as doing the TOML parsing required.
LMK if you have questions on how that works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just needs lab time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the changes requested by Tom, all seems fine
logger.log(core::LogLevel::kDebug, "Inverter current: %d", *current); | ||
}; | ||
auto read_command | ||
= std::make_unique<Command>(read_command_name, read_command_description, read_command_handler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should also be a usage entry here
static const core::Float MIN_VOLTAGE = -3.3; | ||
|
||
// These can be adjusted | ||
const int reference_voltage = 1.65; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const int ... = 1.65;
most type understanding webdev smdh (i'm totally not one now)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💀
static const int MAX_CURRENT = 75; | ||
static const int MIN_CURRENT = -75; | ||
static const core::Float MAX_VOLTAGE = 3.3; | ||
static const core::Float MIN_VOLTAGE = -3.3; | ||
|
||
// These can be adjusted | ||
const int reference_voltage = 1.65; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all of these should be in the header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point
= inverter_current_voltage * ((MAX_CURRENT - MIN_CURRENT) / (MAX_VOLTAGE - MIN_VOLTAGE)) | ||
+ MIN_CURRENT; | ||
const core::Float reference_voltage_biased = reference_voltage + virtual_ground; | ||
const core::Float current_difference = current - reference_voltage_biased; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this do? I doubt it's valid to subtract a voltage from an amperage
// The sensor maps -75A to 75A into -3V to 3V | ||
// Since the ADC isn't differential, we need to differentiate the reference signal and the output | ||
// signal | ||
static const int MAX_CURRENT = 75; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int
should be sized, std::uint32_t
or whatever suits
|
||
// These can be adjusted | ||
const int reference_voltage = 1.65; | ||
const core::Float virtual_ground = MAX_VOLTAGE / 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the difference between reference voltage and virtual ground?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lint also
Sensors implementation for the inverter current sensor.
Datasheet: https://www.lem.com/sites/default/files/products_datasheets/hob-p_series.pdf