Skip to content

Add multi phone numbers support#6605

Merged
pvizeli merged 2 commits into
home-assistant:devfrom
titilambert:fido
Apr 5, 2017
Merged

Add multi phone numbers support#6605
pvizeli merged 2 commits into
home-assistant:devfrom
titilambert:fido

Conversation

@titilambert
Copy link
Copy Markdown
Contributor

@titilambert titilambert commented Mar 13, 2017

Description:

As @tanilolli requested (here #5997), this PR add the multi phone number support.

Please don't merge this PR. I'm waiting for @tanilolli confirmation for cleaning.

@tanilolli Could you confirm that this patch is working for you ?

Related issue (if applicable): fixes #5997

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

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: fido
    username: MYUSERNAME
    password: MYPASSWORD
    monitored_variables:
     - fido_dollar
     - balance
     - data_used

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.

@anthonydiiorio
Copy link
Copy Markdown

It works great with a single instance of the component. When I add multiple Fido sensors, initially some of the accounts give this error:
Error on receive last Fido data: Can not get token or uuid

After some time, all accounts do get updated though.

@titilambert
Copy link
Copy Markdown
Contributor Author

@tanilolli do you have this sometimes or only at startup ?

@anthonydiiorio
Copy link
Copy Markdown

@titilambert Only at startup

@titilambert
Copy link
Copy Markdown
Contributor Author

@tanilolli EVERYTIME your start HA ? (Is it reproducible ?)

@anthonydiiorio
Copy link
Copy Markdown

@titilambert Yes, everytime. It seems the issue is Fido doesn't like the requests being made so fast.

@titilambert
Copy link
Copy Markdown
Contributor Author

titilambert commented Mar 15, 2017

@tanilolli yes I guess,... So I don't know what to do for that :(
Do you have any idea ?

@titilambert
Copy link
Copy Markdown
Contributor Author

@balloob Can we share some data/object between to instance of the same component ?

@anthonydiiorio
Copy link
Copy Markdown

@titilambert Maybe the component could load all the data for the account and create it's own sensors.

sensor.fido could display general account details like balance
sensor.fido_NUMBER could display number specific details like data usage

@titilambert
Copy link
Copy Markdown
Contributor Author

@tanilolli good idea, I will try it tomorrow

@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Mar 30, 2017

Ping, any updates?

@titilambert
Copy link
Copy Markdown
Contributor Author

Sorry, my "tomorrow" is really long :/
But, I'm currently finishing it and waiting for more tests

@titilambert
Copy link
Copy Markdown
Contributor Author

Ready for review !
@tanilolli Could you retry with this new version ?

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.

line too long (88 > 79 characters)

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.

line too long (84 > 79 characters)

@anthonydiiorio
Copy link
Copy Markdown

Thanks for the update! It's working great with all 3 numbers in my account.

@titilambert titilambert changed the title [WIP] Add multi phone numbers support Add multi phone numbers support Apr 3, 2017
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 code style

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.

You can only use if self.fido_data.data.get(self.type): that replace this two lines.

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.

Same here

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.

continuation line with same indent as next logical line

@titilambert
Copy link
Copy Markdown
Contributor Author

Fixed !

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.

Leave a comment

@pvizeli pvizeli merged commit 118bd34 into home-assistant:dev Apr 5, 2017
@fabaff fabaff mentioned this pull request Apr 6, 2017
@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.

6 participants