Skip to content

[WIP] Add Fido sensor#5997

Merged
pvizeli merged 3 commits into
home-assistant:devfrom
titilambert:fido
Feb 15, 2017
Merged

[WIP] Add Fido sensor#5997
pvizeli merged 3 commits into
home-assistant:devfrom
titilambert:fido

Conversation

@titilambert
Copy link
Copy Markdown
Contributor

@titilambert titilambert commented Feb 14, 2017

Description:
Get your current talk/text/data usage from your Fido (http://fido.ca) account

Pull request in home-assistant.github.io with documentation: home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: fido
    username: YOUPHONENUMBER
    password: PASSWORD
    monitored_variables:
     - fido_dollar
     - balance
     - data_used
     - data_limit
     - data_remaining
     - text_used
     - text_limit
     - text_remaining
     - mms_used
     - mms_limit
     - mms_remaining
     - text_int_used
     - text_int_limit
     - text_int_remaining
     - talk
     - talk_limit
     - talt_remaining
     - talk_other
     - talk_other_limit
     - talt_other_remaining

Checklist:

  • Documentation added/updated in home-assistant.github.io
  • 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.

@mention-bot
Copy link
Copy Markdown

@titilambert, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @fabaff and @robbiet480 to be potential reviewers.

Comment thread homeassistant/components/sensor/fido.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undefined name 'PyEboxError'

Comment thread homeassistant/components/sensor/fido.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'pyfido.client.PyFidoError' imported but unused

Comment thread homeassistant/components/sensor/fido.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

're' imported but unused

Comment thread homeassistant/components/sensor/fido.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'json' imported but unused

Copy link
Copy Markdown
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Only small minor changes

Comment thread homeassistant/components/sensor/fido.py Outdated
self.fido_data = fido_data
self._state = None

self.update()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove that and use add_devices(sensors, True)

Copy link
Copy Markdown
Contributor Author

@titilambert titilambert Feb 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where can I find the definition of this function add_devices ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

helpers/entity_component.py

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks !

Comment thread homeassistant/components/sensor/fido.py Outdated
try:
self.client.fetch_data()
except PyFidoError as exp:
_LOGGER.error(exp)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use _LOGGER.exception and add a message to it.

Comment thread homeassistant/components/sensor/fido.py Outdated
"""Initialize the sensor."""
self.client_name = name
self.type = sensor_type
self.entity_id = "sensor.{}_{}".format(name, sensor_type)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need that. remove it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just want to keep the same string between the sensor entity_id and the monitored_variables list

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will be done. But if you setup same thing twice you disrate the core. So the core will produce your entity_id and make sure that is real unique

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oki ! So I changed the name of some sensor to get something more consistent

Comment thread homeassistant/components/sensor/fido.py Outdated

def setup_platform(hass, config, add_devices, discovery_info=None):
"""Set up the Fido sensor."""
# Create a data fetcher to support all of the configured sensors. Then make
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleanup this comments. It should be clear.

Copy link
Copy Markdown
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small cleanups and ready to go 👍

Comment thread homeassistant/components/sensor/fido.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what is that static?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just bad all stuff ...

Comment thread homeassistant/components/sensor/fido.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is that used?

@titilambert
Copy link
Copy Markdown
Contributor Author

@pvizeli just missing the documentation, I will push it later today

@titilambert titilambert changed the title Add Fido sensor [WIP] Add Fido sensor Feb 15, 2017
@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Feb 15, 2017

Merge after test pass. You can push your doc and refere to this. I can merge it without docu.

@pvizeli pvizeli merged commit 5895f43 into home-assistant:dev Feb 15, 2017
@RiRomain
Copy link
Copy Markdown
Contributor

It seems the docu was not merged or not there yet (thus the WIP?):
https://home-assistant.io/components/sensor.fido/

@balloob
Copy link
Copy Markdown
Member

balloob commented Feb 26, 2017

😢

Added a doc stub.

@titilambert
Copy link
Copy Markdown
Contributor Author

titilambert commented Feb 27, 2017

wtf ?! I'm doing it right now !

@titilambert
Copy link
Copy Markdown
Contributor Author

@anthonydiiorio
Copy link
Copy Markdown

Our account uses 1 number to login but has 3 numbers associated to the account. Would you be able to add a config option for the phone number to get the data from? Right now it gets the data from the primary number only. Thanks.

@titilambert
Copy link
Copy Markdown
Contributor Author

titilambert commented Mar 13, 2017

@tanilolli could you try the version 0.2.0 of pyfido: https://github.com/titilambert/pyfido
To confirm that is working with multiple phone number ?
If this is working, I will update the HA component.

Thanks

@anthonydiiorio
Copy link
Copy Markdown

@titilambert It works! I tested with all 3 numbers in the account and got the data I was expecting. Thank you very much!

@titilambert
Copy link
Copy Markdown
Contributor Author

Nice ! I hope make the PR this week !

@titilambert titilambert mentioned this pull request Mar 13, 2017
6 tasks
@home-assistant home-assistant locked and limited conversation to collaborators Jul 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants