Fix geniushub smart plug state at start-up #102110
Conversation
home-assistant
bot
added
bugfix
cla-signed
Hacktoberfest
integration: geniushub
small-pr
|
Hey there @manzanotti, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
MartinHjelmare
changed the title
|
@GeoffAtHome I'm a little concerned that the original comment explicitly rules out Timer as being on, any idea why that might be? @zxdavb Can you recall why Timer/on wasn't considered a switch being in the On state? |
|
@manzanotti I'm sorry - you might have to sign-post me to exact the piece of code... But, if it is this doctext you're referring to: IIRC, it is because that was the behaviour of the WebApp/v1 API at the time. I wanted the library to have a consistent schema/namespace/attributes/methods/etc., regardless of whether it is using the v1 or v3 API. Obviously, v3 will be a superset of v1. The two different APIs each offer their own advantages. The goal is that the user can choose either, or even seamlessly switch between. If this is the case, then choosing to diverge the two behaviours would be a breaking change. |
|
It is better to have it working correctly even if this is a breaking change. I will look at the code when I have a chance and see if the behaviour can be modified based on the API version. I think this should be possible. But until I investigate I don't know. @zxdavb thanks for explanation as I really wasn't sure why the code was written like it was. |
frenck
added
the
smash
|
It should be noted that this change is not to do with just startup status, it is changing how the switch behaves all the time (certainly for v3 API). Currently it only shows as 'on' if it is overridden on, and activating the switch will cause the plug to overridden (for the configured, in app, amount of time). The change will mean it will shows as 'on' when it is either overridden or in timer mode and is switched on. For me, I'm fine with that, but it is changing behaviour and as noted is a breaking change. PR #102112, which has already been merged, seems to be dependant on this PR to provide accurate plug status information using the switch entity, since 102112 removes the binary sensor that previously provided that function. Conversation also present on this issue - #103195 |
|
I think I have got to the bottom of this. I believe, and may be wrong, that state of the switch (ON | OFF) was also trying to reflect the mode of the zone (Off | Timer | Boost). If the mode of the zone is required this should be retrieved via a service call. I do have a longer goal of making the switch more useful but at this time I am struggle how to bring it inline with HA. I am going to raise this discussion else where as it not strictly related to this PR. I believe the code I submitted to be correct and should be merged. |
|
For my part I’m happy it is merged, and urgently because the related PR having been merged leaves no reliable on/off status for the plug. |
…lect-correct-state-on-start-up
…lect-correct-state-on-start-up
|
@manzanotti - please can you approve this PR. |
|
@GeoffAtHome I'll have a look (and test it) this evening, once I've got work and dinner out of the way. |
|
@GeoffAtHome So, I now have the smart plugs just appearing in the Binary Sensors. I'm going to have a look and see if I can work out why they aren't showing as switches, as I do agree with you that these should be classed as switches, not sensors. |
|
@GeoffAtHome So, bear with me, as this is the first time I've had a decent look at the HA integration (I'd only really worked on the genius client previously). My smart plugs can only show up as a binary sensor because they aren't assigned to a zone. They are therefore only in the broker.client.device_objs list that the Binary Sensors are created from and not in the broker.client.zone_objs list that the switches are created from. So, whilst this change does fix the issue @RogerSelwyn has, we do have another one to add to the list, namely switches that aren't in a zone. Then there's the additional issue of truly knowing if a switch zone is not. But, those are separate issues, so I'm happy for this to be merged in. |
|
You will need to create switches based on the list of devices, not on the zones if the plug is not in a zone. And then figure out how the switch will behave when toggled. Not sure you can do anything direct to a device, you can only override a zone, and if the plug has no zone, what does the switch do? If it does nothing, then is it a switch? |
|
@manzanotti this is a good corner case that I did not expect. That said, it should be fairly trivial to fix. We can iterate over devices to find the smart plugs (switches) and do the same for zones. From the results remove duplicates so we only create one switch for each smart plugs. Main difference is a smart plug that is not in a zone will appear as a switch with a device ID and not a zone name. I give this a go later and report back. |
|
@GeoffAtHome I am wondering if at least some of this logic should go into the client, rather than here. Obviously, there shouldn't be anything in the client that is HA-specific. But I do think that as part of the work I want to do to expose the V3 API via properties, and leave the JSON as the V1-compatible data, we could make the client do more of the work of indicating the true state of the system. And if we can improve on the official Genius functionality, I'm happy to do that. For example, putting my smart plug in a zone means that when I turn the switch on in HA, the plug only stays on for an hour, which isn't how I'd expect a switch to work. I get that's how zones work, and that is also how some people would want their switch to work. But it'd be good if we can introduce more control over switches. |
|
@manzanotti the switches have always been a bug bear of mine. I only got them as the suggestion was I needed three to cover the communications in the house. I have never tried not putting them in a zone and don't know what to expect. For me, I have schedule set up that keeps the switches on 24/7. Then I simple toggle between off and timer. I see little value in using the boost function. This way I can manage turning on and off the switch from HA. |
…h,-does-not-reflect-correct-state-on-start-up
|
The code is ready to to merge. Sorry I didn't mark it as ready. |
There was a problem hiding this comment.
Thanks!
Do we agree that this is a breaking change and should be highlighted as such in the release notes with a paragraph explaining the change for the users? If so, please add that to the PR description. I'll add the label. If it's a breaking change we'll wait with releasing the PR until the next minor release in December.
|
@MartinHjelmare in my view this fixes something that is now broken. So whilst in itself it is a breaking change, a related PR was merged that broke the integration, so this one is needed to bring it back to a working but different state. See this issue for more details - #103195 |
I failed to get any code for this as deleting a zone with a smart plug to try and simulate your setup give me too much grief. The smart plug got orphaned. Geniushub-client could not login into my hub. Powercycling the hub and a bug report the GH fixed the issue. I don't want to try that again! |
I understand that it fixes the integration for some users but it could also break assumptions for other users as I understand it from your posts. If that's true I suggest we label it as a breaking change and release in December. Otherwise we'd be making another unexpected breaking change. Two wrongs don't make a right, so to speak. If all of you think we should release it as a fix anyway, I'll concede. |
|
I’ll go with the PR creator and integration maintainer. In my view how the plugs work needs some serious thinking through all the different scenarios which I suspect will take some time. |
|
@manzanotti and @GeoffAtHome, what do you think? |
|
I too tired to really think so apologies now in case I am wrong. Agree with two wrongs don't make a right but in this case if the two PRs are released together it does. But I can't remember all the details. As I am currently running a private build selfishly this doesn't impact me. Just driven 12 hours from Cardiff to the Scrabster and currently waiting for a ferry and getting my head down. |
|
To be clear, #102112 was released in core-2023.11.0. So these PRs won't be released together. |
|
And that is the problem. Personally I would prefer that 102112 is reverted. Then a new PR raised with all the required changes in it after thinking through all the impacts. From the issue I raised I can see it is not just smart plugs that are impacted but also electric switches. As we go into winter I suspect people can’t see if the heating is on or not (assuming the switches are being used to drive under floor heating and hot water tanks). |
|
That's also a possibility. I suggest the three of you decide together what we should do. It's hard for me to judge the impact of the changes to these features. I can only make recommendations based on what you say. |
|
Again for @manzanotti and @GeoffAtHome to decide. I’ve stated my preference and I’m in a working condition because I’ve substituted the old code base. But I’m trying to think of others who won’t know how to bring in a working code base. |
|
With this one running locally, I think I have a Genius integration looking like it did before the first PR went in. Reading your initial comment about this pr correcting whether a switch is shown as being on correctly, I don't think this would be a breaking change, as it is fixing an issue where they aren't currently shown in the correct state. I'm struggling to think of a scenario where someone would be relying on the switch status being shown incorrectly when in timer mode. Do we have scenarios that we think we have broken, rather than fixed an issue? |
I don't know because I haven't tested all the scenarios. I've tested mine and it works for my scenario. Do we have somewhere the last comms date for the plugs, like we do for all the other devices in the system. Something to think about since these will be the only devices not represented by the integration? I can think of people using that to see if their system is broken, but of course that would be presented in an error as well, even if you don't have the date/time info. |
|
So, can we either reach a consensus that this is fine to be released, or decide what needs to be updated to get it into that state? @RogerSelwyn @GeoffAtHome |
|
@manzanotti , I’m leaving this to you guys since you are the code owner and @GeoffAtHome created the PR. For my part I am using a custom installation so I’ve got a working environment. I’ll work with whatever you two agree on. |
|
I believe that it is fine to release. I have been using it as a custom installation without any problems.
Sent from Outlook for Android<https://aka.ms/AAb9ysg>
…________________________________
From: RogerSelwyn ***@***.***>
Sent: Saturday, December 2, 2023 2:54:33 PM
To: home-assistant/core ***@***.***>
Cc: GeoffAtHome ***@***.***>; Mention ***@***.***>
Subject: Re: [home-assistant/core] Fix geniushub smart plug state at start-up (PR #102110)
@manzanotti<https://github.com/manzanotti> , I’m leaving this to you guys since you are the code owner and @GeoffAtHome<https://github.com/GeoffAtHome> created the PR. For my part I am using a custom installation so I’ve got a working environment. I’ll work with whatever you two agree on.
—
Reply to this email directly, view it on GitHub<#102110 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABVW2WB6AKMSCK3WF227N23YHM6KTAVCNFSM6AAAAAA6CKFW5OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZXGE3DMNRUGI>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
Should we add a breaking change paragraph or not? |
|
As the behaviour has changed, then this is a breaking change from before. People should be aware that the overcome is different. |
|
Ok. Please update the breaking change section in the PR description. |
|
Ping me when that's done as PR description updates don't generate notifications. |
|
Updated breaking change comment. |
|
Nice one, @GeoffAtHome , thanks for that. Ready for you now, @MartinHjelmare |
* Smart plug did state wrong at start-up * Update docstring to reflect code
Breaking change
The initial state of the smart plug did not reflect the same state as in the Genius Hub application. Correcting this creates a breaking change in that the behaviour is now correct but is different than before.
If you have an automation that relies on this state you should check in case this change has broken your automation.
Proposed change
When creating a switch the initial state was incorrectly set.
The proposed change was to correct the is_on method.
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.To help with the load of incoming pull requests: