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

Ezviz battery camera work mode #130478

Draft
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

srescio
Copy link
Contributor

@srescio srescio commented Nov 12, 2024

Proposed change

Add a new select entity called "Battery work mode". Allows setting and getting camera work mode for battery powered camera.

original PR by @pjbuffard
#105802

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

@home-assistant
Copy link

Hey there @RenierM26, @BaQs, mind taking a look at this pull request as it has been labeled with an integration (ezviz) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of ezviz can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign ezviz Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

@srescio
Copy link
Contributor Author

srescio commented Nov 12, 2024

FYI @pjbuffard I moved your branch to my fork as I did not have push permissions and PR got closed
@joostlek I addressed most of the comments you left on the previous PR, am not too clear about the suggestion you made for turning current_option into a separate function

@srescio
Copy link
Contributor Author

srescio commented Nov 13, 2024

is this codecov step a new thing? i don't remember this one on the old PR, also it complains about test coverage being basically zero meaning that there were no prior tests for select.py to begin with 🙃

Copy link
Contributor

@edenhaus edenhaus left a comment

Choose a reason for hiding this comment

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

This PR is doing multiple things, and therefore, this PR should be split.
Split this PR into two PRs, where one is bumping the library only and the second is adding the new select entity. Please keep the second one in draft until the first one is merged.

@home-assistant home-assistant bot marked this pull request as draft December 2, 2024 10:33
@home-assistant
Copy link

home-assistant bot commented Dec 2, 2024

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@srescio srescio mentioned this pull request Dec 2, 2024
19 tasks
@srescio
Copy link
Contributor Author

srescio commented Dec 2, 2024

@edenhaus PR for library bump: #132060

@srescio srescio marked this pull request as ready for review December 2, 2024 12:43
@home-assistant home-assistant bot requested a review from edenhaus December 2, 2024 12:43
@srescio
Copy link
Contributor Author

srescio commented Dec 9, 2024

due to a bug in the latest library version the bump had to be rolledback with:

this PR must wait till pyezviz is properly patched to handle non-battery powered devices and battery devices with different work modes

see issue:

@RenierM26
Copy link
Contributor

Hi @srescio,

I created a new repository and renamed the library. (Original developer is no longer reachable. So thought it's easier to keep the name consistent with pypi package etc.)

Left a note on the in the pyezviz github readme to the new location.

Other than that, you could look at the SupportExt constant to only add when cameras support this function. It's most likely "SupportBatteryManage"

@RenierM26
Copy link
Contributor

#135926

@RenierM26
Copy link
Contributor

Hi @srescio,

I think it's a bit cleaner to add the methods to the entity_description dictionary. Also adds the ability to use supported features from the api for entities.

Please have a look at this example:
https://github.com/RenierM26/ha-ezviz/blob/main/custom_components/ezviz_cloud/select.py

It's just a suggestion that might make it easier for additional select entities in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants