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

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

Open
rsocko opened this issue Aug 28, 2023 · 11 comments
Open

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

rsocko opened this issue Aug 28, 2023 · 11 comments

Comments

@rsocko
Copy link

rsocko commented Aug 28, 2023

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
Copy link
Author

rsocko commented Aug 28, 2023

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

@rsocko
Copy link
Author

rsocko commented Aug 28, 2023

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
Copy link
Author

rsocko commented Oct 13, 2023

@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
Copy link

opq8 commented Oct 13, 2023

Thanks @rsocko -- I'm still pretty new with this integration, and I'm still running into #10 but this is what it looks like if I run either:

  • MizterB's latest homeassistant-phyn and aiophyn
  • your fork of homeassistant-phyn and aiophyn

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

a

@rsocko
Copy link
Author

rsocko commented Oct 13, 2023

@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
Copy link

opq8 commented Oct 13, 2023

@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
Copy link
Author

rsocko commented Oct 13, 2023

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
Copy link

opq8 commented Oct 13, 2023

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
Copy link

opq8 commented Oct 13, 2023

... 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
Copy link

opq8 commented Oct 13, 2023

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:

  • name (e.g. Bathroom Sink) because it is hard to disambiguate the various pw1 sensors.
  • sd_status v (I think this is the sensor tripping on a water leak?)
  • humidity
  • battery_level
  • signal_strength

@opq8
Copy link

opq8 commented Oct 13, 2023

OK, I created #12 on top of the main branch and tested my changes on both main and with @rsocko 's changes in #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!

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

No branches or pull requests

2 participants