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

TekSavvy Sensor unlimited bandwidth support #12325

Merged
merged 2 commits into from Mar 1, 2018

Conversation

Projects
None yet
3 participants
@mikeodr
Copy link
Contributor

commented Feb 11, 2018

Description:

Add support for ISP TekSavvy sensor on uncapped plans.
Unit tests added for TekSavvy component, none were present previously.

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#4645

Breaking Change:

The sensor entity id for peak upload usage used to be sensor.teksavvy_on_peak_upload_ this has been changed to sensor.teksavvy_on_peak_upload

The usage title was shared between and therefore indeterminate between GB and % usage. Therefore % usage entity ID has been changed to sensor.teksavvy_usage_ratio

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: teksavvy
    api_key:  API_KEY_HERE
    total_bandwidth: 0
    monitored_variables:
      - usage
      - usage_gb
      - limit
      - onpeak_download
      - onpeak_upload
      - onpeak_total
      - offpeak_download
      - offpeak_upload
      - offpeak_total
      - onpeak_remaining

Checklist:

  • The code change is tested and works locally.

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

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

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@mikeodr mikeodr referenced this pull request Feb 11, 2018

Merged

Update TekSavvy sendor documentation. #4645

2 of 2 tasks complete

@mikeodr mikeodr force-pushed the mikeodr:teksavvy-unlimited-bw-support branch Feb 11, 2018

'usage_gb': ['Usage', GIGABYTES, 'mdi:download'],
'limit': ['Data limit', GIGABYTES, 'mdi:download'],
'onpeak_download': ['On Peak Download', GIGABYTES, 'mdi:download'],
'onpeak_upload': ['On Peak Upload ', GIGABYTES, 'mdi:upload'],
'onpeak_upload': ['On Peak Upload', GIGABYTES, 'mdi:upload'],

This comment has been minimized.

Copy link
@mikeodr

mikeodr Feb 11, 2018

Author Contributor

This and line 35 would be breaking changes?

How do I ensure that makes it into the release notes?

This comment has been minimized.

Copy link
@mikeodr

mikeodr Feb 11, 2018

Author Contributor

Notably sensor.teksavvy_on_peak_upload_ (which is incorrect with the trailing underscore) becomes sensor.teksavvy_on_peak_upload

Additionally with both the percentage and GB values both being named usage you would get a non-deterministic result in tests for sensor.teksavvy_usage and sensor.teksavvy_usage_2.
Renaming to Usage Ratio makes it very deterministic.

This comment has been minimized.

Copy link
@balloob

balloob Feb 12, 2018

Member

Add a section to your PR description and I'll add the label "Breaking Change"

This comment has been minimized.

Copy link
@mikeodr

mikeodr Feb 12, 2018

Author Contributor

Added a description, thanks!

@mikeodr mikeodr force-pushed the mikeodr:teksavvy-unlimited-bw-support branch Feb 12, 2018

@mikeodr mikeodr force-pushed the mikeodr:teksavvy-unlimited-bw-support branch Feb 12, 2018

@mikeodr mikeodr force-pushed the mikeodr:teksavvy-unlimited-bw-support branch Feb 12, 2018

@mikeodr mikeodr force-pushed the mikeodr:teksavvy-unlimited-bw-support branch Feb 12, 2018

@mikeodr

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2018

Test seems to be failing on 3.4.2 due to some SQL timeout issue... unsure what that is about.

homeassistant/components/sensor/teksavvy.py Outdated
@@ -18,6 +18,11 @@
import homeassistant.helpers.config_validation as cv
from homeassistant.helpers.entity import Entity
from homeassistant.util import Throttle
# Python 3.4 support
try:
from json.decoder import JSONDecodeError

This comment has been minimized.

Copy link
@balloob

balloob Feb 12, 2018

Member

Use ValueError. JSONDecodeError is new in Python 3.4.

(or wait 2 weeks and we drop Py34 support)

This comment has been minimized.

Copy link
@mikeodr

mikeodr Feb 12, 2018

Author Contributor

Dropped to ValueError only.

@mikeodr mikeodr force-pushed the mikeodr:teksavvy-unlimited-bw-support branch Feb 12, 2018

homeassistant/components/sensor/teksavvy.py Outdated
self.data["offpeak_total"] = off_peak_download + off_peak_upload
self.data["onpeak_remaining"] = limit - on_peak_download
return True
else:

This comment has been minimized.

Copy link
@balloob

balloob Feb 12, 2018

Member

This else is not needed

This comment has been minimized.

Copy link
@mikeodr

mikeodr Feb 12, 2018

Author Contributor

Ah yes, I see it. Will update.

homeassistant/components/sensor/teksavvy.py Outdated
off_peak_upload
self.data["onpeak_remaining"] = limit - on_peak_download
return True
except ValueError:

This comment has been minimized.

Copy link
@balloob

balloob Feb 12, 2018

Member

What can cause a JSON decode error?

This comment has been minimized.

Copy link
@mikeodr

mikeodr Feb 12, 2018

Author Contributor

It's an edge case, assuming the API responds with HTTP OK/200 but the payload data isn't proper JSON. Then this would catch it.

homeassistant/components/sensor/teksavvy.py Outdated
off_peak_upload = self.data["offpeak_upload"]
limit = self.data["limit"]
# Support "unlimited" users
self.data["usage"] = 100*on_peak_download/self.bandwidth_cap\

This comment has been minimized.

Copy link
@balloob

balloob Feb 12, 2018

Member

Please rewrite as normal if/else if the inline if/else has to span multiple lines.

homeassistant/components/sensor/teksavvy.py Outdated
if self.bandwidth_cap > 0 else 0
self.data["usage_gb"] = on_peak_download
self.data["onpeak_total"] = on_peak_download + on_peak_upload
self.data["offpeak_total"] = off_peak_download +\

This comment has been minimized.

Copy link
@balloob

balloob Feb 12, 2018

Member

Can you put the \ after the = ?

tests/components/sensor/test_teksavvy.py Outdated


@asyncio.coroutine
def test_bad_json_decond(hass, aioclient_mock):

This comment has been minimized.

Copy link
@balloob

balloob Feb 12, 2018

Member

decode

This comment has been minimized.

Copy link
@mikeodr

mikeodr Feb 12, 2018

Author Contributor

Will fix, oops

mikeodr added some commits Feb 11, 2018

Support TekSavvy Unlimited Plans
Support TekSavvy account usage for unlimited plans.
Seeing cap limit to 0 will now provide unlimited behaviour on usage calculations.
Add unit tests to sensor.teksavvy
Add coverage unit tests to TekSavvy Sensor component, none existing previously.

@mikeodr mikeodr force-pushed the mikeodr:teksavvy-unlimited-bw-support branch to e504c79 Feb 12, 2018

@mikeodr

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2018

Poke for updated review.

@mikeodr

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2018

The documentation update seems merged and live for 0.64, but this not being merged makes it but match.

Not a huge deal, but if a user sets 0 for unlimited they would recieve a division by zero exception.

@balloob

balloob approved these changes Mar 1, 2018

@balloob balloob merged commit 3416d3f into home-assistant:dev Mar 1, 2018

5 checks passed

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.01%) to 94.071%
Details
hound No violations found. Woof!

@balloob balloob referenced this pull request Mar 9, 2018

Merged

0.65 #12995

@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.