Add Bbox Router bandwidth as sensors #3956
Conversation
Add possibility to monitor max and currently used bandwidth of your xdsl connection for Bbox Routeur
|
@HydrelioxGitHub, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @robbiet480 and @fabaff to be potential reviewers. |
| @@ -0,0 +1,144 @@ | |||
| """ | |||
| Support for Bbox Bouygues Modem Routeur. | |||
There was a problem hiding this comment.
Oups translating mistake, thank you.
| @@ -228,6 +228,10 @@ | |||
| VOLUME_GALLONS = 'gal' # type: str | |||
| VOLUME_FLUID_OUNCE = 'fl. oz.' # type: str | |||
|
|
|||
| # Bandwidth units | |||
| BANDWIDTH_MEGABITS_SECONDS = 'Mbps' # type: str | |||
| BANDWIDTH_GIGABITS_SECONDS = 'Gbps' # type: str | |||
There was a problem hiding this comment.
Other sensors are already using the form MB/s and not the abbreviation Mbps. I think that we should stick with the existing format.
There was a problem hiding this comment.
So why there is not unit of this kind in the const.py ?
Anyway Mb/s and MB/s are not the same unit. The small b means bit, the other (B) means byte (https://www.softperfect.com/contact/knowledgebase.php?article=10)
There was a problem hiding this comment.
Because if only one platform need something we don't add it to const.py. Then the constants should only be defined in the platform itself. Well, it looks like that we are not consistent with those units anyway.
I was talking about the form for the units...
There was a problem hiding this comment.
Ok maybe for this PR I will put these units in the sensor file. I will look for other composent that used this kind of units. If there are a few component usine thèses, I will make another PR to gather all in the const.py file
There was a problem hiding this comment.
Sounds like a plan. See tracking issue.
| @@ -385,3 +389,4 @@ | |||
| TEMPERATURE = 'temperature' # type: str | |||
| SPEED_MS = 'speed_ms' # type: str | |||
| ILLUMINANCE = 'illuminance' # type: str | |||
| BANDWIDTH = 'bandwidth' # type: str | |||
There was a problem hiding this comment.
It doesn't make much sense to add constants which are not used. If needed new constants can be added every time.
There was a problem hiding this comment.
I planned to use it, but not on this PR. And I forgot to remove it on the commit. Thanks again
| @property | ||
| def state(self): | ||
| """Return the state of the sensor.""" | ||
| return round(self._state, 2) |
There was a problem hiding this comment.
You should move the rounding to the calculations in update().
There was a problem hiding this comment.
I think rouding and divided by 1000 should both be in the state method because the update purpose is just to refresh data and not not make some transformation on it.
But I'm maybe wrong. Can you add some details on why this should be at one place or not ? (It will be useful for me for future development)
There was a problem hiding this comment.
The reason to keep it at one place is simple, better maintainability. No need to search over the whole file if you want to change something. Most platforms are doing it in update() and just returning the state here.
| # Create a data fetcher to support all of the configured sensors. Then make | ||
| # the first call to init the data. | ||
| try: | ||
| bbox_data = BboxData() |
There was a problem hiding this comment.
No configuration needed? No password, username, IP of the router, not even defaults?
There was a problem hiding this comment.
No because the bbox is provided to be plug and play so IP the box give to itself is always 192.168.1.254 . And credentials are only needed for critical operation (reboot of the router for example). Restriction is you can only use this API on your local network
Unit constant get back into the main sensor file
Description:
Add possibility to monitor max and currently used bandwidth of your xdsl connection for Bbox Routeur
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#1292
Example entry for
configuration.yaml(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
toxrun successfully. Your PR cannot be merged unless tests passREQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.