Restructure solarcharger classes #1457
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AndreasBoehm
force-pushed
the
rename-vedirect-to-solarcharger
branch
from
d4d9737 to
d11be3b
Compare
AndreasBoehm
force-pushed
the
andreasboehm/feature/restructure-vedirect
branch
from
e23be05 to
983abde
Compare
AndreasBoehm
force-pushed
the
rename-vedirect-to-solarcharger
branch
from
d11be3b to
a3d0408
Compare
AndreasBoehm
force-pushed
the
rename-vedirect-to-solarcharger
branch
2 times, most recently
from
08e5b9a to
635a450
Compare
AndreasBoehm
force-pushed
the
andreasboehm/feature/restructure-vedirect
branch
5 times, most recently
from
5723f4e to
b630fb9
Compare
AndreasBoehm
commented
Jan 4, 2025
|
|
||
| void MqttHandleVedirectHassClass::init(Scheduler& scheduler) | ||
| void HassIntegration::publishSensors() const |
There was a problem hiding this comment.
@schlimmchen because we need details about the individual charge controller it might make sense to move this into Stats instead of having this separated. what do you think?
I did set this up separately, following your battery restructure, but for the batteries you don't need any data from the battery itself.
There was a problem hiding this comment.
Not sure that I can follow all the way... This is what I can say:
- As this class is specific to Victron charge controllers, I don't feel too bad having to ask for a Victron charge controller data structure here. So, I guess the method is fine here.
- Passing a
std::optional<VeDirectMpptController::data_t>around, even as a reference, to only use the serial number from it (if I see it correctly), is not a good idea. You might be able to de-couple thepublishSensor()method into the base class if you instead pass a serial number as a string. - This whole setup is... questionable. What I mean is conditionally publishing the info for HA auto-discovery. What if this method is only executed before the first data is received from the charge controller? It is not executed periodically, for sure, but only once after reboot and when reconnecting to the MQTT broker (right?). So, this method would not even need a
std::optional<VeDirectMpptController::data_t>but only the serial number if we published all sensors unconditionally. My hope is that HA is fine with published sensors that don't have a value at the respective MQTT topic. ChatGPT says the sensor is still created and visible and so one, but its value will "forever" read "unknown". Meh... So we have to deal with this and publish the sensors conditionally, which means we should also revisit this method regularly?
AndreasBoehm
force-pushed
the
andreasboehm/feature/restructure-vedirect
branch
2 times, most recently
from
8f5e560 to
d5775dc
Compare
AndreasBoehm
commented
Jan 5, 2025
Comment on lines
+25
to
+27
| // TODO(andreasboehm): _data and _lastUpdate in two different structures is not ideal and needs to change | ||
| mutable std::map<String, std::optional<VeDirectMpptController::data_t>> _data; | ||
| mutable std::map<String, uint32_t> _lastUpdate; |
There was a problem hiding this comment.
@schlimmchen any ideas without duplicating all data found in data_t?
There was a problem hiding this comment.
Keep using a copy. This is a direct copy of _tmpFrame of VeDirectFrameHandler, isn't it? It is good that we keep at leas one immutable copy of the data around. VeDirectFrameHandler is messing with it's _tmpFrame in-situ...
BTW: In the long run I see DataPoints being used instead.
AndreasBoehm
force-pushed
the
rename-vedirect-to-solarcharger
branch
2 times, most recently
from
4811e65 to
9e73fcc
Compare
Base automatically changed from
rename-vedirect-to-solarcharger
to
development
January 8, 2025 20:46
AndreasBoehm
force-pushed
the
andreasboehm/feature/restructure-vedirect
branch
2 times, most recently
from
9ce82e7 to
bf3d3f8
Compare
did not touch 'MqttHandleVedirect'. will be done in a stats realted branch/PR fix includes
AndreasBoehm
force-pushed
the
andreasboehm/feature/restructure-vedirect
branch
from
bf3d3f8 to
585d262
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on the structure that @schlimmchen introduced here: #1451
I did not touch 'MqttHandleVedirect'. will be done in a stats related branch/PR.