Fixes issue #40 and general improvements #58
Conversation
…ng over 10 seconds" in HA error log. Added an async delay function Added a writeasync with delay to slow down send of commands Modify set_mode to handle situation where IntesisBox returns out of order. Ensure that mode is set prior turning on unit.
There was a problem hiding this comment.
On the whole I like the direction you're going with the changes, I think it will make the code base better and less buggy.
Besides the specific comments I left, I'd love to suggest one further change - introducing a mutex for "ack-able commands". At a high level the idea would be something like:
class IntesisBox:
ackable_cmd = asyncio.Condition()
def data_received(...):
...
if cmd == "ACK":
asyncio.run(self.ackable_cmd.notify_all())
...
class IntesisBoxAC:
async def set_mode(self, ...):
async with self.controller.ackable_cmd:
await self.async_write(...)
with contextlib.suppress(asyncio.TimeoutError):
await asyncio.wait_for(self.controller.ackable_cmd.wait(), timeout=5)
This way we'd ring-fence the various flows that result in out of order responses/acks somewhat (maybe at the cost of speed for sending multiple commands) but that might simplify a lot of the rest of the code.
I should also add that I don't feel strongly about this, at best this is a suggestion, at worst we ignore it and move on.
| @@ -111,8 +134,9 @@ def data_received(self, data): | |||
| if cmd == "ID": | |||
| self._parse_id_received(args) | |||
| self._connectionStatus = API_AUTHENTICATED | |||
| _ = asyncio.ensure_future(self.keep_alive()) | |||
| # _ = asyncio.ensure_future(self.keep_alive()) | |||
There was a problem hiding this comment.
Please remove the old commented code (here and in a few other places)
There was a problem hiding this comment.
Noted. Shall I delete and re-do it?
| f"Waiting for MODE to return {mode}, currently {str(self.mode)}" | ||
| ) | ||
| _LOGGER.debug(f"Retry attempt = {retry}") | ||
| # asyncio.run(self.delay(1)) # SHANE |
There was a problem hiding this comment.
With this commented out I think we'll spam the controller while we wait for it to update right?
There was a problem hiding this comment.
I don't think so because we send the GET,1:MODE with a delay of 1 second in _writeasync.
|
Hi @jbergler thank you for the comments. I should share, if its not obvious from my code, that I am not a developer and had to learn to use Github and PyCharm to do this! First time with anything version control other than comments and backup files - hence the mess. I don't know the details of the mutex but get the basic concept of what you are proposing. With my experience with the 4 Intesisbox's I have had (I only have 3 now), I don't see there being issues with slowdowns or multiple commands and feel that the additional robustness will be a benefit the integration. The upside of my pull request is that it has you pro's looking at it, so thank you! Cheers, |
|
@jbergler Not sure of the next step here. My system with 3 of IB's has been working reliably after implementing these changes. It now turns on the AC units, only after confirming that the correct mode is set. |
.idea/hass-intesisbox.iml
Outdated
There was a problem hiding this comment.
It looks like you added quite a few files to the PR unintentionally. Could you push another commit that removes these again please?
There was a problem hiding this comment.
Goodness, that was fun, my bad. Removed and added to .gitignore.
Addresses issue #40 by polling AMBTEMP more frequently to stop "is taking over 10 seconds" messages in HA log.
No longer return target_temperature if Mode is FAN or OFF.
Changes order from Power On then set Mode to Set Mode, wait until mode Mode is per requested then Power On. Was causing issues with hass-template-climate.
Some Intesisbox and heat pump combos respond slowly to mode changes and CHN messages can be returned out of order.
Example:
Ran black and the pre-commit hooks.