search

mindmelting / hass-powerpal

Home Assistant custom integration to fetch data from Powerpal
MIT License
44 stars 11 forks source link

Move device_class and native_unit_of_measurement to the sensor class #17

Closed YodaDaCoda closed 1 year ago

YodaDaCoda commented 1 year ago

Move device_class and native_unit_of_measurement from the device class to the sensor class. Fixes issue with HomeAssistant complaining that the live consumption sensor has the wrong device_class. Also adjusted the native_unit_of_measurement to reduce calculations within the sensors - HomeAssistant can handle display of the units gracefully.

Closes #16

mindmelting commented 1 year ago

Thanks for the PR let me review and test

mindmelting commented 1 year ago

@YodaDaCoda Can you merge main branch into your PR - there was an issue with the test and validate pipelines which I fixed

YodaDaCoda commented 1 year ago

@mindmelting I've rebased onto main :)

mindmelting commented 1 year ago

@YodaDaCoda thanks! I have just tested and all seems to work fine - however I think we should keep the sensors to return kWh which is more common from a user perspective - changing it to Wh results in the Energy configuration requiring things like static price to be entered as AUD/Wh which is not as sensible as AUD/kWh which most energy companies will list (This also might end up being a breaking change if migrating, which I did not test)

Thoughts?

YodaDaCoda commented 1 year ago

Does HASS provide any guidelines on the preferred units a sensor should return? My thinking is that given HASS can convert units, a sensor should emit values in its native unit (as much as possible) and leave the conversion to HASS.

The value HASS wants in the energy config is related to the user-selectable "Unit of Measurement" config option on the sensor entity, which does default to Wh with this change...

image

mindmelting commented 1 year ago

Good question - ideally would love to return the native metric without translating to kWh - but the default to Wh is not a great experience. I would also need to test the impact of what changing this would do to existing installations...

Can I suggest moving the change to Wh into a seperate PR where we can debate that further as needed, and leave this PR to the other state class changes....

Thanks!

YodaDaCoda commented 1 year ago

Updated to restore kilo units. :)

When checking the consts in hass source to ensure I was putting the correct value for native_unit_of_measurement I found that DEVICE_CLASS_ENERGY and similar are deprecated so updated those to the new consts.