Improvements for WeMo Insight switches by deftdawg · Pull Request #6001 · home-assistant/core

deftdawg · 2017-02-14T23:31:56Z

Description:

Changes current power units to watts
Adds power on times and additional totals

Related issue (if applicable): fixes #

Partially addresses messed up labels and adds missing Insight data fields Wemo insight (+ perhaps maker) state attributes don't update #3506
Recommend patch (Troubleshooting insightparams subscriptions for Home Assistant pywemo/pywemo#64) to resolve lack of updates for WeMo Insight devices

* Changes current power units to watts * Adds power on times and additional totals

mention-bot · 2017-02-14T23:31:57Z

@jumpkick, thanks for your PR! By analyzing the history of the files in this pull request, we identified @pavoni, @stu-gott and @balloob to be potential reviewers.

homeassistant · 2017-02-14T23:31:58Z

Hi @jumpkick,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

houndci-bot · 2017-02-14T23:32:01Z

+            try:
+               return self.insight_params['todaymw']
+            except:
+               return None


indentation is not a multiple of four

houndci-bot · 2017-02-14T23:32:01Z

        if self.insight_params:
-            return self.insight_params['todaymw']
+            try:
+               return self.insight_params['todaymw']


indentation is not a multiple of four

houndci-bot · 2017-02-14T23:32:02Z

+            try:
+               return self.insight_params['totalmw']
+            except:
+               return None


indentation is not a multiple of four

houndci-bot · 2017-02-14T23:32:02Z

        if self.insight_params:
-            return self.insight_params['currentpower']
+            try:
+               return self.insight_params['totalmw']


indentation is not a multiple of four

houndci-bot · 2017-02-14T23:32:02Z

+      d = datetime(1,1,1) + timedelta(seconds=_seconds)
+      return "{:0>2d}d {:0>2d}h {:0>2d}m {:0>2d}s".format(d.day-1,
+                                                     d.hour,
+                                                     d.minute, d.second)


continuation line under-indented for visual indent

houndci-bot · 2017-02-14T23:32:02Z

+           return self.insight_params['powerthreshold'] / 1000
+
+    def _as_uptime(self, _seconds):
+      d = datetime(1,1,1) + timedelta(seconds=_seconds)


indentation is not a multiple of four
missing whitespace after ','

houndci-bot · 2017-02-14T23:32:02Z

+    @property
+    def power_threshold(self):
+        if self.insight_params:
+           return self.insight_params['powerthreshold'] / 1000


indentation is not a multiple of four

houndci-bot · 2017-02-14T23:32:02Z

+            try:
+               return self._current_power_mw() / 1000
+            except:
+               return None


indentation is not a multiple of four

houndci-bot · 2017-02-14T23:32:02Z

+        """Current power usage in W."""
+        if self.insight_params:
+            try:
+               return self._current_power_mw() / 1000


indentation is not a multiple of four

houndci-bot · 2017-02-14T23:32:03Z

+    def _current_power_mw(self):
+        """Current power usage in mW."""
+        if self.insight_params:
+           return self.insight_params['currentpower']


indentation is not a multiple of four

* Reordered datetime import * Spaces by 4

* continuation line under-indented for visual indent

houndci-bot · 2017-02-15T20:38:51Z

+                                                          d.hour,
+                                                          d.minute, 
+                                                          d.second)
+


blank line contains whitespace

houndci-bot · 2017-02-15T20:38:51Z

+      d = datetime(1,1,1) + timedelta(seconds=_seconds)
+      return "{:0>2d}d {:0>2d}h {:0>2d}m {:0>2d}s".format(d.day-1,
+                                                          d.hour,
+                                                          d.minute, 


trailing whitespace

trailing whitespace... (argh... the bot should just trim it)

houndci-bot · 2017-02-15T21:47:08Z

+    def on_total(self):
+        """On time in seconds."""
+        if self.insight_params:
+            return as_uptime(self.insight_params['ontotal'])


undefined name 'as_uptime'

houndci-bot · 2017-02-15T21:47:08Z

+    def on_today(self):
+        """On time in seconds."""
+        if self.insight_params:
+            return as_uptime(self.insight_params['ontoday'])


undefined name 'as_uptime'

houndci-bot · 2017-02-15T21:47:08Z

+    def on_for(self):
+        """On time in seconds."""
+        if self.insight_params:
+            return as_uptime(self.insight_params['onfor'])


undefined name 'as_uptime'

houndci-bot · 2017-02-15T21:47:08Z

+
+    @staticmethod
+    def as_uptime(_seconds):
+        """Format seconds in to uptime string in the format: 00d 00h 00m 00s """


line too long (80 > 79 characters)

balloob · 2017-02-16T06:42:54Z

+    def _current_power_mw(self):
+        """Current power usage in mW."""
+        if self.insight_params:
+            try:


Instead of try … catch, use self.insight_param.get('current_power')

guess I could do if self.insight_params and self.insight_param.get('current_power'):, guessing the original code if self.insight_params condition because non-Insight switches don't have those attribs.

Exactly right.

balloob · 2017-02-16T06:43:00Z


        return attr

+#    @property


Stale comment ?

Yeah, not needed, was from originally when that method was currrent_power_mwh

balloob · 2017-02-16T06:43:25Z

+            except TypeError:
+                return None
+
+    @property


Why are somethings properties and other methods ? Also, why are they properties/method to begin with and not just all part of device_state_attributes ?

Started out cutting and pasting what was there (I'm not a python guy, nor do I know HA components)... If we don't need the methods, I can just load everything into the state attribs assignments.

balloob · 2017-02-16T06:44:34Z

    @property
-    def current_power_mwh(self):
-        """Current power usage in mWh."""
+    def power_total_mw_min(self):


You can't just rename overrides of official properties. Please check out the switch ABC and see what is supported and what is not.

The Insight returns average milliwatt power consumed for each minute. So for 12W it will increment this value by 12,000 each minute. So it's mW/min.

mWh is average milliwatts per hour... The Insight doesn't provide that.

At some point I hope to refactor some of the HA power/energy code - but this requires changing some of the base components properties - and any other components that override then.

Until then I think you need to leave the official properties as they are - and adddevice_state_properties for the correct / new attributes.

pavoni · 2017-02-18T17:28:49Z

I've released your pywemo changes as 0.4.13 ready for when you have time to wrap up this PR.

lwis · 2017-02-20T13:08:41Z


+# Wemo Insight
+ATTR_POWER_CURRENT_W = 'power_current_w'
+# ATTR_POWER_AVG_W = 'power_average_w'


Is this required?

pavoni · 2017-02-22T10:37:39Z

@jumpkick Let me know if you need some help on this.

My suggestion would be to leave the overridden switch attributes (current_power_mwh and today_power_mw) as they are for this PR.

Some of the issues you've found with these properties should be reworked as a broader PR to change all these attributes across all devices (#3828)).

The other attributes you want to include can just be added as device_state_attributes.

deftdawg · 2017-02-23T08:15:45Z

@pavoni yeah, I'm thinking of starting over on this patch... leave the stuff that's there and just load up device_state_attributes w/ the additional attributes. I agree with #3828, but not sure how to incorporate it.

This gets rid of the other stuff and just updates device_state_attributes, leaving the default properties alone.

pavoni · 2017-02-23T10:28:36Z


        if self.insight_params or (self.coffeemaker_mode is not None):
            attr[ATTR_CURRENT_STATE_DETAIL] = self.detail_state
+            attr['current_power_w'] = \


Don't expect the coffee machine will support these new attributes - so I'd change this so that you only retrieve these attributes for the insight device.

pavoni · 2017-02-23T10:30:13Z

+            attr['current_power_w'] = \
+                self.insight_params['currentpower'] / 1000
+            attr['today_power_mW_min'] = self.insight_params['todaymw']
+            attr['total_power_mW_min'] = self.insight_params['totalmw']


This line is duplicated - but it's also a duplicate of the existing current_power_mwh property of the switch - so you can just remove these - and depend on the switch attributes.

Will remove in favour of the incorrectly labeled existing ones

pavoni · 2017-02-23T10:33:42Z


        if self.insight_params or (self.coffeemaker_mode is not None):
            attr[ATTR_CURRENT_STATE_DETAIL] = self.detail_state
+            attr['current_power_w'] = \


This is just the existing switch attribute divided by 1000.

At some point we'll change the switch attribute to be W rather than mW anyway. So this is only really helpful until we do that.

My suggestion would be to remove it

- Separate attribs from coffeemaker condition - Set power units for threshold to mW to be consistent with others - Adjust on-time labels to be more clear

pavoni · 2017-02-24T11:13:20Z

👍 Looks good to me.

@balloob are you happy for me to merge?

lwis · 2017-02-24T11:40:30Z

            attr[ATTR_CURRENT_STATE_DETAIL] = self.detail_state

+        if self.insight_params:
+            attr['on_latest_time'] = \


Can we move these to ATTR_* constants as above?

We could put them in ATTR_ constants, but they are probably all going to be renamed and content replaced as part of addressing #3828... more places to change, so should I do it anyway?

The only one you'd need to rename is the threshold in mw.

Perhaps switch it to `W’ and break out the attributes in constants. Then nothing will need to be changed if my PR gets merged.

balloob · 2017-03-02T07:35:29Z

+    def as_uptime(_seconds):
+        """Format seconds into uptime string in the format: 00d 00h 00m 00s."""
+        uptime = datetime(1, 1, 1) + timedelta(seconds=_seconds)
+        return "{:0>2d}d {:0>2d}h {:0>2d}m {:0>2d}s".format(uptime.day-1,


Why not use the timedelta object directly? It supports days, hours etc https://docs.python.org/3/library/datetime.html#datetime.timedelta

I'm probably doing something stupid (python noob), but when I try that I get this:

>>> from datetime import datetime, timedelta >>> uptime = timedelta(days=0,seconds=86401,hours=0,minutes=0) >>> print ("{:0>2d}d {:0>2d}h {:0>2d}m {:0>2d}s".format(uptime.days, uptime.hours, uptime.minutes, uptime.seconds)) Traceback (most recent call last): File "<stdin>", line 1, in <module> AttributeError: 'datetime.timedelta' object has no attribute 'hours'

Oh you're right, I was wrong. Looks like timedelta converts it all to seconds.

robbiet480 · 2017-03-10T05:14:29Z

Thanks! 🐬 🍪 💯

Improvements for WeMo Insight switches

29c7987

* Changes current power units to watts * Adds power on times and additional totals

homeassistant added platform: switch.wemo cla-needed labels Feb 14, 2017

houndci-bot reviewed Feb 14, 2017

View reviewed changes

homeassistant added cla-signed and removed cla-needed labels Feb 14, 2017

deftdawg added 2 commits February 15, 2017 15:34

Update wemo.py

c404fb7

* Reordered datetime import * Spaces by 4

Update wemo.py

44d274e

* continuation line under-indented for visual indent

houndci-bot reviewed Feb 15, 2017

View reviewed changes

deftdawg added 2 commits February 15, 2017 15:40

Update wemo.py

a718e92

trailing whitespace... (argh... the bot should just trim it)

Back to you travis....

b163544

houndci-bot reviewed Feb 15, 2017

View reviewed changes

deftdawg added 3 commits February 15, 2017 16:57

Update wemo.py

e221c8a

Update wemo.py

e9cf5f6

Update wemo.py

f6e46ae

balloob reviewed Feb 16, 2017

View reviewed changes

lwis reviewed Feb 20, 2017

View reviewed changes

pavoni mentioned this pull request Feb 22, 2017

Bump pywemo version. Improves notifications for insight devices. #6159

Merged

8 tasks

Update device_state_attributes only

ef87d4d

This gets rid of the other stuff and just updates device_state_attributes, leaving the default properties alone.

homeassistant added the cla-signed label Feb 23, 2017

pavoni reviewed Feb 23, 2017

View reviewed changes

Incorporate comment suggestions

fc5e25a

- Separate attribs from coffeemaker condition - Set power units for threshold to mW to be consistent with others - Adjust on-time labels to be more clear

lwis reviewed Feb 24, 2017

View reviewed changes

balloob reviewed Mar 2, 2017

View reviewed changes

robbiet480 merged commit 11c4d38 into home-assistant:dev Mar 10, 2017

deftdawg deleted the patch-2 branch March 11, 2017 06:27

home-assistant locked and limited conversation to collaborators Jul 17, 2017

ghost added integration: wemo and removed platform: switch.wemo labels Mar 21, 2019

Uh oh!

Conversation

deftdawg commented Feb 14, 2017

Uh oh!

mention-bot commented Feb 14, 2017

Uh oh!

homeassistant commented Feb 14, 2017

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!

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!

pavoni commented Feb 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavoni commented Feb 22, 2017

Uh oh!

deftdawg commented Feb 23, 2017

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!

pavoni commented Feb 24, 2017

Uh oh!