Skip to content

Add sensors for BMW electric cars #14293

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.

Sign up for GitHub

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

Merged
MartinHjelmare merged 4 commits into from
May 8, 2018
Merged

Add sensors for BMW electric cars #14293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c52d96e
Add sensors for electric cars
gerard33 May 4, 2018
5fb7f09
Updates based on review of @MartinHjelmare
gerard33 May 6, 2018
051ed03
Fix Travis error
gerard33 May 6, 2018
e47ff14
Another fix for Travis
gerard33 May 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 70 additions & 5 deletions homeassistant/components/binary_sensor/bmw_connected_drive.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,19 @@
SENSOR_TYPES = {
'lids': ['Doors', 'opening'],
'windows': ['Windows', 'opening'],
'door_lock_state': ['Door lock state', 'safety']
'door_lock_state': ['Door lock state', 'safety'],
'lights_parking': ['Parking lights', 'light'],
'condition_based_services': ['Condition based services', 'problem'],
'check_control_messages': ['Control messages', 'problem']
}

SENSOR_TYPES_ELEC = {
'charging_status': ['Charging status', 'power'],
'connection_status': ['Connection status', 'plug']
}

SENSOR_TYPES_ELEC.update(SENSOR_TYPES)


def setup_platform(hass, config, add_devices, discovery_info=None):
"""Set up the BMW sensors."""
Expand All @@ -29,10 +39,18 @@ def setup_platform(hass, config, add_devices, discovery_info=None):
devices = []
for account in accounts:
for vehicle in account.account.vehicles:
for key, value in sorted(SENSOR_TYPES.items()):
device = BMWConnectedDriveSensor(account, vehicle, key,
value[0], value[1])
devices.append(device)
if vehicle.has_hv_battery:
Copy link
Copy Markdown

@m1n3rva m1n3rva May 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@gerard33 gerard33 May 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

_LOGGER.debug('BMW with a high voltage battery')
for key, value in sorted(SENSOR_TYPES_ELEC.items()):
device = BMWConnectedDriveSensor(account, vehicle, key,
value[0], value[1])
devices.append(device)
elif vehicle.has_internal_combustion_engine:
_LOGGER.debug('BMW with an internal combustion engine')
for key, value in sorted(SENSOR_TYPES.items()):
device = BMWConnectedDriveSensor(account, vehicle, key,
value[0], value[1])
devices.append(device)
add_devices(devices, True)


Expand Down Expand Up @@ -92,12 +110,41 @@ def device_state_attributes(self):
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
Copy link
Copy Markdown

@m1n3rva m1n3rva May 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

@gerard33 gerard33 May 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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:
Copy link
Copy Markdown

@m1n3rva m1n3rva May 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@gerard33 gerard33 May 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@m1n3rva m1n3rva May 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Contributor Author

@gerard33 gerard33 May 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

service_type = report.service_type.lower().replace('_', ' ')
result['{} status'.format(service_type)] = report.state.value
if report.due_date is not None:
result['{} date'.format(service_type)] = \
report.due_date.strftime('%Y-%m-%d')
if report.due_distance is not None:
result['{} distance'.format(service_type)] = \
'{} km'.format(report.due_distance)
elif self._attribute == 'check_control_messages':
check_control_messages = vehicle_state.check_control_messages
if not check_control_messages:
result['check_control_messages'] = 'OK'
else:
result['check_control_messages'] = check_control_messages
elif self._attribute == 'charging_status':
result['charging_status'] = vehicle_state.charging_status.value
# pylint: disable=W0212
result['last_charging_end_result'] = \
vehicle_state._attributes['lastChargingEndResult']
if self._attribute == 'connection_status':
# pylint: disable=W0212
result['connection_status'] = \
vehicle_state._attributes['connectionStatus']

return result

def update(self):
"""Read new state data from the library."""
from bimmer_connected.state import LockState
from bimmer_connected.state import ChargingState
vehicle_state = self._vehicle.state

# device class opening: On means open, Off means closed
Expand All @@ -111,6 +158,24 @@ def update(self):
# 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
Copy link
Copy Markdown

@m1n3rva m1n3rva May 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@gerard33 gerard33 May 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my first comment.

if self._attribute == 'lights_parking':
self._state = vehicle_state.are_parking_lights_on
# device class problem: On means problem detected, Off means no problem
if self._attribute == 'condition_based_services':
self._state = not vehicle_state.are_all_cbs_ok
if self._attribute == 'check_control_messages':
self._state = vehicle_state.has_check_control_messages
# device class power: On means power detected, Off means no power
if self._attribute == 'charging_status':
self._state = vehicle_state.charging_status in \
[ChargingState.CHARGING]
# device class plug: On means device is plugged in,
# Off means device is unplugged
if self._attribute == 'connection_status':
# pylint: disable=W0212
self._state = (vehicle_state._attributes['connectionStatus'] ==
'CONNECTED')

def update_callback(self):
"""Schedule a state update."""
Expand Down
2 changes: 1 addition & 1 deletion homeassistant/components/bmw_connected_drive/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
from homeassistant.helpers.event import track_utc_time_change
import homeassistant.helpers.config_validation as cv

REQUIREMENTS = ['bimmer_connected==0.5.0']
REQUIREMENTS = ['bimmer_connected==0.5.1']

_LOGGER = logging.getLogger(__name__)

Expand Down
69 changes: 42 additions & 27 deletions homeassistant/components/sensor/bmw_connected_drive.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,22 +9,12 @@

from homeassistant.components.bmw_connected_drive import DOMAIN as BMW_DOMAIN
from homeassistant.helpers.entity import Entity
from homeassistant.helpers.icon import icon_for_battery_level

DEPENDENCIES = ['bmw_connected_drive']

_LOGGER = logging.getLogger(__name__)

LENGTH_ATTRIBUTES = {
'remaining_range_fuel': ['Range (fuel)', 'mdi:ruler'],
'mileage': ['Mileage', 'mdi:speedometer']
}

VALID_ATTRIBUTES = {
'remaining_fuel': ['Remaining Fuel', 'mdi:gas-station']
}

VALID_ATTRIBUTES.update(LENGTH_ATTRIBUTES)


def setup_platform(hass, config, add_devices, discovery_info=None):
"""Set up the BMW sensors."""
Expand All @@ -34,27 +24,26 @@ def setup_platform(hass, config, add_devices, discovery_info=None):
devices = []
for account in accounts:
for vehicle in account.account.vehicles:
for key, value in sorted(VALID_ATTRIBUTES.items()):
device = BMWConnectedDriveSensor(account, vehicle, key,
value[0], value[1])
for attribute_name in vehicle.drive_train_attributes:
device = BMWConnectedDriveSensor(account, vehicle,
attribute_name)
devices.append(device)
device = BMWConnectedDriveSensor(account, vehicle, 'mileage')
devices.append(device)
add_devices(devices, True)


class BMWConnectedDriveSensor(Entity):
"""Representation of a BMW vehicle sensor."""

def __init__(self, account, vehicle, attribute: str, sensor_name, icon):
def __init__(self, account, vehicle, attribute: str):
"""Constructor."""
self._vehicle = vehicle
self._account = account
self._attribute = attribute
self._state = None
self._unit_of_measurement = None
self._name = '{} {}'.format(self._vehicle.name, self._attribute)
self._unique_id = '{}-{}'.format(self._vehicle.vin, self._attribute)
self._sensor_name = sensor_name
self._icon = icon

@property
def should_poll(self) -> bool:
Expand All @@ -74,7 +63,27 @@ def name(self) -> str:
@property
def icon(self):
"""Icon to use in the frontend, if any."""
return self._icon
from bimmer_connected.state import ChargingState
vehicle_state = self._vehicle.state
charging_state = vehicle_state.charging_status in \
[ChargingState.CHARGING]

if self._attribute == 'mileage':
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare May 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return 'mdi:speedometer'
elif self._attribute in (
'remaining_range_total', 'remaining_range_electric',
'remaining_range_fuel', 'max_range_electric'):
return 'mdi:ruler'
elif self._attribute == 'remaining_fuel':
return 'mdi:gas-station'
elif self._attribute == 'charging_time_remaining':
return 'mdi:update'
elif self._attribute == 'charging_status':
return 'mdi:battery-charging'
elif self._attribute == 'charging_level_hv':
return icon_for_battery_level(
battery_level=vehicle_state.charging_level_hv,
charging=charging_state)

@property
def state(self):
Expand All @@ -88,7 +97,17 @@ def state(self):
@property
def unit_of_measurement(self) -> str:
"""Get the unit of measurement."""
return self._unit_of_measurement
if self._attribute in (
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare May 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use a dict here too.

'mileage', 'remaining_range_total', 'remaining_range_electric',
'remaining_range_fuel', 'max_range_electric'):
return 'km'
elif self._attribute == 'remaining_fuel':
return 'l'
elif self._attribute == 'charging_time_remaining':
return 'h'
elif self._attribute == 'charging_level_hv':
return '%'
return None

@property
def device_state_attributes(self):
Expand All @@ -101,14 +120,10 @@ def update(self) -> None:
"""Read new state data from the library."""
_LOGGER.debug('Updating %s', self._vehicle.name)
vehicle_state = self._vehicle.state
self._state = getattr(vehicle_state, self._attribute)

if self._attribute in LENGTH_ATTRIBUTES:
self._unit_of_measurement = 'km'
elif self._attribute == 'remaining_fuel':
self._unit_of_measurement = 'l'
if self._attribute == 'charging_status':
self._state = getattr(vehicle_state, self._attribute).value
else:
self._unit_of_measurement = None
self._state = getattr(vehicle_state, self._attribute)

def update_callback(self):
"""Schedule a state update."""
Expand Down
2 changes: 1 addition & 1 deletion requirements_all.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ beautifulsoup4==4.6.0
bellows==0.5.2

# homeassistant.components.bmw_connected_drive
bimmer_connected==0.5.0
bimmer_connected==0.5.1

# homeassistant.components.blink
blinkpy==0.6.0
Expand Down