Add extra sensors for BMW ConnectedDrive by gerard33 · Pull Request #12591 · home-assistant/core

gerard33 · 2018-02-21T23:42:52Z

Description:

The following items are added to BMW ConnectedDrive:

Binary sensors for Doors, Windows and Door lock state
Lock

For the existing sensors remaining_range_fuel, mileage and remaining_fuel the attribute car: <car_model> and an applicable mdi icon are added.

~~Breaking change~~ reverted in #13380

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

Checklist:

The code change is tested and works locally.

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

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

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

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
New dependencies are only imported inside functions that use them (example).

gerard33 · 2018-02-21T23:44:41Z

@ChristianKuehnel I have added the binary sensor, lock and made some small changes to the existing sensor. Can you please review these?

MartinHjelmare · 2018-02-22T20:14:00Z

homeassistant/components/binary_sensor/bmw_connected_drive.py

Binary sensors don't have a unit of measurement.

Removed it.

MartinHjelmare · 2018-02-22T20:14:36Z

homeassistant/components/binary_sensor/bmw_connected_drive.py

Please sort 🔤 within groups standard library, 3rd party and homeassistant.

MartinHjelmare · 2018-02-22T20:15:58Z

homeassistant/components/binary_sensor/bmw_connected_drive.py

Time stamp is not allowed in state attributes. The state should not update just because of a time change.

This shows the last update time which is received from the car via the bmw_connected_drive module.
This time stamp is only updated together with other updates received from the car, e.g. an update on the mileage, see the screenshot below.

It's the same usage as in the Mercedes ME component.

@gerard33
I get your point, I agree it's quite useful to see when the last measurement was taken. But isn't the information redundant? You would also get the age of the information from the headline "3 hours ago".

Yes, this is redundant, please remove it.

As a matter of fact it is not redudant. The last update contains the time on which the update from the car is received, but that doesn't always mean that every sensor is being updated. So this can result in a different time shown at the time information in the headline than at the last update time. See the example with the below screenshots.
With this update the door lock state has changed, but the windows state hasn't, resulting in different times for the windows sensor in the headline versus the last update time.

I think it has added value to show this attribute as you can easily see if the sensor has changed or not as part of the last update received from the car.

If a state attribute changes, the last_updated attribute of the state will be updated with the time, so this information is already available in the the state machine. If a user wants to know about this we offer the template sensor which can display state attributes including last_updated. We shouldn't duplicate this information in the state machine.

MartinHjelmare · 2018-02-22T20:17:24Z

homeassistant/components/binary_sensor/bmw_connected_drive.py

Is lid name lowercase snakecase? That's the home assistant standard.

It is, this is an example of a lid name: door_driver_front.

MartinHjelmare · 2018-02-22T20:18:40Z

homeassistant/components/binary_sensor/bmw_connected_drive.py

This is iterating windows not lids.

Changed it to window :)

MartinHjelmare · 2018-02-22T20:31:58Z

homeassistant/components/lock/bmw_connected_drive.py

Since we register a callback for state updates, I expect that the callback is called when the state has changed on the device, (car lock). Isn't that true? If that is true, the lock method should only call the device lock method.

Locking/unlocking the car doesn't trigger a state update.

We're polling the update every 5 minutes. So if we do not change the state here, the UI will show an old state for 5 minutes.

And if we do poll right away, you still get the old state. You have to wait about 10 seconds until you get the correct state from the server.

I think a middle way is to do an optimistic state update immediately in home assistant, then have the library call the update callback after 10 seconds with the correct state when we are sure that that is present.

MartinHjelmare · 2018-02-22T20:33:12Z

homeassistant/components/lock/bmw_connected_drive.py

MartinHjelmare · 2018-02-22T20:33:23Z

homeassistant/components/lock/bmw_connected_drive.py

Locking/unlocking the car doesn't trigger a state update.

MartinHjelmare · 2018-02-22T20:33:40Z

homeassistant/components/lock/bmw_connected_drive.py

Debug at most.

Changed to debug

MartinHjelmare · 2018-02-22T20:35:55Z

homeassistant/components/sensor/bmw_connected_drive.py

Please remove.

See my comment above an this last update time.

MartinHjelmare

It would be best if the interface library made sure the update callback got called upon new state after the lock is locked/unlocked.

MartinHjelmare · 2018-02-23T07:09:24Z

homeassistant/components/lock/bmw_connected_drive.py

This should probably be handled in the interface library.

I have removed this check completly in here and will check with @ChristianKuehnel to handle this in the interface library and make that part of a next release.

MartinHjelmare · 2018-02-23T07:09:54Z

homeassistant/components/lock/bmw_connected_drive.py

Sleeping is not allowed in the executor thread pool.

MartinHjelmare · 2018-02-23T07:11:51Z

homeassistant/components/lock/bmw_connected_drive.py

Either the update callback should handle setting state or we should do an optimistic state update from here, immediately. The logic below doesn't belong in this method.

Removed, see above

MartinHjelmare · 2018-02-23T07:12:53Z

homeassistant/components/lock/bmw_connected_drive.py

Don't use async methods from a sync context.

ChristianKuehnel · 2018-02-23T21:34:49Z

homeassistant/components/binary_sensor/bmw_connected_drive.py

Does the name of the sensor also contain the name of the vehicle?
If someone has more than one vehicle, we need to make sure that the sensors then get different names...

The vehicle name is not show in the sensor name, but it is shown in the attributes. Not sure what is the best approach, but maybe those people with more than one vehicle can update the friendly name of the sensor? Because in my opinion it's not necessary to show the vehicle name in the sensor name if you have one BMW (which will be the majority I guess).

Should we add an option where people can give names to their vehicles? Something like:

bmw_connected_drive: my_account: username: USERNAME_BMW_CONNECTED_DRIVE password: PASSWORD_BMW_CONNECTED_DRIVE country: COUNTRY_BMW_CONNECTED_DRIVE vehicles: - vin: SOMEVIN name: his - vin: SOMEOTHERVIN name: hers

That way people can

select which cars should be listed in HASS at all

set custom names for their vehicles

we have different names for the different vehicles?

That is indeed something we could add as an option.

MartinHjelmare · 2018-02-24T13:03:47Z

homeassistant/components/lock/bmw_connected_drive.py

Please add a comment before this, saying why we need an optimistic state update, when we also have an update callback.

Added the comments

gerard33 · 2018-02-25T20:33:29Z

Closing and opening the PR to trigger another Travis CI check.

MartinHjelmare · 2018-02-25T20:41:08Z

You changed access permissions for the requirements_all.txt file. Please revert that. That file should only be modified via the gen_requirements_all.py script.

Please also revert the access permission change on the component module.

gerard33 · 2018-02-27T21:52:02Z

Another close/open for Travis CI.

ChristianKuehnel · 2018-03-03T08:05:20Z

homeassistant/components/bmw_connected_drive.py

Please update to version 0.4.1. I fixed some exceptions there and it's currently already in use on master

ChristianKuehnel · 2018-03-03T08:08:36Z

@gerard33 Looks good to me.

As I updated to bimmer_connected version 0.4.1 on master you probably need to rebase your branch and re-run ./scripcs/gen-requirements.

Sorry for that, but I wanted to get the bugfix out quickly...

MartinHjelmare

Please update the state update logic in all platforms. It will be more efficient and more on line with the rest of home assistant.

Also please remove the time stamp from all state attributes. It's redundant.

With that done this should be good to go.

MartinHjelmare · 2018-03-13T06:31:42Z

homeassistant/components/binary_sensor/bmw_connected_drive.py

Yes, this is redundant, please remove it.

MartinHjelmare · 2018-03-13T06:43:23Z

homeassistant/components/binary_sensor/bmw_connected_drive.py

+            self._state = bool(vehicle_state.door_lock_state.value
+                               in ('SELECTIVELOCKED', 'UNLOCKED'))
+
+        self.schedule_update_ha_state()


It would be more efficient if you move this to a separate new method which calls this with argument True which will make sure update is called before the state is updated.

Then register this new method as state update callback in async_added_to_hass.

def update_callback(self): """Schedule a state update.""" self.schedule_update_ha_state(True)

Updated this, can you please doublecheck if it's okay?

MartinHjelmare · 2018-03-13T06:44:50Z

homeassistant/components/binary_sensor/bmw_connected_drive.py

+
+        Show latest data after startup.
+        """
+        self._account.add_update_listener(self.update)


self._account.add_update_listener( self.update_callback)

MartinHjelmare · 2018-03-13T06:46:45Z

homeassistant/components/binary_sensor/bmw_connected_drive.py

+        Show latest data after startup.
+        """
+        self._account.add_update_listener(self.update)
+        yield from self.hass.async_add_job(self.update)


Remove this and instead add an argument True to add_devices.

MartinHjelmare · 2018-03-13T06:48:08Z

homeassistant/components/binary_sensor/bmw_connected_drive.py

+                device = BMWConnectedDriveSensor(account, vehicle, key,
+                                                 value[0], value[1])
+                devices.append(device)
+    add_devices(devices)


add_devices(devices, True)

This will make sure update is called before adding the device.

MartinHjelmare · 2018-03-13T06:49:17Z

homeassistant/components/lock/bmw_connected_drive.py

+        for vehicle in account.account.vehicles:
+            device = BMWLock(account, vehicle, 'lock', 'BMW lock')
+            devices.append(device)
+    add_devices(devices)


MartinHjelmare · 2018-03-13T06:49:30Z

homeassistant/components/lock/bmw_connected_drive.py

+        self._state = (STATE_LOCKED if vehicle_state.door_lock_state.value
+                       in ('LOCKED', 'SECURED') else STATE_UNLOCKED)
+
+        self.schedule_update_ha_state()


MartinHjelmare · 2018-03-13T06:49:43Z

homeassistant/components/lock/bmw_connected_drive.py

+
+        Show latest data after startup.
+        """
+        self._account.add_update_listener(self.update)


MartinHjelmare · 2018-03-13T06:49:51Z

homeassistant/components/lock/bmw_connected_drive.py

+        Show latest data after startup.
+        """
+        self._account.add_update_listener(self.update)
+        yield from self.hass.async_add_job(self.update)


MartinHjelmare · 2018-03-13T07:00:41Z

homeassistant/components/sensor/bmw_connected_drive.py

+
    def update(self) -> None:
        """Read new state data from the library."""
        _LOGGER.debug('Updating %s', self.entity_id)


Replace this entity_id with name as in the other platforms.

Also please update the state update logic below as in the other platforms.

MartinHjelmare

Looks great!

MartinHjelmare · 2018-03-15T07:02:01Z

Please add a paragraph and highlight the breaking changes in the description, so it's easy to copy paste that to the release notes.

gerard33 · 2018-03-15T20:08:54Z

@MartinHjelmare thanks!
I have added a paragraph in the description highlighting the breaking changes to the existing sensors.

MartinHjelmare · 2018-03-15T21:53:52Z

Great!

gerard33 · 2018-03-20T22:24:33Z

@MartinHjelmare
cc @ChristianKuehnel

I just realized that the breaking change for the names of the sensors can be avoided.
I can leave the entity name of the sensor untouched and just add the new (more descriptive) name as friendly_name attribute to the sensors, so that name will be shown in the frontend together with the new mdi icons.

What do you think? If you agree I can make a PR (including the new binary sensors and lock) asap so this can be part of the HA 0.66 release.
Just let me know.

MartinHjelmare · 2018-03-20T22:32:09Z

That won't work. We're not allowed to set friendly_name. What you can do is to set the entity_id. But I suggest you don't do that. Instead it would be better if we can define unique ids for each entity. Then the user can change entity_id if they want. Can we use the vin number?

gerard33 · 2018-03-20T22:50:02Z

Ah, I see.
Yes the VIN is available both in the current API as in the new API #13305.
I suggest adding the VIN to the entity_id in the new PR then?
And for now change the sensor name back to the currently used name to avoid having 2 breaking changes for this?

MartinHjelmare · 2018-03-20T22:52:38Z

We should define unique_id property and use the vin number plus sensor type when applicable. Don't change entity_id.

gerard33 · 2018-03-20T23:02:17Z

Ok. And shall I adapt this PR so there is no breaking change in the name of the current 3 sensors?

Then the unique_id can be implemented in the WIP PR.

MartinHjelmare · 2018-03-21T01:00:42Z

If the breaking name change can be avoided, that sounds good.

Try to only have one new feature per PR.

homeassistant added integration: bmw_connected_drive new-platform platform: sensor.bmw_connected_drive cla-signed labels Feb 21, 2018

MartinHjelmare requested changes Feb 22, 2018

View reviewed changes

homeassistant added the cla-signed label Feb 22, 2018

gerard33 mentioned this pull request Feb 22, 2018

Documentation for extra sensors BMW Connected Drive home-assistant/home-assistant.io#4733

Merged

2 tasks

MartinHjelmare reviewed Feb 23, 2018

View reviewed changes

homeassistant added the cla-signed label Feb 23, 2018

ChristianKuehnel mentioned this pull request Feb 23, 2018

Changed API for remote services bimmerconnected/bimmer_connected#29

Merged

ChristianKuehnel reviewed Feb 23, 2018

View reviewed changes

homeassistant added the cla-signed label Feb 24, 2018

MartinHjelmare reviewed Feb 24, 2018

View reviewed changes

gerard33 requested a review from andrey-git as a code owner February 24, 2018 13:42

homeassistant added the cla-signed label Feb 24, 2018

gerard33 closed this Feb 25, 2018

gerard33 reopened this Feb 25, 2018

homeassistant added the cla-signed label Feb 25, 2018

gerard33 closed this Feb 27, 2018

gerard33 reopened this Feb 27, 2018

ChristianKuehnel reviewed Mar 3, 2018

View reviewed changes

gerard33 added 2 commits March 3, 2018 19:25

Added extra sensors for BMW ConnectedDrive

a4887f0

Updates based on review of @MartinHjelmare

7117933

gerard33 mentioned this pull request Mar 6, 2018

Features for electric/hybrid cars bimmerconnected/bimmer_connected#41

Closed

ChristianKuehnel mentioned this pull request Mar 9, 2018

BMW device tracker - fix toggling state if vehicle tracking is disabled #12999

Merged

3 tasks

MartinHjelmare requested changes Mar 13, 2018

View reviewed changes

Changes based on review by @MartinHjelmare

57b13d7

homeassistant added the cla-signed label Mar 14, 2018

MartinHjelmare approved these changes Mar 15, 2018

View reviewed changes

MartinHjelmare added the breaking-change label Mar 15, 2018

MartinHjelmare changed the title ~~Added extra sensors for BMW ConnectedDrive~~ Add extra sensors for BMW ConnectedDrive Mar 15, 2018

MartinHjelmare merged commit d13bcf8 into home-assistant:dev Mar 15, 2018

This was referenced Mar 18, 2018

Add lock bimmerconnected/bimmer_connected#19

Closed

Add binary sensors bimmerconnected/bimmer_connected#17

Closed

gerard33 deleted the bmw-connecteddrive branch March 21, 2018 22:27

gerard33 mentioned this pull request Mar 21, 2018

Avoid breaking change for BMW ConnectedDrive sensors in #12591 #13380

Merged

2 tasks

MartinHjelmare pushed a commit that referenced this pull request Mar 23, 2018

Avoid breaking change for BMW ConnectedDrive sensors in #12591 (#13380)

c50b002

MartinHjelmare removed the breaking-change label Mar 23, 2018

balloob mentioned this pull request Mar 30, 2018

0.66.0 #13554

Merged

gerard33 mentioned this pull request Apr 14, 2018

Add unique_id for BMW ConnectedDrive #13888

Merged

2 tasks

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

ghost removed platform: binary_sensor.bmw_connected_drive labels Mar 21, 2019

Uh oh!

Conversation

gerard33 commented Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Checklist:

Uh oh!

gerard33 commented Feb 21, 2018

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!

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!

ChristianKuehnel Feb 23, 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!

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!

Choose a reason for hiding this comment

Uh oh!

gerard33 Feb 23, 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!

gerard33 commented Feb 21, 2018 •

edited

Loading

ChristianKuehnel Feb 23, 2018 •

edited

Loading

gerard33 Feb 23, 2018 •

edited

Loading

gerard33 Feb 23, 2018 •

edited

Loading

MartinHjelmare Feb 24, 2018 •

edited

Loading

MartinHjelmare commented Feb 25, 2018 •

edited

Loading