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

vesync new platform of binary sensor #134221

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

Conversation

cdnninja
Copy link
Contributor

@cdnninja cdnninja commented Dec 29, 2024

Proposed change

This is the initial draft to add binary sensor platform type. A few key things to note.

  1. It was designed to not require references to the mapping tables like much of the other portion of this integrations. If the underlying library exposes the value it shows it.
  2. Currently it only looks for fan types of devices, humidifiers are a fan so it does add sensors for them now. Not much but a start.
  3. I would like to see the whole integration rely less on mapping and the types within the library under it. Items like humidifiers are a challenge to add because of how other platforms work today.
  4. Creating as draft so a few others get early eyes on this. Outstanding items I have:
  • Switch this to support the coordinator model when that is merged. I expect it to go first.

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

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

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:

  • Documentation added/updated for www.home-assistant.io
    Vesync documentation binary sensor home-assistant.io#36602
    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 @markperdue, @webdjoe, @TheGardenMonkey, mind taking a look at this pull request as it has been labeled with an integration (vesync) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of vesync 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 vesync 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.

@cdnninja
Copy link
Contributor Author

@iprak I am interested in your feedback on the approach of this platform type. As well if you can spot why online sensor goes unavailable I am open to it! I have to add some debug lines to figure that out.

@cdnninja cdnninja marked this pull request as draft December 29, 2024 18:24
cdnninja added a commit to cdnninja/home-assistant.io that referenced this pull request Dec 29, 2024
@iprak
Copy link
Contributor

iprak commented Dec 29, 2024

@iprak I am interested in your feedback on the approach of this platform type. As well if you can spot why online sensor goes unavailable I am open to it! I have to add some debug lines to figure that out.

Looks good, some comments:

  • We should remove the comment about Kia.
  • I am not sure if the on_icon and off_icon are useful, as it stands the one sensor is not defining them.
    What if we used device_class on VeSyncBinarySensorEntityDescription? These are the available device_classes and they should automatically provide the icon? https://www.home-assistant.io/integrations/binary_sensor/#device-class
  • I implemented a version of binary_sensor and number but wanted to resolve the dataCoordinator stuff before making it official -https://github.com/iprak/core-fork/blob/add-humidifier/homeassistant/components/vesync/binary_sensor.py
  • I have been continuously running with debugging enabled in the python package with this change manager = VeSync(username, password, time_zone, debug=True) and debug logging and all the entities have been chugging fine for past week. The entity will go unavailable if devicedevice.connection_status == "online" which means that maybe the Vesync server was not able to reach the device or did not hear back from the device in timely manner. Is your update interval still the same 30 seconds?

@cdnninja
Copy link
Contributor Author

  1. Good catch, I missed one spot.
  2. I removed the code for icons for now, it wasn't used anyway. Device class is already implemented as shown in the descriptions.
  3. That is good to know! We should find a way to coordinate to reduce efforts.
  4. I figured it out. This code here causes it to go unavailable when offline. This means all sensors including useful ones like an "Online" sensor are gone.
    @property
    def available(self) -> bool:
    """Return True if device is available."""
    return self.device.connection_status == "online"
    Leaving that for now.

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.

2 participants