Restructure battery interfaces implementations #1451
Conversation
* split source code into individual files, per interface type. * sort files into subfolder. * introduce and use namespaces.
|
@AndreasBoehm I appreciate your feedback, but I am unsure whether I want to do this or #1221 first (they conflict each other). |
| object["name"] = "Battery(" + _serial + ")"; | ||
|
|
||
| auto const& config = Configuration.get(); | ||
| if (config.Battery.Provider == 1) { |
There was a problem hiding this comment.
Shouldn't this be part of the JK BMS Battery Stats class instead?
There was a problem hiding this comment.
Since I moved whole functions around, I did not notice this. Yes, this must be cleaned up. Not sure how... I'll look into it.
| publishSensor("Voltage", "mdi:battery-charging", "voltage", "voltage", "measurement", "V"); | ||
| publishSensor("Current", "mdi:current-dc", "current", "current", "measurement", "A"); |
There was a problem hiding this comment.
Should this be part of the generic BatteryNs::HassIntegration based on isVoltageValid and isCurrentValid? (This comment might apply to some more sensors)
| publishSensor("Battery voltage", NULL, "voltage", "voltage", "measurement", "V"); | ||
| publishSensor("Battery current", NULL, "current", "current", "measurement", "A"); |
There was a problem hiding this comment.
Why are those prefixed with Battery? was it like this before? Should we change it?
There was a problem hiding this comment.
I guess we can risk making this the same as in the other providers. When/if we move these to the parent class, there is no way around it anyways.
| public: | ||
| void publishSensors() const final | ||
| { | ||
| ::BatteryNs::HassIntegration::publishSensors(); |
There was a problem hiding this comment.
this means that we will publish sensors that won't get any data. Or did i understand i get it wrong?
There was a problem hiding this comment.
this makes the manufacturer, the data age, and the soc auto-discoverable. I don't know what we set as the manufacturer, but the data age makes sense and I guess SoC is a mandatory value for the mqtt battery interface? I guess this is fine?
There was a problem hiding this comment.
In general we should not republish data from mqtt providers. We applied the same logic for the mqtt powermeter.
Especially the BatteryStats source code file was way too large. This restructuring should not change anything for the user, but will ease maintenance. It shall be the blueprint for structuring source code for other multi-provider-features like the solar charger.