Skip to content
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

add: "State of Operation", "Float voltage" and "Absorption Voltage" #1576

Merged
merged 4 commits into from
Feb 4, 2025

Conversation

SW-Niko
Copy link

@SW-Niko SW-Niko commented Jan 23, 2025

I'm not sure if I did it the right way.

I want to get the "State of Operation", "Float voltage" and "Absorption Voltage" from the victron charge controller.
But I don't want to force every solar charger provider to provide these functions.
If other provider (MPPT) support these functions, then other functions like the "Surplus" or the "Batterie Guard" should also work.

At least that was the idea behind.

Any suggestions?

include/solarcharger/Stats.h Outdated Show resolved Hide resolved
@SW-Niko SW-Niko requested a review from AndreasBoehm January 26, 2025 16:04
Copy link
Member

@AndreasBoehm AndreasBoehm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now, thanks!
Only one last thing, please also adjust https://github.com/hoylabs/OpenDTU-OnBattery/blob/development/include/solarcharger/mqtt/Stats.h

@SW-Niko
Copy link
Author

SW-Niko commented Jan 31, 2025

I'm not sure what you mean. Add the new functions to the solar charger MPTT provider?

Copy link
Member

@AndreasBoehm AndreasBoehm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think i know why the confusion about the MQTT stats came up.
Please don't put default declarations in the Stats.h base class. After this change the following sentence applies: All classes that inherit 'SolarChargers::Stats' need to implement all functions of the base class. In the case of the MQTT stats it will be necessary to implement the three new methods in the same fashion that you implemented them in DummyStats.h..

include/solarcharger/Stats.h Outdated Show resolved Hide resolved
@SW-Niko
Copy link
Author

SW-Niko commented Feb 4, 2025

Ok, done. Hmm .... There must be some advantage to doing it this way. 🤔
Seems I have the read the virtual class chapter in the C++ book again.

@AndreasBoehm
Copy link
Member

Maybe i need to read more about abstract classes and virtual functions in C++ as well.

@AndreasBoehm AndreasBoehm merged commit f3fbcf2 into hoylabs:development Feb 4, 2025
9 checks passed
@schlimmchen
Copy link
Member

The difference is that this way someone who implements a new solar charger provider is forced to think about how to implement these functions. Otherwise, their new solar charger provider class will have a pure virtual class and cannot be instantiated.

The way it was before one could add a new solar charger provider class, derive from Stats ("implement the interface"), but could skip implementing these three functions, i.e., not think about that they exists and that they need to return something meaningful.

Also, as hinted already, the Stats class is meant to be an interface, and that means all functions in the class are pure virtual. Nope, there are also non-virtual functions, nevermind.

Actually, now that I think about it... They should be

virtual uint32_t getAgeMillis() const = 0;

and so on. It tells the compiler that the function will never have an implementation in the base class, forcing the behavior I described above.

@AndreasBoehm
Copy link
Member

Thanks for the hint @schlimmchen!
pure virtual was the kind of declaration that i was looking for. I will adjust the base class in a future PR.

@SW-Niko
Copy link
Author

SW-Niko commented Feb 5, 2025

@schlimmchen
Understood, Thanks for the explanation 👍

@AndreasBoehm
Very good. Now I can rebase the surplus PR.

@SW-Niko SW-Niko deleted the SolarCharger branch February 7, 2025 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants