Skip to content

Conversation

@rytilahti
Copy link
Member

@rytilahti rytilahti commented May 16, 2020

A single update() will now fetch information from all interesting modules,
including the current device state and the emeter information.

In practice, this will allow dropping the number of I/O reqs per homeassistant update cycle to 1,
which is paramount at least for bulbs which are very picky about sequential accesses.
This can be further extend to other modules/methods, if needed.

This will also remove the implicit update() calls from device state changing methods such as set_brightness(). This allows executing multiple changes on a device without unnecessary I/O.

Currently fetched data:

  • sysinfo
  • realtime, today's and this months emeter stats

New properties:

  • emeter_realtime: return the most recent emeter information, update()-version of get_emeter_realtime()
  • emeter_today: returning today's energy consumption
  • emeter_this_month: same for this month

Other changes:

  • Accessing @requires_update properties will cause SmartDeviceException if the device has not ever been update()d
  • Fix repr for devices that haven't been updated
  • Smartbulb uses now the state data from get_sysinfo instead of separately querying the bulb service
  • SmartStrip's state_information no longer lists onsince for separate plugs
  • The above mentioned properties are now printed out by cli
  • Simplify is_on handling for bulbs

Closes #53, closes #61.

@rytilahti rytilahti requested review from basnijholt and kirichkov May 16, 2020 21:34
@rytilahti
Copy link
Member Author

rytilahti commented May 16, 2020

This is the last showstopper I wanted to tackle before making a release. It may not be be prettiest code you have ever seen, but I think it's the time to get the first (pre-)release out and let the downstream users to enjoy the improvements, at last..

edit: I think there is one more thing that should be done pre-release: getting rid of implicit update()s. If automatic I/O ops after a change are really wanted, we can have a flag for that, but otherwise I think it should be the responsibility of the end user to do an explicit update. Opionions?

rytilahti added a commit to rytilahti/home-assistant that referenced this pull request May 16, 2020
the backend lib will after python-kasa/python-kasa#59
fetch all the necessary wanted in a single update cycle.
@kirichkov
Copy link
Contributor

I'm in favor of removing the automatic updates everywhere. I think it should be up to the user of the library to determine when its best to ask for information.

rytilahti added 2 commits May 20, 2020 21:18
A single update() will now fetch information from all interesting modules,
including the current device state and the emeter information.

In practice, this will allow dropping the number of I/O reqs per homeassistant update cycle to 1,
which is paramount at least for bulbs which are very picky about sequential accesses.
This can be further extend to other modules/methods, if needed.

Currently fetched data:
* sysinfo
* realtime, today's and this months emeter stats

New properties:
* emeter_realtime: return the most recent emeter information, update()-version of get_emeter_realtime()
* emeter_today: returning today's energy consumption
* emeter_this_month: same for this month

Other changes:
* Accessing @requires_update properties will cause SmartDeviceException if the device has not ever been update()d
* Fix __repr__ for devices that haven't been updated
* Smartbulb uses now the state data from get_sysinfo instead of separately querying the bulb service
* SmartStrip's state_information no longer lists onsince for separate plugs
* The above mentioned properties are now printed out by cli
* Simplify is_on handling for bulbs
@rytilahti rytilahti force-pushed the refactor/request_combine branch from 9c641a5 to 4faa24a Compare May 20, 2020 19:33
@codecov-commenter
Copy link

codecov-commenter commented May 20, 2020

Codecov Report

Merging #59 into master will decrease coverage by 2.46%.
The diff coverage is 76.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
- Coverage   78.73%   76.27%   -2.47%     
==========================================
  Files           7        7              
  Lines         790      826      +36     
  Branches      104      110       +6     
==========================================
+ Hits          622      630       +8     
- Misses        150      172      +22     
- Partials       18       24       +6     
Impacted Files Coverage Δ
kasa/smartdevice.py 79.87% <72.00%> (-3.71%) ⬇️
kasa/smartbulb.py 92.96% <87.50%> (-2.56%) ⬇️
kasa/smartdimmer.py 88.23% <100.00%> (-0.34%) ⬇️
kasa/smartplug.py 100.00% <100.00%> (ø)
kasa/smartstrip.py 80.00% <100.00%> (-3.79%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 012436c...4faa24a. Read the comment docs.

@rytilahti
Copy link
Member Author

I presume that we can merge this now (and fix any issues in future PRs).

@rytilahti rytilahti merged commit 836f170 into python-kasa:master May 24, 2020
@rytilahti rytilahti deleted the refactor/request_combine branch May 24, 2020 15:57
rytilahti added a commit to rytilahti/home-assistant that referenced this pull request May 24, 2020
the backend lib will after python-kasa/python-kasa#59
fetch all the necessary wanted in a single update cycle.
rytilahti added a commit to rytilahti/home-assistant that referenced this pull request Jun 4, 2020
the backend lib will after python-kasa/python-kasa#59
fetch all the necessary wanted in a single update cycle.
rytilahti added a commit to rytilahti/home-assistant that referenced this pull request Jun 30, 2020
the backend lib will after python-kasa/python-kasa#59
fetch all the necessary wanted in a single update cycle.
rytilahti added a commit to rytilahti/home-assistant that referenced this pull request Jul 23, 2020
the backend lib will after python-kasa/python-kasa#59
fetch all the necessary wanted in a single update cycle.
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

Successfully merging this pull request may close these issues.

RFC: remove implicit updates after state changes? Request all necessary information during update()

3 participants