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

StarLine integration #27197

Merged
merged 73 commits into from Nov 26, 2019
Merged

StarLine integration #27197

merged 73 commits into from Nov 26, 2019

Conversation

@Anonym-tsk
Copy link
Contributor

Anonym-tsk commented Oct 4, 2019

Description:

The starline integration lets you retrieve data of your StarLine security system from the StarLine portal. You will need a working StarLine account.

This integration provides the following platforms:

  • Binary Sensors: Hand brake, hood, trunk, alarm status and doors lock state.
  • Device tracker: The location of your car.
  • Lock: Control the lock of your car.
  • Sensors: Battery level, SIM card balance, GSM signal level, interior temperature and engine temperature.
  • Switches: Start/stop engine, heater (webasto) and additional channel.
  • Services: Update the state.

This is one complex integration (car platform); it cannot be divided.

Related issue (if applicable): fixes #

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#10662

Example entry for configuration.yaml (if applicable):

(integration and configuration from UI)

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

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

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

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
Anonym-tsk added 13 commits Oct 1, 2019
WIP
WIP
@Anonym-tsk Anonym-tsk requested a review from home-assistant/core as a code owner Oct 4, 2019
@project-bot project-bot bot added this to Needs review in Dev Oct 4, 2019
Anonym-tsk added 5 commits Oct 4, 2019
CI
CI
Copy link
Member

MartinHjelmare left a comment

Please extract the device specific interface code and start with just the component and one platform in this PR. Further platforms can come in future PRs one platform per PR.

homeassistant/components/starline/api.py Outdated Show resolved Hide resolved
Dev automation moved this from Needs review to Review in progress Oct 4, 2019
Anonym-tsk added 4 commits Oct 7, 2019
@Anonym-tsk

This comment has been minimized.

Copy link
Contributor Author

Anonym-tsk commented Nov 7, 2019

@Anonym-tsk remove the options flow for it. Keep the service around.

@MartinHjelmare @balloob how can i find out current update interval without options flow?
Please, keep this. It's really useful.

@Anonym-tsk Anonym-tsk closed this Nov 7, 2019
Dev automation moved this from Review in progress to Cancelled Nov 7, 2019
@Anonym-tsk Anonym-tsk reopened this Nov 7, 2019
Dev automation moved this from Cancelled to Incoming Nov 7, 2019
@Anonym-tsk Anonym-tsk requested a review from MartinHjelmare Nov 7, 2019
@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Nov 7, 2019

how can i find out current update interval without options flow?

I assume you mean after a restart, right? Ie that we need to persist the update interval on file. We could use our storage helper.

@Anonym-tsk

This comment has been minimized.

Copy link
Contributor Author

Anonym-tsk commented Nov 7, 2019

I assume you mean after a restart, right? Ie that we need to persist the update interval on file. We could use our storage helper.

No. If i set update interval with service and forgot this value (why not), I can’t see the set value anywhere

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Nov 7, 2019

Yes that's true. Usually services are called via automations and these hold the values.

@Anonym-tsk

This comment has been minimized.

Copy link
Contributor Author

Anonym-tsk commented Nov 7, 2019

Yes that's true. Usually services are called via automations and these hold the values.

I think, options flow is more user-friendly in this case.
If you want to change update interval permanently, use UI.
If you want dynamic changes, use service.
Maybe we can keep options flow?

CI
@StevenRudenko

This comment has been minimized.

Copy link

StevenRudenko commented Nov 7, 2019

@MartinHjelmare @balloob as for me the setting to have an interval for scanning is the same as it is for HERE Travel Time service: https://www.home-assistant.io/integrations/here_travel_time/#scan_interval

it looks like that was added for the same purposes as @Anonym-tsk suggests, - to reduce API requests number and prevent API limit be reached. any thoughts?

@Anonym-tsk

This comment has been minimized.

Copy link
Contributor Author

Anonym-tsk commented Nov 7, 2019

any thoughts?

There is one important moment.
Some users use two instances of HA for fault tolerance. These users need to set permanent update interval twice higher than default. And for these users i made options flow.
Other users want to set update interval by automations, e.g. day/night. They can use service.
And since we don't use configuration.yaml, we can’t see the currently set interval anywhere without options flow.

Anonym-tsk added 2 commits Nov 7, 2019
@MartinHjelmare MartinHjelmare moved this from Incoming to Review in progress in Dev Nov 7, 2019
@Anonym-tsk

This comment has been minimized.

Copy link
Contributor Author

Anonym-tsk commented Nov 8, 2019

@MartinHjelmare @balloob Please explain me last two questions about icons and options flow.

@StevenRudenko

This comment has been minimized.

Copy link

StevenRudenko commented Nov 15, 2019

@MartinHjelmare @balloob @Anonym-tsk is there any option to have a quick chat between three of you in any messenger you use for this? This may help to perform all necessary changes and discuss open questions.

@frenck

This comment has been minimized.

Copy link
Member

frenck commented Nov 16, 2019

Guys, please stop tagging everybody constantly. Everybody is busy already.
If you need help or want to discuss things, we have a Discord server with public development channels.

@StevenRudenko

This comment has been minimized.

Copy link

StevenRudenko commented Nov 16, 2019

thanks @frenck. that's what needed

Anonym-tsk added 3 commits Nov 18, 2019
@Anonym-tsk

This comment has been minimized.

Copy link
Contributor Author

Anonym-tsk commented Nov 18, 2019

So, i removed options_flow and icons. Please check this PR again.
@balloob @MartinHjelmare @pvizeli

Anonym-tsk added 2 commits Nov 18, 2019
@balloob balloob merged commit a37260f into home-assistant:dev Nov 26, 2019
11 checks passed
11 checks passed
CI Build #20191118.25 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch 100% of diff hit (target 94.4%)
Details
codecov/project 94.44% (target 90%)
Details
Dev automation moved this from Review in progress to Done Nov 26, 2019
@lock lock bot locked and limited conversation to collaborators Nov 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
You can’t perform that action at this time.