Provide entity_id to avoid sensor mixup (fixes #7636). Use async_dispatcher. Provide icon. #7946

molobrakos · 2017-06-07T19:58:20Z

Description:

Do not mix up fuel sensor (litres) with fuel sensor (percent). Resolves Volvo on Call: Fuel and Fuel_2 seem to mix up every few days #7636
Use async dispatcher instead of storing function callback
Provide default icon (mdi:car)

Related issue (if applicable): fixes #
#7636

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>

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

Checklist:

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 have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

If the code does not interact with devices:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
Tests have been added to verify that the new code works.

pvizeli

This is not a good fix. You can also not use DOMAIN from component in a sensor namespace. Please change this and don't use the same name twice:
https://github.com/molobrakos/home-assistant/blob/3081853ee45aabf0ec3d9a160b54a46e6211198b/homeassistant/components/volvooncall.py#L35-L36

Other points that you should a bit cleanup on your code in a later PR 😅 :

https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/volvooncall.py#L71-L76 your are the first Person they do that in hass. Can you export this data class on root of your component, please.
https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/volvooncall.py#L169 we use format string normaly
https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/device_tracker/volvooncall.py#L33 It is not good designe to forward the callback into hass.data. Please use dispatcher from helper/dispatcher.
Don't use DOMAIN for hass.data, it overwrite the local DOMAIN constante and can make problem in future changes of core. Some people use a DATA_* constante for that

molobrakos · 2017-06-10T13:36:08Z

Yes, renaming the properties would also solve the bug. They are currently named (with units) "Fuel (L)" and "Fuel (%)". The reason both are exposed is that the latter depends on the tank volume. They could of course be renamed to "Fuel level (L)" and "Fuel ratio (%)". I considered that at first. But don't you think it is a shortcoming of the hass core to mixup sensors with same name? And don't they both measure a fuel level anyway? With the only difference being in what unit it is measured (litres or percentage). That was the reason I decided to keep the names of the sensors and just change the entity_id so they wont be mixed up by hass core. No big deal, I can just change the sensor names as you suggest, just wanted to clarify my reasoning.
Not sure what You can also not use DOMAIN from component in a sensor namespace. means. It is a constant defined in the same file. But I understand you want me to define DATA_* instead?
Sure, the state namespace/class could me moved from local to global scope. It would make it a clearer (but less concise). (actually I like that design pattern of easily creating a namespace to hold a state).
Re format string: Ok, I'll fix! There are still plenty of places that still uses the old style string formatting though:

> find -name "*.py" | xargs grep -E "('|\") %" | wc -l
150

re callbacks: I wasn't aware of that, I'll fix. (I assume you would like a SIGNAL_VEHICLE_SEEN to be defined?)
I am not sure what you mean by it overwrite the local DOMAIN constante and can make problem in future changes of core Why would it get overwritten? I should just create another constant with the same value? Also:

> find . -name "*.py" | xargs grep "hass.data\[DOMAIN\]" | wc -l
84

houndci-bot · 2017-06-10T22:16:40Z

homeassistant/components/volvooncall.py

@@ -92,7 +96,7 @@ def update_vehicle(vehicle):
            if isinstance(entity, Entity):
                entity.schedule_update_ha_state()
            else:
-                entity(vehicle)  # device tracker
+                async_dispatcher_send(SIGNAL_VEHICLE_SEEN, vehicle)  # device tracker


line too long (85 > 79 characters)

pvizeli

It was not the idea to do the cleanup with this PR, but good work. Only the point of DOMAIN is a bit missunderstand. I hope with my last Change request you understand it now. You can use DOMAIN inside your component for hass.data but you should not Import DOMAIN as DOMAIN in a platform they have a other domain like device_tracker or sensor.

pvizeli · 2017-06-14T12:49:47Z

homeassistant/components/volvooncall.py

@@ -123,6 +126,13 @@ def __init__(self, hass, vin, attribute):
        self._state.entities[self._vin].append(self)

    @property
+    def entity_id(self):


Move this into sensor/volvooncall entity and import the domain from sensor. Now you see why you should not import the DOMAIN as DOMAIN inside a platform from a other component then volvooncall.

Thank you for feedback - do I understand correctly that everything is fine if it is kept as it is, i.e. entity_id is specified in class VolvoEntity, which is defined in components/volvooncall?

Also, do you still prefer that I just rename the sensors instead of providing unique entity_id:s? (I still prefer the latter)

Yes. You can move the property into entity class inside sensor/volvooncall.py and use the DOMAIN from sensor/__init__.py or you remove the entity_id property and use a unique Name that generate a unique entity_id inside core. All other is perfect.

Please help me understand why you think it is wrong to keep it in the VolvoEntity base class (base class for both the implementation within sensor/ and binary_sensor/)?

houndci-bot · 2017-06-15T08:51:43Z

homeassistant/components/volvooncall.py

-             'fuel_amount': ('sensor', 'Fuel', 'mdi:gas-station', 'L'),
-             'fuel_amount_level': ('sensor', 'Fuel', 'mdi:water-percent', '%'),
+             'fuel_amount': ('sensor', 'Fuel amount', 'mdi:gas-station', 'L'),
+             'fuel_amount_level': ('sensor', 'Fuel level', 'mdi:water-percent', '%'),


line too long (85 > 79 characters)

houndci-bot · 2017-06-15T08:51:43Z

homeassistant/components/volvooncall.py

 from homeassistant.util.dt import utcnow
+from homeassistant.util import slugify


'homeassistant.util.slugify' imported but unused

…s some smaller fixes.

homeassistant added cla-signed integration: volvooncall labels Jun 7, 2017

balloobbot added the component label Jun 7, 2017

pvizeli requested changes Jun 8, 2017

View reviewed changes

molobrakos force-pushed the volvooncall-fix branch from 3081853 to 1c0a608 Compare June 10, 2017 21:25