Implement support for additional ecobee hold modes #40520

jamincollins · 2020-09-24T02:08:34Z

Breaking change

The current state of the ecobee integration always behaves as if the selected hold duration is Until next scheduled activity.

With this change the following settings are now respected:

2 hours
4 hours
Until I change it

Users relying on the previous behavior may need to update their automations to account for the support for the new settings.

Proposed Change

The ecobee thermostat has 5 different hold duration settings that a user can specify either on the unit itself or through the ecobee application. These are:

2 hours
4 hours
Until next scheduled activity
Until I change it
Ask every time

Leaving only Ask every time not implemented, which retains the existing behavior of Until next scheduled activity.

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)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

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
The code has been formatted using Black (black --fast 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

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.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

homeassistant · 2020-09-24T02:08:37Z

Hi @jamincollins,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

probot-home-assistant · 2020-09-24T02:10:24Z

Hey there @marthoc, mind taking a look at this pull request as its been labeled with an integration (ecobee) you are listed as a codeowner for? Thanks!
_{^{(message by CodeOwnersMention)}}

homeassistant · 2020-09-24T02:17:01Z

Hi @jamincollins,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

marthoc · 2020-09-27T14:48:03Z

@jamincollins The functionality of this PR looks good to me. You will need to fill out the PR template properly however and describe what it is that this PR is intended to achieve in order for a HA dev to merge it.

jamincollins · 2020-09-27T16:05:36Z

You will need to fill out the PR template properly however and describe what it is that this PR is intended to achieve in order for a HA dev to merge it.

@marthoc I've left items of the PR template at default that didn't seem to make sense or apply. For instance, am I suppoed to do anything with the Additional Information section? It doesn't seems to have anything that applies to this PR. Also on the Local tests pass. Your PR cannot be merged unless tests pass** checkbox, the local unit tests don't pass, several (unrelated to this change) fail.

So, if you can be more specific about what in the PR template needs to be filled out or corrected, I'd be happy to correct it.

Bre77

Seems good to me.

hakusaro · 2020-10-07T05:27:31Z

@jamincollins I think the best bet would just be to go check all of the boxes under checklist and hope for the best. I have literally no idea, but that's my only thought.

I'm really excited for this to get merged :)

homeassistant/components/ecobee/climate.py

bdraco · 2020-10-18T19:10:47Z

I think this needs to be labeled a breaking change since we used fixed behavior before and will now use the configured device behavior.

Please update the description or explain if otherwise.

bdraco

Please see comments above

hakusaro · 2020-10-18T19:59:59Z

I think this needs to be labeled a breaking change since we used fixed behavior before and will now use the configured device behavior.

At least in my experience, it's actually the opposite: ecobee isn't respecting settings that hass sets and automatically reverts the hold settings based on some weird interaction of user settings related to hold action and scheduling. I know people are wanting this for different reasons, but I can at least say that the current behavior I feel is "broken" for me, because I've only managed to get to the point where hass changes are applied for 24 hours max.

To be clear, I just want to make understanding clear -- code changes are code changes, and I agree in marking it as breaking (but at least, breaking it in a useful way).

jamincollins · 2020-10-18T21:19:29Z

I think this needs to be labeled a breaking change since we used fixed behavior before and will now use the configured device behavior.

Please update the description or explain if otherwise.

I'll change the description, but as @hakusaro has indicated the current implementation being static is broken. This largely corrects that.

bdraco · 2020-10-18T21:20:36Z

I think this needs to be labeled a breaking change since we used fixed behavior before and will now use the configured device behavior.
Please update the description or explain if otherwise.

I'll change the description, but as @hakusaro has indicated the current implementation being static is broken. This largely corrects that.

I can agree with that perspective, however we should still mark it as a breaking change since some users are going to be expecting the broken behavior.

jamincollins · 2020-11-05T15:45:11Z

Waiting on second opinion before making the requested changes.

zolstarym · 2020-12-19T06:04:37Z

The hold mode not being settable and defaulting to holding until next schedule change has been broken for almost 2 years now. Hoping that this gets fixed soon.

hakusaro · 2020-12-19T06:25:01Z

Since ecobee auth broke for everybody due to an unrelated issue, approximately two weeks ago, maybe we can just ship this?We can mark it as a breaking change, etc. Notationally it doesn't matter. I was just pointing out that it's more urgent because the current behavior is defective.

hakusaro · 2021-01-02T06:52:03Z

FYI: I'm going to be switching to Nest Thermostat E soonish (3-5 days). If any developer/contributor wants an Ecobee Lite3, I'd be happy to help get it into the hands of someone who wants to use it for development purposes. I can send my entire account too, if anyone wants to fiddle with temperature sensors (no guarantees that the two I have can be re-paired -- I'm pretty sure the answer is no, so I can just send my entire account instead).

bdraco · 2021-01-02T07:44:23Z

I've updated the text so its mergeable and will be seen in the release notes.

Once PR is rebased (https://developers.home-assistant.io/docs/development_catching_up/) to fix the conflict, this should be good to merge.

marthoc · 2021-01-02T13:43:01Z

@hakusaro I would happily take your hardware to use it to test! Thanks for the offer.

hakusaro · 2021-01-07T05:56:30Z

@marthoc you can email me at argo at hey dot com (from your github profile email) and we can see if this is the right way to go :)

* useEndTime2hour - 2 hours * useEndTime4hour - 4 hours * indefinite - Until I change it These changes have been tested with an ecobee3 lite running firmware version 4.5.81.200 Signed-off-by: Jamin W. Collins <[email protected]>

jamincollins · 2021-01-08T02:09:07Z

@bdraco should be good to go now

* 'dev' of https://github.com/home-assistant/core: (98 commits) Bump pymyq to 2.0.13 (home-assistant#44961) Add zwave to ozw migration (home-assistant#39081) Add pressure forecast to HA weather entity model (home-assistant#44965) Deduplicate MQTT entity discovery code (home-assistant#44970) Fix parameters when toggling light (home-assistant#44950) Move MQTT entity helpers to separate file (home-assistant#44838) Helpers type hint improvements (home-assistant#44964) Remove script/test (home-assistant#44967) Add MQTT Number (non optimistic) (home-assistant#44883) Fix Netatmo climate boost for valves (home-assistant#44957) Disambiguate Supervisor HTTPUnauthorized on user/password validation (home-assistant#44940) Add torrent id to Transmission events (home-assistant#44187) Prefix versions in system health (home-assistant#44921) Fix media renderers without volume control (home-assistant#44874) Fix wait_template incorrectly matching falsey values (home-assistant#44938) Upgrade youtube_dl to 2021.01.03 (home-assistant#44942) Upgrade discord.py to 1.6.0 (home-assistant#44941) Fix KNX cover state return open when unknown (home-assistant#44926) Use parent_id to find cause of logbook events with new contexts (home-assistant#44416) Implement support for additional ecobee hold modes (home-assistant#40520) ...

homeassistant added the cla-needed label Sep 24, 2020

probot-home-assistant bot added has-tests integration: ecobee small-pr PRs with less than 30 lines. labels Sep 24, 2020

jamincollins force-pushed the dev branch from 3db14cb to 1d3e5a5 Compare September 24, 2020 02:16

homeassistant added cla-signed and removed cla-needed labels Sep 24, 2020

jamincollins force-pushed the dev branch 4 times, most recently from 463e1e8 to a6c9ef4 Compare September 24, 2020 04:00

springstan changed the title ~~implement support for additional ecobee hold modes~~ Implement support for additional ecobee hold modes Sep 24, 2020

marthoc approved these changes Sep 27, 2020

View reviewed changes

Bre77 approved these changes Sep 28, 2020

View reviewed changes

cgarwood added the Ready for review label Oct 17, 2020

bdraco reviewed Oct 18, 2020

View reviewed changes

homeassistant/components/ecobee/climate.py Show resolved Hide resolved

bdraco reviewed Oct 18, 2020

View reviewed changes

homeassistant/components/ecobee/climate.py Show resolved Hide resolved

bdraco added the breaking-change label Oct 18, 2020

bdraco requested changes Oct 18, 2020

View reviewed changes

bdraco added the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Oct 18, 2020

bdraco removed Ready for review second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. labels Jan 2, 2021

jamincollins force-pushed the dev branch 2 times, most recently from b0c1639 to 03d53b9 Compare January 8, 2021 01:40

implement support for additional ecobee hold modes

283523f

* useEndTime2hour - 2 hours * useEndTime4hour - 4 hours * indefinite - Until I change it These changes have been tested with an ecobee3 lite running firmware version 4.5.81.200 Signed-off-by: Jamin W. Collins <[email protected]>

jamincollins force-pushed the dev branch from 03d53b9 to 283523f Compare January 8, 2021 01:48

bdraco approved these changes Jan 8, 2021

View reviewed changes

bdraco merged commit 3b184ad into home-assistant:dev Jan 8, 2021

geekofweek mentioned this pull request Feb 4, 2021

Ecobee hold modes with presets #45936

Closed

marthoc mentioned this pull request Feb 16, 2021

Fix climate hold bug in ecobee #46613

Merged

21 tasks

spacegaier mentioned this pull request Nov 23, 2021

ecobee integration page should have links to climate integration page home-assistant/frontend#10636

Closed

3 tasks

Uh oh!

Implement support for additional ecobee hold modes #40520

Implement support for additional ecobee hold modes #40520

Uh oh!

Conversation

jamincollins commented Sep 24, 2020 • edited by bdraco Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking change

Proposed Change

Type of change

Checklist

Uh oh!

homeassistant commented Sep 24, 2020

Uh oh!

probot-home-assistant bot commented Sep 24, 2020

Uh oh!

homeassistant commented Sep 24, 2020

Uh oh!

marthoc commented Sep 27, 2020

Uh oh!

jamincollins commented Sep 27, 2020

Uh oh!

Bre77 left a comment

Choose a reason for hiding this comment

Uh oh!

hakusaro commented Oct 7, 2020

Uh oh!

Uh oh!

Uh oh!

bdraco commented Oct 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdraco left a comment

Choose a reason for hiding this comment

Uh oh!

hakusaro commented Oct 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamincollins commented Oct 18, 2020

Uh oh!

bdraco commented Oct 18, 2020

Uh oh!

jamincollins commented Nov 5, 2020

Uh oh!

zolstarym commented Dec 19, 2020

Uh oh!

hakusaro commented Dec 19, 2020

Uh oh!

hakusaro commented Jan 2, 2021

Uh oh!

bdraco commented Jan 2, 2021

Uh oh!

marthoc commented Jan 2, 2021

Uh oh!

hakusaro commented Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamincollins commented Jan 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

jamincollins commented Sep 24, 2020 •

edited by bdraco

Loading

bdraco commented Oct 18, 2020 •

edited

Loading

hakusaro commented Oct 18, 2020 •

edited

Loading

hakusaro commented Jan 7, 2021 •

edited

Loading