TekSavvy Sensor unlimited bandwidth support #12325

mikeodr · 2018-02-11T23:44:49Z

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:

Documentation added/updated in home-assistant.github.io

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 · 2018-02-11T23:49:07Z

homeassistant/components/sensor/teksavvy.py

    '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 and line 35 would be breaking changes?

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

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.

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

Added a description, thanks!

mikeodr · 2018-02-12T01:48:59Z

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

balloob · 2018-02-12T04:31:23Z

homeassistant/components/sensor/teksavvy.py

@@ -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


Use ValueError. JSONDecodeError is new in Python 3.4.

(or wait 2 weeks and we drop Py34 support)

Dropped to ValueError only.

balloob · 2018-02-12T04:52:50Z

homeassistant/components/sensor/teksavvy.py

-        self.data["offpeak_total"] = off_peak_download + off_peak_upload
-        self.data["onpeak_remaining"] = limit - on_peak_download
-        return True
+        else:


This else is not needed

Ah yes, I see it. Will update.

balloob · 2018-02-12T04:53:18Z

homeassistant/components/sensor/teksavvy.py

+                    off_peak_upload
+                self.data["onpeak_remaining"] = limit - on_peak_download
+                return True
+            except ValueError:


What can cause a JSON decode error?

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.

balloob · 2018-02-12T04:53:53Z

homeassistant/components/sensor/teksavvy.py

+                off_peak_upload = self.data["offpeak_upload"]
+                limit = self.data["limit"]
+                # Support "unlimited" users
+                self.data["usage"] = 100*on_peak_download/self.bandwidth_cap\


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

balloob · 2018-02-12T04:54:29Z

homeassistant/components/sensor/teksavvy.py

+                    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 +\


Can you put the \ after the = ?

balloob · 2018-02-12T04:55:04Z

tests/components/sensor/test_teksavvy.py

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


Will fix, oops

Support TekSavvy account usage for unlimited plans. Seeing cap limit to 0 will now provide unlimited behaviour on usage calculations.

Add coverage unit tests to TekSavvy Sensor component, none existing previously.

mikeodr · 2018-02-26T12:50:05Z

Poke for updated review.

mikeodr · 2018-02-26T12:53:29Z

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.

homeassistant added cla-signed platform: sensor.teksavvy labels Feb 11, 2018

mikeodr mentioned this pull request Feb 11, 2018

Update TekSavvy sendor documentation. home-assistant/home-assistant.io#4645

Merged

2 tasks

homeassistant added the cla-signed label Feb 11, 2018

mikeodr commented Feb 11, 2018

View reviewed changes

homeassistant added cla-signed labels Feb 12, 2018

balloob reviewed Feb 12, 2018

View reviewed changes

balloob added the breaking-change label Feb 12, 2018

homeassistant added the cla-signed label Feb 12, 2018

balloob reviewed Feb 12, 2018

View reviewed changes

mikeodr added 2 commits February 12, 2018 10:47

Support TekSavvy Unlimited Plans

e95aa96

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

e504c79

Add coverage unit tests to TekSavvy Sensor component, none existing previously.

homeassistant added the cla-signed label Feb 12, 2018

balloob approved these changes Mar 1, 2018

View reviewed changes

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

balloob mentioned this pull request Mar 9, 2018

0.65 #12995

Merged

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

ghost added integration: teksavvy and removed platform: sensor.teksavvy labels Mar 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TekSavvy Sensor unlimited bandwidth support #12325

TekSavvy Sensor unlimited bandwidth support #12325

mikeodr commented Feb 11, 2018 •

edited

mikeodr Feb 11, 2018

mikeodr Feb 11, 2018

balloob Feb 12, 2018

mikeodr Feb 12, 2018

mikeodr commented Feb 12, 2018

balloob Feb 12, 2018

mikeodr Feb 12, 2018

balloob Feb 12, 2018

mikeodr Feb 12, 2018

balloob Feb 12, 2018

mikeodr Feb 12, 2018

balloob Feb 12, 2018

balloob Feb 12, 2018

balloob Feb 12, 2018

mikeodr Feb 12, 2018

mikeodr commented Feb 26, 2018

mikeodr commented Feb 26, 2018



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

TekSavvy Sensor unlimited bandwidth support #12325

TekSavvy Sensor unlimited bandwidth support #12325

Conversation

mikeodr commented Feb 11, 2018 • edited

Description:

Breaking Change:

Example entry for configuration.yaml (if applicable):

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeodr commented Feb 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeodr commented Feb 26, 2018

mikeodr commented Feb 26, 2018

mikeodr commented Feb 11, 2018 •

edited

Example entry for `configuration.yaml` (if applicable):