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

Restore states for RFLink devices #18816

Merged
merged 9 commits into from Dec 11, 2018

Conversation

Projects
None yet
6 participants
@javicalle
Copy link
Contributor

javicalle commented Nov 29, 2018

Description:

This PR aims to add the ability to recover the previous state to RFLink devices.
RFLink devices do not have the ability to ask for their status, so in the reboots, the previous state is lost. This delivery gives these devices the ability to 'remember' the status on the reboots.
This PR comes from the failed PR #18208

Some of the points to review:

  • I don't think that will be a breaking change
  • If the PR goes live I will adjust the documentation to the new behavior (on reboots)
  • Sugestions and improvement proposals

My intention would be to release an initial version 'as is' and then work on some of the points of improvement:

  1. How to disable the behavior? My approach would be to make use of a new 'initial_value' attribute that, if informed, prevails over the previous value. I don't like the option to define a flag attribute to modify the behavior and I think that 'initial_value' is already used in other cases.
  2. The new behavior does not apply to devices of the sensor and binary_sensor type. In my opinion, I do not believe that the previous state should be restored in these cases, in the next update the correct state will be restored, regardless of the previous state.

Related issue (if applicable): fixes #

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>

Example entry for configuration.yaml (if applicable):

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example). N/A
  • New dependencies are only imported inside functions that use them (example). N/A
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py. N/A
  • New files were added to .coveragerc. N/A

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
"""Restore RFLink light brightness attribute."""
await super().async_added_to_hass()

old_state = await async_get_last_state(self.hass, self.entity_id)

This comment has been minimized.

@houndci-bot

houndci-bot Nov 29, 2018

undefined name 'async_get_last_state'

@javicalle javicalle referenced this pull request Nov 29, 2018

Closed

Restore states for RFLink devices #18208

1 of 3 tasks complete
@javicalle

This comment has been minimized.

Copy link
Contributor

javicalle commented Nov 30, 2018

Well, after the RestoreEntity refactor, now I am having a few Too many ancestors pylint errors:

************* Module homeassistant.components.switch.rflink
homeassistant/components/switch/rflink.py:70:0: R0901: Too many ancestors (8/7) (too-many-ancestors)
************* Module homeassistant.components.light.rflink
homeassistant/components/light/rflink.py:152:0: R0901: Too many ancestors (8/7) (too-many-ancestors)
homeassistant/components/light/rflink.py:158:0: R0901: Too many ancestors (8/7) (too-many-ancestors)
homeassistant/components/light/rflink.py:192:0: R0901: Too many ancestors (8/7) (too-many-ancestors)
homeassistant/components/light/rflink.py:244:0: R0901: Too many ancestors (8/7) (too-many-ancestors)

For all the other Travis errors, I will take a look when I have a break.

I think that refactor the inheritance for RFLink classes wil be a delicate task that I do not know if I can/must assume.
Code 'owner' has commented on several occasions that he can not devote time to it.

I will see what can I do.

@emontnemery

This comment has been minimized.

Copy link
Contributor

emontnemery commented Nov 30, 2018

@javicalle You can disable the warning like this:

# pylint: disable=too-many-ancestors
class MqttSwitch(MqttAvailability, MqttDiscoveryUpdate, MqttEntityDeviceInfo,
                 SwitchDevice, RestoreEntity):
@javicalle

This comment has been minimized.

Copy link
Contributor

javicalle commented Nov 30, 2018

Thanks @emontnemery
But, it's 'allowed' to do this kind of tricks?

@javicalle

This comment has been minimized.

Copy link
Contributor

javicalle commented Dec 2, 2018

At last!
I'm not sure what the problem with the pylint Travis' tests was. Locally they seemed to work well (testing separately).
I will go back to setting up a environment from zero because I think I had some kind of problem with the line breaks. Now I will work on the documentation.

@pvizeli

This comment has been minimized.

Copy link
Member

pvizeli commented Dec 3, 2018

Can you rebase your PR? There was a change in background and verify that it works with new handling?

javicalle added some commits Nov 29, 2018

Merge branch 'master' of https://github.com/home-assistant/home-assis…
…tant into dev

# Conflicts:
#	homeassistant/components/binary_sensor/point.py
#	homeassistant/components/cloud/__init__.py
#	homeassistant/components/cloud/prefs.py
#	homeassistant/components/frontend/__init__.py
#	homeassistant/components/light/fibaro.py
#	homeassistant/components/logbook.py
#	homeassistant/components/point/__init__.py
#	homeassistant/config_entries.py
#	homeassistant/const.py
#	homeassistant/helpers/service.py
#	requirements_all.txt
#	requirements_test_all.txt
@javicalle

This comment has been minimized.

Copy link
Contributor

javicalle commented Dec 3, 2018

OK. No problem, I am on it...

@javicalle javicalle force-pushed the javicalle:rflink_restore branch from 7756d09 to 8ae5e4c Dec 3, 2018

@javicalle

This comment has been minimized.

Copy link
Contributor

javicalle commented Dec 3, 2018

Can you rebase your PR? There was a change in background and verify that it works with new handling?

@pvizeli Done.
Anything else I can help?

Show resolved Hide resolved homeassistant/components/light/rflink.py Outdated
Show resolved Hide resolved homeassistant/components/light/rflink.py Outdated
Show resolved Hide resolved tests/components/cover/test_rflink.py Outdated
Show resolved Hide resolved tests/components/cover/test_rflink.py Outdated
Show resolved Hide resolved tests/components/cover/test_rflink.py Outdated
Show resolved Hide resolved tests/components/cover/test_rflink.py Outdated
Show resolved Hide resolved tests/components/cover/test_rflink.py Outdated
Show resolved Hide resolved tests/components/cover/test_rflink.py Outdated
Show resolved Hide resolved tests/components/cover/test_rflink.py Outdated

javicalle added some commits Dec 9, 2018

Remove all 'coroutine' decorator, replace for 'async def'
Replace all 'yield from' for 'await'
Replace 'hass.async_add_job' for 'hass.async_create_task'
@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Looks good! @emontnemery what do you think?

@emontnemery

This comment has been minimized.

Copy link
Contributor

emontnemery commented Dec 10, 2018

I also think it looks good!

@javicalle

This comment has been minimized.

Copy link
Contributor

javicalle commented Dec 11, 2018

Thanks!

@MartinHjelmare MartinHjelmare merged commit 61ca9bb into home-assistant:dev Dec 11, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 92.96%
Details

@wafflebot wafflebot bot removed the in progress label Dec 11, 2018

mxworm added a commit to mxworm/home-assistant that referenced this pull request Dec 11, 2018

Merge branch 'dev' into current
* dev: (26 commits)
  Move daikin to package (home-assistant#19187)
  Restore states for RFLink devices (home-assistant#18816)
  Add SCAN_INTERVAL (home-assistant#19186)
  Enable alarmdecoder to see open/close state of bypassed RF zones when armed (home-assistant#18477)
  Updated frontend to 20181211.0
  Fix cloud defaults (home-assistant#19172)
  TelldusLive config flow (home-assistant#18758)
  ZHA - Event foundation (home-assistant#19095)
  Add raw service data to event (home-assistant#19163)
  Updated frontend to 20181210.1
  Google assistant fix target temp for *F values. (home-assistant#19083)
  Fix lovelace save (home-assistant#19162)
  Drop OwnTracks bad packets (home-assistant#19161)
  Update translations
  Updated frontend to 20181210.0
  Lovelace using storage (home-assistant#19101)
  Update pygtfs to upstream's 0.1.5 (home-assistant#19151)
  Update radiotherm to 2.0.0 and handle change in tstat error detection (home-assistant#19107)
  Upgrade sphinx-autodoc-typehints to 1.5.2 (home-assistant#19140)
  Update geizhals dependency (home-assistant#19152)
  ...

@javicalle javicalle referenced this pull request Dec 21, 2018

Merged

Clean up RFLink tests and add two tests #19511

4 of 4 tasks complete

@balloob balloob referenced this pull request Jan 10, 2019

Merged

0.85.0 #19897

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment