Add services for bmw_connected_drive by ChristianKuehnel · Pull Request #13497 · home-assistant/core

ChristianKuehnel · 2018-03-27T20:30:43Z

Description:

Added services to bmw_connected_drive so that the Remote Services of the vehicle can also be triggered from Home Assistant.

Added vin to attributes of tracker, so that the users can see the VIN of the vehicle
I had to move the component to new package, so that I can add the services.yaml
Added service description in services.yaml

Related issue (if applicable): N/A

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

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

no changes

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

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

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

gerard33 · 2018-03-27T21:20:04Z

homeassistant/components/bmw_connected_drive/__init__.py

gerard33 · 2018-03-27T21:22:47Z

homeassistant/components/device_tracker/bmw_connected_drive.py

Is trackr_id used for something specific? As this field has the same value as id it seems a bit double.

the attributes are removed in #12999

MartinHjelmare · 2018-03-29T13:45:32Z

homeassistant/components/bmw_connected_drive/__init__.py

CONF_ constants are only used in config schemas. Here you want to use ATTR_ in the service schema.

MartinHjelmare · 2018-03-29T13:49:48Z

homeassistant/components/bmw_connected_drive/__init__.py

Do this directly in setup instead.

MartinHjelmare · 2018-03-29T13:50:55Z

homeassistant/components/bmw_connected_drive/__init__.py

Make function to handle service calls directly inside setup instead. You have access to this instance there, so this doesn't have to be a method here.

MartinHjelmare · 2018-03-29T13:51:29Z

homeassistant/components/bmw_connected_drive/__init__.py

Service handlers don't return anything.

MartinHjelmare · 2018-03-29T13:53:09Z

homeassistant/components/device_tracker/bmw_connected_drive.py

Remove this, it's duplicate info from host_name.

fixed, see also #12999

MartinHjelmare · 2018-03-29T13:54:45Z

Please also document the services in the docs.

pvizeli

Split this into a component with a device tracker and lock platform. So you can add the needed service to main component.

MartinHjelmare · 2018-03-29T19:58:25Z

Good point @pvizeli.

@ChristianKuehnel
This component already exposes some of these services in the lock platform eg. Why add them again in the main component?

ChristianKuehnel · 2018-03-30T15:20:13Z

@pvizeli @MartinHjelmare
My assumption was: offer all services that the underlying library offers also in a common API in hass. But I'm also fine with splitting in keeping the locking-related services with the lock and only offer the rest here. I implemented your proposal now.

Added documentation for the services (see link above).

houndci-bot · 2018-03-30T15:20:24Z

homeassistant/components/bmw_connected_drive/__init__.py

local variable 'result' is assigned to but never used

houndci-bot · 2018-03-30T15:20:24Z

homeassistant/components/bmw_connected_drive/__init__.py

'bimmer_connected.remote_services.ExecutionState' imported but unused

* added vin to attributes of tracker * moved component to new package * added service description

"Cell variable bimmer defined in loop (cell-var-from-loop)"

ChristianKuehnel · 2018-04-08T20:30:22Z

Build failed because of flaky test --> re-triggering build with open/close.

MartinHjelmare

Some minor comments left to attend. Otherwise looks good!

MartinHjelmare · 2018-04-08T21:29:25Z

homeassistant/components/bmw_connected_drive/__init__.py


-    for account in accounts:
-        account.update()
+    def _update_all(_) -> None:


Please add a parameter, service or call, even though you don't use it, it's clearer and according to our standard.

MartinHjelmare · 2018-04-08T21:30:20Z

homeassistant/components/bmw_connected_drive/__init__.py

-        account.update()
+    def _update_all(_) -> None:
+        """Update all BMW accounts."""
+        for account_update in hass.data[DOMAIN]:


Below you call the item bimmer. Why not call it the same here? It would read nicer.

ACK calling the object cd_account (Connected Drive Account).

MartinHjelmare · 2018-04-08T21:31:23Z

homeassistant/components/bmw_connected_drive/__init__.py

+        object here.
+        """
+        vin = call.data[ATTR_VIN]
+        _LOGGER.debug('Triggering service %s of vehicle %s',


This is already logged in the core at info level.

MartinHjelmare · 2018-04-08T21:32:36Z

homeassistant/components/bmw_connected_drive/__init__.py

+
+    # register the remote services
+    for service in _SERVICE_MAP:
+        _LOGGER.debug('Registering service %s', service)


We usually don't log this.

MartinHjelmare

Great! Can be merged when build passes.

homeassistant added integration: bmw_connected_drive integration: device_tracker core labels Mar 27, 2018

ChristianKuehnel mentioned this pull request Mar 27, 2018

Implement services in Home Assistant bimmerconnected/bimmer_connected#59

Closed

homeassistant added the cla-signed label Mar 27, 2018

gerard33 reviewed Mar 27, 2018

View reviewed changes

MartinHjelmare requested changes Mar 29, 2018

View reviewed changes

pvizeli requested changes Mar 29, 2018

View reviewed changes

homeassistant added the cla-signed label Mar 30, 2018

houndci-bot reviewed Mar 30, 2018

View reviewed changes

ChristianKuehnel mentioned this pull request Mar 30, 2018

bmw_connected_drive: added documentation for new services home-assistant/home-assistant.io#5055

Merged

2 tasks

ChristianKuehnel force-pushed the bimmer_services branch from 695641d to 641c468 Compare March 30, 2018 19:41

homeassistant added the cla-signed label Mar 30, 2018

ChristianKuehnel added 9 commits April 1, 2018 13:07

implemented services for bmw remote services

b132132

* added vin to attributes of tracker * moved component to new package * added service description

fixed static analysis warnings

bc4e50a

implemented first set of code reviews

0848a56

removed locking related services

69a4ffa

fixed static analysis warnings

d7515e0

removed excess blank lines

ff2c86a

refactoring of setup() to resolve warning

8b0fddb

"Cell variable bimmer defined in loop (cell-var-from-loop)"

added missing docstring

d78401d

added service to update all vehicles from the server

e4d3b36

ChristianKuehnel force-pushed the bimmer_services branch from 641c468 to e4d3b36 Compare April 1, 2018 11:25

homeassistant added the cla-signed label Apr 1, 2018

m1n3rva mentioned this pull request Apr 6, 2018

Add binary sensors bimmerconnected/bimmer_connected#17

Closed

8 tasks

ChristianKuehnel closed this Apr 8, 2018

ChristianKuehnel reopened this Apr 8, 2018

MartinHjelmare reviewed Apr 8, 2018

View reviewed changes

ChristianKuehnel added 2 commits April 9, 2018 21:05

implemented changes requested in code review

560a7a4

added check if invalid vin is entered

b87c5f1

homeassistant added the cla-signed label Apr 9, 2018

MartinHjelmare approved these changes Apr 9, 2018

View reviewed changes

MartinHjelmare changed the title ~~bmw_connected_drive - added services~~ Add services for bmw_connected_drive Apr 17, 2018

MartinHjelmare merged commit e472436 into home-assistant:dev Apr 17, 2018

ChristianKuehnel deleted the bimmer_services branch April 19, 2018 15:20

gerard33 mentioned this pull request Apr 23, 2018

BMW 520d F11 2016 bimmerconnected/bimmer_connected#69

Closed

balloob mentioned this pull request Apr 27, 2018

0.68 #14124

Merged

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

Uh oh!

Conversation

ChristianKuehnel commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare commented Mar 29, 2018

Uh oh!

pvizeli left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChristianKuehnel commented Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristianKuehnel commented Apr 8, 2018

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Apr 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

ChristianKuehnel commented Mar 27, 2018 •

edited

Loading

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

MartinHjelmare Mar 29, 2018 •

edited

Loading

pvizeli left a comment •

edited

Loading

MartinHjelmare commented Mar 29, 2018 •

edited

Loading

ChristianKuehnel commented Mar 30, 2018 •

edited

Loading

MartinHjelmare Apr 8, 2018 •

edited

Loading