MizterB / homeassistant-phyn

16 stars 12 forks source link

Add a feature to control away mode for the Phyn Plus device #5

Open rsocko opened 1 year ago

rsocko commented 1 year ago

Corresponding to the feature in aiophyn this feature would add a switch entity to the Phyn device in Home Assistant in order to toggle on/off the away mode.

Switch On = Away Mode Active Switch Off = Away Mode Inactive / Disabled

This would allow not only monitoring and awareness of the away mode setting, but it could then be used in conjunction with other triggers, scenes and automations to protect your home as well as not cause unexpected functionality.

Example Automation Scenarios:

  1. Disable automated home water use when Away Mode is turned On (for example hot water recirculation, sprinkler system, pet and plant watering systems)
  2. Use GeoLocation / Presence sensors to automatically enable Away Mode when no one is home / everyone leaves and to enable when people return.
  3. Disable automatic away mode when 'guest' mode is enabled so it doesn't inadvertently disable water with a guest in your home
rsocko commented 1 year ago

I've started a forked branch to work on this feature.

rsocko commented 1 year ago

I have implemented this new feature in my forked branch and will submit prepare to submit a pull request to merge this new functionality into the integration.

Note that for testing it required pointing adding my own github repository to HACs to install/download the updated integration.

Then I also needed to adjust the manifest.json file to point to my forked repository for the modified aiophyn module and also to update the version number of that module for it to force HA to pull the newer aiophyn module with the updated method.

I had trouble choosing the right icons to show for away mode. I ended up choosing the suitcase to signify AWAY (on) and home cirlce for HOME (off).

It might be more clear to choose airplane for the AWAY (on) icon.

An example of the UX showing the new switch is here

@MizterB - let me know if you want me to work in a particular way to submit the pull request or have any other feedback.

rsocko commented 11 months ago

@opq8 - I don't actually have any of the addon sensors. I currently only have a single Phyn Plus device in each of my properties.

I had debugged the aiophyn library that calls the Phyn API to determine the response data and derive the get_away_mode attributes. But I didn't take into account the possibility that it would not be included in the payload.

I infer from your note that the phyn integration enumerates all sensors and the addons must not have an away mode.

I will perhaps try two steps:

  1. See if I can enhance the sample / test harness for the aiophyn library - so that you and users with different configurations could more easily provide debug info / diagnostic details of the api payloads so that the integration can be enhance more predictably.
  2. Add smarter logic / error handling in the event that a phyn device doesn't expose away mode (and likely to either disable that entity or remove it entirely from the device).

A couple of questions for you to perhaps clarify: without my fork, does Home Assistant show a device for each of your sensors (main and addon)? Are there any different, additional entities on the addon devices that I should be aware of?

opq8 commented 11 months ago

Thanks @rsocko -- I'm still pretty new with this integration, and I'm still running into https://github.com/MizterB/homeassistant-phyn/issues/10 but this is what it looks like if I run either:

(and in both cases, with the workaround I wrote in #10)

a

rsocko commented 11 months ago

@opq8 - you said you implemented a work-around. Are you able to share with me where you get the error occurring? Is it during the initialization of the integration? I suspect its either

In the device.py file (_update_away_mode method) which in turn is calling the API (line 141)

or is it when retrieving the property value in device.py line 121

opq8 commented 11 months ago

@rsocko Sorry the workaround wasn't for the issue I reported here with the Phyn sensors getting errors. It was a crash I reported in #10 .

I need to onion peel a little more before I can get back to the crash I reported with the above.

rsocko commented 11 months ago

No problem. I didn't crack open the code to debug yet and was just analyzing. I might push a "debugging" version of the feature to have you validate against your config since I don't have the add on devices to test against locally.

opq8 commented 11 months ago

Thanks! Looking at my new issue #11 I'm starting to wonder if the whole integration was written assuming no water sensors (pw1) devices which don't have sov_status or flow attributes....

opq8 commented 11 months ago

... yeah, so that's the issue. I don't think this integration works at all with a Phyn setup where it's more than just the Phyn Plus (aka there are water sensors)...

async_add_entities(entities) tries to add the same set in sensor.py and switch.py regardless of the device being a pp1/2 (the Phyn Plus) vs. a pw1 (water sensor)

In case it's helpful -- here are the attributes taken from my logs:

For a water sensor (PW1)

2023-10-12 22:28:13.312 DEBUG (MainThread) [custom_components.phyn] Phyn device state: {'battery_level': {'v': 100, 'ts': REDACTED}, 'signal_strength': 71, 'online_status': {'ts': REDACTED, 'v': 'online'}, 'sd_status': {'ts': REDACTED, 'v': 'G'}, 'hw_version': '0', 'device_id': 'REDACTED', 'product_code': 'PW1', 'network_name': 'REDACTED', 'temperature': {'v': REDACTED, 'ts': REDACTED}, 'name': 'REDACTED', 'auto_shutoff_eligible': 0, 'serial_number': 'REDACTED', 'fw_version': 'REDACTED', 'timezone': 'America/Los_Angeles', 'partner': 'phyn', 'humidity': {'v': REDACTED, 'ts': REDACTED}, 'users': ['REDACTED'], 'auto_shutoff_enable': False, 'created_ts': REDACTED}

For the Phyn Plus itself (pp1 or pp2, example taken from pp1)

2023-10-12 22:28:13.407 DEBUG (MainThread) [custom_components.phyn] Phyn device state: {'signal_strength': -65, 'online_status': {'v': 'online', 'sid': 'REDACTED', 'ts': REDACTED}, 'sd_status': {'v': 'G', 'r': 'watchdog', 'ts': REDACTED}, 'hw_version': '0', 'device_id': 'REDACTED', 'product_code': 'PP1', 'network_name': 'REDACTED', 'temperature': {'min': REDACTED, 'max': REDACTED, 'mean': REDACTED, 'ts': REDACTED}, 'auto_shutoff_eligible': 100, 'serial_number': 'REDACTED', 'sov_status': {'a': 'REDACTED', 'v': 'Open', 'ts': REDACTED}, 'flow': {'min': REDACTED, 'max': REDACTED, 'mean': REDACTED, 'ts': REDACTED}, 'pressure': {'min': REDACTED, 'median': REDACTED, 'max': REDACTED, 'mean': REDACTED, 'percentile95': REDACTED, 'pressure_threshold_95': REDACTED, 'percentile5': REDACTED, 'ts': REDACTED}, 'fw_version': 'REDACTED', 'timezone': 'America/Los_Angeles', 'partner': 'phyn', 'users': ['REDACTED'], 'auto_shutoff_enable': True, 'created_ts': REDACTED}

So yeah, we might have to fix this for the entire integration and not just your proposed changes...

opq8 commented 11 months ago

OK my last update for tonight -- I managed to fix it both for @MizterB's master and for @rsocko's changes:

The easiest fix is in switch.py and sensor.py, to add checks to async_setup_entry() to only do entities.extend() if device.model (product_code) is 'PP1' or 'PP2'.

switch.py

    for device in devices:
        if device.model == 'PP1' or device.model == 'PP2':
            entities.extend(
                [
                    PhynValveSwitch(device),
                    PhynAwayModeSwitch(device)
                ]
            )
    async_add_entities(entities)

sensor.py

    for device in devices:
        if device.model == 'PP1' or device.model == 'PP2':
            entities.extend(
                [
                    PhynDailyUsageSensor(device),
                    PhynCurrentFlowRateSensor(device),
                    PhynTemperatureSensor(NAME_WATER_TEMPERATURE, device),
                    PhynPressureSensor(device),
                ]
            )
    async_add_entities(entities)

Later on, to extend the integration to report useful things for water sensors, we can else if 'PW1'. Interesting attributes to report out would be:

opq8 commented 11 months ago

OK, I created https://github.com/MizterB/homeassistant-phyn/pull/12 on top of the main branch and tested my changes on both main and with @rsocko 's changes in https://github.com/MizterB/homeassistant-phyn/pull/6 and my Phyn setup works great.

@MizterB -- @rsocko 's PR has been open since August. Can we accept and merge? I would like to do work to add Water Sensor (pw1) support in the coming weeks so it'd be great to have a single clean master. Thanks!