Repository navigation
Only query emeter on update if feature is supported - #120
mfalzone wants to merge 3 commits into
Conversation
Sorry, something went wrong.
Codecov Report
@@ Coverage Diff @@
## master #120 +/- ##
==========================================
+ Coverage 73.02% 73.22% +0.19%
==========================================
Files 10 10
Lines 1227 1236 +9
Branches 183 184 +1
==========================================
+ Hits 896 905 +9
Misses 298 298
Partials 33 33
Continue to review full report at Codecov.
|
Sorry, something went wrong.
| update_response = {} | ||
| update_response.update( | ||
| await self.protocol.query( | ||
| self.host, self._create_request("system", "get_sysinfo") | ||
| ) | ||
| ) | ||
|
|
||
| if SmartDevice._check_emeter_feature_presence(update_response): | ||
| update_response.update( | ||
| await self.protocol.query(self.host, self._create_emeter_request()) | ||
| ) |
There was a problem hiding this comment.
This would create unnecessary I/O requests on devices that support the emeter. The logic should be:
- If _sysinfo is None (i.e., the device hasn't ever been updated and we don't know if it supports emeter), do fetch only the sysinfo. Check if the _has_emeter is now True, and request the emeter information if so.
- Otherwise (on an already initialized device), if _has_emeter is True, the emeter request should be included in the query like it is at the moment.
This way only the initial update will cause two separate I/O requests.
Sorry, something went wrong.
There was a problem hiding this comment.
So, I had a chance to play around a little more this weekend and it turns out that the fix is actually much more simple than I had originally thought.
On my HS220 device, I was able to determine that:
- The emeter check returns as expected when it's the only query (which is what led me to the original implementation in this PR)
- the emeter check returns as expected when combined with other non-system queries (i.e. time, schedule, etc...)
and finally...
3) The emeter check returns as expected when combined with the system query as long as the emeter query comes first
Since the handful of other devices that I have (HS210, HS200, KP400) were all working with the original code, I inspected the software version to figure out what the difference is. The HS220's look to be running an older version of the firmware than all the other devices, so I suspect there's probably a bug in older versions of the firmware (1.0.5 seems to be the cutoff) when querying for both emeter and system in the same request.
Sorry, something went wrong.
There was a problem hiding this comment.
Ah, thanks for the comprehensive analysis! I just missed this when doing a review on the currently changed code pieces.
A question though, what is the expected response for the emeter when it's the only query? My assumption is that HS220 does not support emeter at all => there is no need to do any tricks to add emeter queries for devices without emeter (as reported by the sysinfo)?
Sorry, something went wrong.
|
As an additional data point here, HS100s running the new 1.1.0 firmware will crash if asked for emeter information - so, for those at least, we definitely need to ask for sysinfo first. |
Sorry, something went wrong.
|
Thanks @SimonWilkinson, that's good to know. I've updated to remove the emeter query if it's not a feature of the device. |
Sorry, something went wrong.
| # get_sysinfo. | ||
| if self._last_update is None: | ||
| initial_update_req = self._update_query(include_emeter=False) | ||
| await self._update_with_request(initial_update_req) |
There was a problem hiding this comment.
Save the response somewhere temporarily, that way there is no need to do the second query in the case if there's no emeter found.
Having two requests when the emeter is found is fine (as second one should've been made anyway).
Otherwise this looks good to me, but please add a unit test to check the update() to make sure no future refactoring will break this behavior!
Sorry, something went wrong.
Sorry, something went wrong.
Starts to address #105 by splitting the get_sysinfo and emeter queries into two distinct API calls. As noted today by @dcmorton on #119, the underlying cause of the current request hanging is the combination of multiple queries in the same request. Making a second, standalone emeter request should always work, but since we can determine whether an emeter is present by checking the response from get_sysinfo, I figured let's just avoid the second call if it's not needed.
This PR is only designed to address points 2 and 3 of this comment from @rytilahti but I'd be interested in tackling the first point as time allows unless someone else gets there first.
Fixes #161