Add sensors for BMW electric cars by gerard33 · Pull Request #14293 · home-assistant/core

gerard33 · 2018-05-04T22:36:58Z

Description:

Add new sensors for BMW vehicles.
CC @ChristianKuehnel @m1n3rva

General:

Binary sensor for parking_lights
Binary sensor for condition_based_services
Binary sensor for check_control_messages

For electric and hybrid cars:

Binary sensor for charging_status
Binary sensor for connection_status
Sensor for charging_time_remaining
Sensor for charging_status
Sensor for max_range_electric
Sensor for charging_level_hv
Sensor for remaining_range_electric

Breaking change
The current sensor remaining_range_fuel is replaced by remaining_range_total for cars with a combustion engine.

Checklist:

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

rafale77 · 2018-05-05T19:03:39Z

Just tested this and it is not recognizing my car as electrical (it is an i3)

gerard33 · 2018-05-05T19:29:43Z

@rafale77 can you share a screenshot with all the cars entities from the States page?

rafale77 · 2018-05-05T19:38:36Z

I was looking for it and actually there is none of the additional sensors. Not sure if I pulled this wrong. I am in US. Not sure if the server has an impact.

gerard33 · 2018-05-05T19:47:00Z

Server should not have impact, so probably something with the pull.
You can try the custom component which is available here https://github.com/gerard33/home-assistant/tree/bmw-dev/bmw_connecteddrive.
That is tested with an i3 in US, see this post https://community.home-assistant.io/t/bmw-connecteddrive-component/39569/249?u=gerard33.

rafale77 · 2018-05-05T21:17:27Z

Sorry for the wrong message, I oddly had two version of Hass installed and updating no longer updates my virtual env one which is strange. It is all working.

MartinHjelmare · 2018-05-05T21:37:18Z

+        # device class plug: On means device is plugged in,
+        #                    Off means device is unplugged
+        if self._attribute == 'connection_status':
+            self._state = bool(vehicle_state._attributes['connectionStatus'] ==


You don't need to use bool here.

Removed bool

MartinHjelmare · 2018-05-05T21:41:10Z

+        elif self._attribute == 'charging_level_hv':
+            return '%'
+        else:
+            self._unit_of_measurement = None


Return None.

Changed it to return None

MartinHjelmare · 2018-05-05T21:41:29Z

@@ -53,8 +45,6 @@ def __init__(self, account, vehicle, attribute: str, sensor_name, icon):
        self._unit_of_measurement = None


This isn't used anymore.

Removed the line

m1n3rva

Hey @gerard33,

great job integrating all of the different vehicle types into the sensor!

I added some comments on the design of the sensor and the readability of the code. Nothing major, rather some hints to improve it.

m1n3rva · 2018-05-06T15:11:04Z

                result[window.name] = window.state.value
        elif self._attribute == 'door_lock_state':
            result['door_lock_state'] = vehicle_state.door_lock_state.value
+            result['last_update_reason'] = vehicle_state.last_update_reason


This function is not getting a huge if/elseif statement. This makes it a bit hard to read and maintain. Should we split up this monster class into one class per sensor type and use inheritance?

That sounds like a plan. I would suggest to discuss this seperately, so we can first get this PR finished. Then we can implement the other parts on our list, such as sending the position, the imperial vs metric units, the lscType and the stuff you mentioned here regarding the design and readability of the code.

m1n3rva · 2018-05-06T15:13:31Z

-                device = BMWConnectedDriveSensor(account, vehicle, key,
-                                                 value[0], value[1])
-                devices.append(device)
+            if vehicle.has_hv_battery:


You could also use vehicle.drive_train_attributes here. This will already give you a list of available attributes. This way you're flexible if we add more attributes to bimmer_connected.

Then you would not need SENSOR_TYPES_ELEC and SENSOR_TYPES

The vehicle.drive_train_attributes is being used in the sensor code.
It contains a lot of attributes, but not the ones which are used for the binary_sensors (only charging_status is used in both).
Maybe it's a good idea to add this to bimmer_connected for the binary sensors as well in a next release.

m1n3rva · 2018-05-06T15:19:18Z

+        elif self._attribute == 'lights_parking':
+            result['lights_parking'] = vehicle_state.parking_lights.value
+        elif self._attribute == 'condition_based_services':
+            for report in vehicle_state.condition_based_services:


You could extract the formatting of the CBS data into a separate function:

@staticmethod def _format_cbs_report(report): result = dict() service_type = report.service_type.lower().replace('_', ' ') ... return result

This would make the function more readable.

Or maybe also something that can be done in bimmer_connected?
Otherwise I can indeed make a separate function for this in a next version.

@gerard33
I just had a look at the code and this is quite specific for the HA use case, where you need a dictionary as result. I would rather not implement this in bimmer connected.

@m1n3rva the function is implemented in the component in #14424.

m1n3rva · 2018-05-06T15:21:54Z

            # Possible values: LOCKED, SECURED, SELECTIVE_LOCKED, UNLOCKED
            self._state = vehicle_state.door_lock_state not in \
                          [LockState.LOCKED, LockState.SECURED]
+        # device class light: On means light detected, Off means no light


Similar: huge if statement for the different types of sensors.

See my first comment.

MartinHjelmare · 2018-05-07T07:14:55Z

+        elif self._attribute == 'charging_status':
+            result['charging_status'] = vehicle_state.charging_status.value
+            result['last_charging_end_result'] = \
+                vehicle_state._attributes['lastChargingEndResult']


Travis is failing on three of these accesses to private attributes. You can disable that check above those lines. But you should probably change this in the library so that we don't have to access private attributes.

Thanks! Have disabled those checks for now. It's on the issue list to add these to the library
bimmerconnected/bimmer_connected#73.

MartinHjelmare · 2018-05-08T07:46:27Z

+        charging_state = vehicle_state.charging_status in \
+            [ChargingState.CHARGING]
+
+        if self._attribute == 'mileage':


You could make a constant dict that maps the attributes to the icon and lookup that here, if attribute is not charging_level_hv.

MartinHjelmare · 2018-05-08T07:47:13Z

    def unit_of_measurement(self) -> str:
        """Get the unit of measurement."""
-        return self._unit_of_measurement
+        if self._attribute in (


You could use a dict here too.

MartinHjelmare · 2018-05-08T07:51:25Z

I'm going to merge this now. A new PR that cleans up some of the if...elif trails would be great.

Add sensors for electric cars

c52d96e

homeassistant added cla-signed integration: bmw_connected_drive platform: binary_sensor.bmw_connected_drive labels May 4, 2018

gerard33 mentioned this pull request May 4, 2018

BMW 5 Series 2014 F10 bimmerconnected/bimmer_connected#62

Closed

gerard33 changed the title ~~Add sensors for electric cars~~ Add sensors for BMW electric cars May 5, 2018

Danielhiversen added the breaking-change label May 5, 2018

rafale77 approved these changes May 5, 2018

View reviewed changes

MartinHjelmare reviewed May 5, 2018

View reviewed changes

m1n3rva approved these changes May 6, 2018

View reviewed changes

Updates based on review of @MartinHjelmare

5fb7f09

homeassistant added the cla-signed label May 6, 2018

Fix Travis error

051ed03

This was referenced May 6, 2018

Add some extra attributes as output in VehicleClass bimmerconnected/bimmer_connected#28

Closed

Add binary sensors bimmerconnected/bimmer_connected#17

Closed

Add extra sensor options bimmerconnected/bimmer_connected#16

Closed

MartinHjelmare reviewed May 7, 2018

View reviewed changes

Another fix for Travis

e47ff14

MartinHjelmare reviewed May 8, 2018

View reviewed changes

MartinHjelmare merged commit ba7333e into home-assistant:dev May 8, 2018

gerard33 deleted the bmw-electric branch May 8, 2018 20:04

gerard33 mentioned this pull request May 12, 2018

BMW code cleanup #14424

Merged

2 tasks

balloob mentioned this pull request May 28, 2018

0.70.0 #14657

Merged

home-assistant locked and limited conversation to collaborators Sep 5, 2018

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

		@@ -53,8 +45,6 @@ def __init__(self, account, vehicle, attribute: str, sensor_name, icon):
		self._unit_of_measurement = None

Uh oh!

Conversation

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

Uh oh!

Description:

Checklist:

Uh oh!

rafale77 commented May 5, 2018

Uh oh!

gerard33 commented May 5, 2018

Uh oh!

rafale77 commented May 5, 2018

Uh oh!

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

Uh oh!

Uh oh!

rafale77 commented May 5, 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!

m1n3rva 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!

Choose a reason for hiding this comment

Uh oh!

gerard33 May 6, 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!

gerard33 May 7, 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!

MartinHjelmare commented May 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gerard33 commented May 4, 2018 •

edited

Loading

gerard33 commented May 5, 2018 •

edited

Loading

gerard33 May 6, 2018 •

edited

Loading

gerard33 May 7, 2018 •

edited

Loading