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 yolink siren battery entity #99310
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.
The PR title doesn't match the change. We're not adding a new battery sensor for the siren.
and state.get("powerSupply") == "usb" | ||
): | ||
# When the power supply mode is usb, the power information is wrong | ||
state["battery"] = None |
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.
We shouldn't modify to correct the data coming from the api. If it needs adjustment it should be done in the device library.
Is this a device that can both be powered by USB and battery and power supply can change during the entity lifetime?
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.
Thanks, I will change on API library, When the device has a battery installed and is powered by USB, the battery power obtained is inaccurate.
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.
I think just remove message convert is fine, When a battery is installed in the device, if device changed to USB power supply, if device battery level displayed as unavailable, which is unreasonable.
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.
Is this the current behavior, that the sensor becomes unavailable if the power supply changes to USB? Where is the logic for this?
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.
At my first commit, I tried to change siren battery entity value to None (unavailable) when device power supply change to USB, I think that is incorrect, When a battery is installed in the device, should not change siren battery entity to None even if the battery information is not correct(circuit design leads to),So I think just remove message convert is fine. No API library changes required.
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.
Setting the value None
as the sensor native value will be translated to unknown state in the base entity class, not unavailable state.
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.
Ok.
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Nevermind this comment. I misread the change. It is a new sensor. |
Please mark the PR as ready for review when it's ready to merge. |
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.
LGTM 😊
Proposed change
Add Siren device battery entity.
Type of change
Additional information
Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: