status entities for battery calibration & balancing

[oliverrahner]

This PR implements #264.
It provides two new binary sensor entities that show whether battery calibration or balancing are currently active:

I don't like the naming much, maybe there is a better alternative?

A viable alternative to having two separate entities would be to have only one entity (like Battery Operation Status) with a custom enum state consisting of Calibrating, Balancing, Normal operation.

The current ID generation logic in this integration does not cater for the situation where multiple entities rely on the same set of objects, that's why I needed to "hack" my way around this by adding an actually not used object. I don't fully understand why you didn't rely on the entity key that is defined in entities.py, but there might be good reasons for that.

Last but not least: I am aware that you tried to keep a certain level of abstraction in handling objects, entities and their relation, which gets a bit contradicted by the very explicit methods get_battery_calibration_status and get_battery_balancing_status, but I couldn't come up with a better way.

[weltenwort]

Hi @oliverrahner, thanks for opening this PR and for providing such a useful description 😍 I'll try to set some time aside for a review within the next few days.

[weltenwort]

ℹ️ I took the liberty of merging main into your PR branch so it uses the most recent linting action and dependency versions

[oliverrahner]

@weltenwort Is there anything I can do to have this integrated? Do you see any issues that I could solve or is it just lack of time on your end?

[weltenwort]

It's indeed just a lack of time 🙈 Sorry to keep you waiting for so long. Thanks for making the changes, I'll try to prioritize looking at it this weekend 🤞

[weltenwort]

Hey @oliverrahner, sorry again for the huge delay. In general you changes make sense, but aren't quite in line with the conventions of HA sets up its platforms. You ran into this problem because you introduced a new platform (binary_sensor), which is meant to be setup from a binary_sensor.py entry point.

Your alternative suggestion for an enum-based sensor wouldn't require that.

[weltenwort]

I think we can avoid this hack by setting the unique_id. I honestly don't remember why I'm not using the key either. That's what you get when nobody reviews my code 🙈

Suggested change

	# 'battery.status2' is not required here, this is just a hack
	# so that this entity's generated id doesn't conflict with
	# "battery.bat_status.calibrating"
	# changing the id generation scheme would break existing installations
	object_names=["battery.bat_status", "battery.status2"],
	unique_id="battery.bat_status.balancing",
	object_names=["battery.bat_status"],

[oliverrahner]

Ah, I didn't think that we could just override the use of a generated id by setting it explicitly. Will change this.

[weltenwort]

If I understand HA's component architecture correctly, setting up a binary sensor in sensor.py would not be correct. Instead, it should be set up as part of the binary_sensor platform (meaning in binary_sensor.py), which is being delegated to by the __init__.py's loop over the PLATFORMS list (which would also have to be extended to include binary sensors). In light of that I actually like your alternative suggestion of introducing a single sensor that displays the state using an enum. What do you think?

[oliverrahner]

TBH, I'm not fully aware of the component architecture, as I never built an integration myself. But it makes sense, even though it just seems to work in my installation without complying with the convention.
But nevertheless, I also think we should go for the enum alternative. I just thought about this after I made the current implementation.
I will refactor this and update the PR once I tested it.

[weltenwort]

Hey @oliverrahner, thanks for your willingness to refactor. Since I had the editor open I figured I could also give it a try. I opened oliverrahner#1 against your main branch in case you haven't done any refactoring yourself. Please feel free to close it if you already have a better implementation.

[V4n1X]

Plans to merge these in the next release?

[weltenwort]

@oliverrahner thanks again for submitting this. I took the liberty of including your findings with some modifications in #349.

@V4n1X the new "Battery Status" entity has been released in https://github.com/weltenwort/home-assistant-rct-power-integration/releases/tag/v0.12.0.

[oliverrahner]

@weltenwort thanks for picking this up again, I lost it out of focus… much appreciated :)