core icon indicating copy to clipboard operation
core copied to clipboard
Published 1 week ago • verified publisher icon home-assistant

Set quality scale to platinum for Motionblinds Bluetooth

Open LennP opened this issue 1 year ago • 4 comments

Proposed change

Set Motionblinds Bluetooth quality scale to platinum


 tests/components/motionblinds_ble/test_button.py ✓✓✓                     7% ▊         
 tests/components/motionblinds_ble/test_config_flow.py ✓✓✓✓✓✓✓           23% ██▍       
 tests/components/motionblinds_ble/test_cover.py ✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓        60% ██████▏   
 tests/components/motionblinds_ble/test_diagnostics.py ✓                 63% ██████▍   
 tests/components/motionblinds_ble/test_entity.py ✓✓✓✓                   72% ███████▎  
 tests/components/motionblinds_ble/test_init.py ✓✓                       77% ███████▋  
 tests/components/motionblinds_ble/test_select.py ✓✓                     81% ████████▎ 
 tests/components/motionblinds_ble/test_sensor.py ✓✓✓✓✓✓✓✓              100% ██████████

---------- coverage: platform linux, python 3.12.2-final-0 -----------
Name                                                       Stmts   Miss  Cover
------------------------------------------------------------------------------
homeassistant/components/motionblinds_ble/__init__.py         52      0   100%
homeassistant/components/motionblinds_ble/button.py           28      0   100%
homeassistant/components/motionblinds_ble/config_flow.py     102      0   100%
homeassistant/components/motionblinds_ble/const.py            20      0   100%
homeassistant/components/motionblinds_ble/cover.py            89      0   100%
homeassistant/components/motionblinds_ble/diagnostics.py      14      0   100%
homeassistant/components/motionblinds_ble/entity.py           25      0   100%
homeassistant/components/motionblinds_ble/select.py           34      0   100%
homeassistant/components/motionblinds_ble/sensor.py           64      0   100%
------------------------------------------------------------------------------
TOTAL                                                        428      0   100%

------------------------------- snapshot report summary -------------------------------
1 snapshot passed.

Results (6.28s):
      43 passed

Silver 🥈

This integration is able to cope when things go wrong. It will not print any exceptions nor will it fill the log with retry attempts.

  • [x] Connection/configuration is handled via a component.
  • [x] ~~Set an appropriate SCAN_INTERVAL (if a polling integration),~~
    • No polling integration
  • [x] ~~Raise PlatformNotReady if unable to connect during platform setup (if appropriate)~~
    • No connection required during platform setup
  • [x] ~~Handles expiration of auth credentials. Refresh if possible or print correct error and fail setup. If based on a config entry, should trigger a new config entry flow to re-authorize. (docs)~~
    • No authentication credentials
  • [x] ~~Handles internet unavailable. Log a warning once when unavailable, log once when reconnected.~~
    • No internet is used
  • [x] ~~Handles device/service unavailable. Log a warning once when unavailable, log once when reconnected. (Same as above) Operations like service calls and entity methods (e.g. Set HVAC Mode) have proper exception handling. Raise ServiceValidationError on invalid user input and raise HomeAssistantError for other failures such as a problem communicating with a device. Read more about raising exceptions.~~
    • Since it is a Bluetooth device that requires a connection, the device cannot become unavailable
  • [x] ~~Set available property to False if appropriate (docs)~~
    • Home Assistant can always read the state and control the underlying device
  • [x] Entities have unique ID (if available) (docs)

Gold 🥇

This is a solid integration that is able to survive poor conditions and can be configured via the user interface.

  • [x] Satisfying all Silver level requirements. Configurable via config entries.
    • [x] Don't allow configuring already configured device/service (example: no 2 entries for same hub)
    • [x] Discoverable (if available)
    • [x] ~~Set unique ID in config flow (if available)~~
    • [x] ~~Raise ConfigEntryNotReady if unable to connect during entry setup (if appropriate)~~
  • [x] Entities have device info (if available) (docs) Tests
    • [x] Full test coverage for the config flow
    • [x] Above average test coverage for all integration modules 100% test coverage
    • [x] Tests for fetching data from the integration and controlling it (docs)
  • [x] Has a code owner (docs)
  • [x] Entities only subscribe to updates inside async_added_to_hass and unsubscribe inside async_will_remove_from_hass (docs)
  • [x] Entities have correct device classes where appropriate (docs)
  • [x] Supports entities being disabled and leverages Entity.entity_registry_enabled_default to disable less popular entities (docs)
  • [x] ~~If the device/service API can remove entities, the integration should make sure to clean up the entity and device registry.~~
    • Device cannot remove entities
  • [x] When communicating with a device or service, the integration implements the diagnostics platform which redacts sensitive information.

Platinum 🏆

Best of the best. The integration is completely async, meaning it's super fast. Integrations that reach platinum level will require approval by the code owner for each PR.

  • [x] Satisfying all Gold level requirements.
  • [x] Set appropriate PARALLEL_UPDATES constant (docs)
  • [x] Support config entry unloading (called when config entry is removed)
  • [x] Integration + dependency are async (docs)
  • [x] ~~Uses aiohttp or httpx and allows passing in websession (if making HTTP requests)~~
    • Integration does not use HTTP
  • [x] ~~Handles expired credentials (if appropriate) Re-authentication if cookie expires, initiates reauth flow if it fails~~
    • No credentials required

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:

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:

LennP avatar Jul 12 '24 21:07 LennP

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

Code owner commands

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

home-assistant[bot] avatar Jul 12 '24 21:07 home-assistant[bot]

Hey, can you send me a message on discord?

joostlek avatar Sep 24 '24 07:09 joostlek

Can I give one small feedback on the current state of the integration before marking it Platinum ? In my experience, the default setting is to not have a persistent connection to the motor, resulting in the state being unknown all the time, which is imo a bad user experience.

People (especially non-technical) get confused, is the curtain open or closed ? Is something wrong with it ?

I think it would be better if either the default setting is to have a persistent connection to the motor (so make people opt-out of that in exchange for some additional battery life) OR use optimistic states.

marcelveldt avatar Sep 24 '24 08:09 marcelveldt

Can I give one small feedback on the current state of the integration before marking it Platinum ? In my experience, the default setting is to not have a persistent connection to the motor, resulting in the state being unknown all the time, which is imo a bad user experience.

People (especially non-technical) get confused, is the curtain open or closed ? Is something wrong with it ?

I think it would be better if either the default setting is to have a persistent connection to the motor (so make people opt-out of that in exchange for some additional battery life) OR use optimistic states.

Of course! This was discussed with the manufacturer of the motor and highly discouraged as it would drain the motor very quickly, the motor and battery were simply not made for a permanent connection. Currently, the integration works similarly to the Motionblinds Bluetooth app in that it first requires you to connect to the blind after which the command is sent to the motor and states can be read. That said, the integration does have the option to enable a permanent connection which was included mainly for people who either have wired motors (meaning the battery life is not an issue) or people who find some other solution to the battery life issue (for example always leaving it plugged in and charging it using an automation).

As for using optimistic states, I think that would have been a good idea if the motor had the ability to occasionally transmit its state without a connection. However, unfortunately, that is currently not the case since the state can only be retrieved by manually connecting to the motor. You could therefore end up in a situation where the state still shows 'position X', while in the meantime the blind has already moved up and down hundreds of times. Therefore, I figured it would provide for a better user experience if the state simply goes to 'unknown', as that better reflects that HA actually has no way of guessing its state without the user triggering a connection to the motor. I actually made an exception for the battery here in that it does retain its state, simply because I figured the battery percentage is much more predictable. For example, if I see a battery percentage of 97% that was updated 1 week ago I know the battery won't be empty any time soon. On the other hand, if I see 'position X, updated 1 week ago', there is nothing much I can do with that information and I would much rather have the position be returned to 'unknown' to reflect the fact that HA does not know its state.

LennP avatar Sep 24 '24 11:09 LennP

I'll be putting this one to draft as we expect the new quality scale to be merged after the beta, after which every integration will have to be rechecked, so there is not a lot of point in doing that now

joostlek avatar Oct 28 '24 16:10 joostlek

@LennP thanks for the reply (and sorry for my late response to that). I have played a bit with a bluetooth motionblinds motor (or better said; I let my family test it) and I find the out of the box user experience suboptimal to say it mildly:

  • The state is always "unknown" instead of open/closed/opening/closing which make the user think something is wrong.
  • When you issue a command, nothing happens for 10 to 30 seconds)

This can be prevented by either having that persistent connection to the motor or by implementing optimistic states:

  • HA would simply remember the last known state.
  • If you press a button to make it open or close, it will immediately show a transitioning state like "opening" or "closing"

Given the fact that many people will use HA solely to control the motor (instead of the app directly), it will be less of an issue that the state is wrong. You could even consider making this part of the config flow for the device. Ask the user if a persistent connection or optimistic state should be used.

A Gold (or even Platinum) Quality Scale is also about providing a good user experience, which in this case is lacking, hence me proposing to give that a second look. You could even consider connecting to the motor once every hour just to check/correct the state. It will still drain the battery a bit more but not as bad as a persistent connection.

marcelveldt avatar Oct 29 '24 09:10 marcelveldt

The state is always "unknown" instead of open/closed/opening/closing which make the user think something is wrong.

In my opinion "unknown" accurately reflects the state of the blind, in that HA does not know the location of the blind anymore after it is disconnected. Motionblinds Bluetooth motors require a connection to get state updates, which is also mentioned in the docs:

Since Motionblinds Bluetooth motors require a Bluetooth connection to control them, Home Assistant does not get automatic updates of the motor’s state by default.

That said, I could also add an option to the options flow to control whether the user wants it to go to unknown or not? Or if you really insist on it I can make it not go to unknown, but again I personally don't think that accurately reflects what is happening. If you use the Motionblinds Bluetooth app the blind position will also be unknown at first, then it connects to the blind, and only after that can you control it.

When you issue a command, nothing happens for 10 to 30 seconds)

This is related to issue #128494. The reason currently nothing happens when you send a command is because the command is sent too fast after the connection is established (this was a side effect of the fix to #127664). I was waiting for some more input from someone from HA on how to best solve this, but using asyncio.sleep(0.2) after connection seems to solve the problem so I may as well push that fix

LennP avatar Oct 29 '24 10:10 LennP