Add Aprilaire integration #95093
Add Aprilaire integration #95093
Conversation
home-assistant
bot
added
cla-needed
has-tests
new-integration
integration: aprilaire
labels
|
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! |
home-assistant
bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
chamberlain2007
force-pushed
the
aprilaire
branch
from
09767ea
to
c86bdb9
Compare
chamberlain2007
force-pushed
the
aprilaire
branch
from
c86bdb9
to
eff024d
Compare
chamberlain2007
force-pushed
the
aprilaire
branch
from
9d18e11
to
d2a6be0
Compare
| _LOGGER.exception("Unexpected exception") | ||
| errors["base"] = str(err) | ||
| else: | ||
| coordinator = AprilaireCoordinator( |
There was a problem hiding this comment.
I still need to see what the coordinator does, but I don't prefer to call the coordinator in the config flow
There was a problem hiding this comment.
@joostlek It's probably not ideal, but the complication comes from the wait_for_ready method. Due to the asynchronous nature of the connection and it being pushed based, connecting requires a multi-step process, which is what that method does. Basically, we need to wait for the data to be in the coordinator, regardless of if that is because we sent a request for it or the thermostat sent it on its own. I was thinking if it would be possible to shift the wait_for_ready method to the client (which is in the pyaprilaire package), but there is a dependence on the coordinator's state to make it work.
Considering that, I think there are two options:
- Leave it as-is, favoring properly testing the connection at the expense of referencing the coordinator in the config flow
- Make the config flow only check that the socket is accessible at the host/port combination, and not check that anything else about the connection is successful.
I think the user experience of the first option is better, avoiding adding a thermostat that doesn't actually connect, which does happen. But I welcome your thoughts.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
| """Stop listening for data.""" | ||
| self.client.stop_listen() | ||
|
|
||
| async def wait_for_ready( |
There was a problem hiding this comment.
I am no expert on this, but maybe you can create a task for everything you wait for and use it with asyncio.gather. But I am not sure if that is something good to use here. But just putting it out here
There was a problem hiding this comment.
@joostlek It is possible to, but the complication here is that the connection doesn't work in a request/response manner like you might expect. Packets are sent to the thermostat with no acknowledgement or response, and at some point in the future the thermostat sends an altogether separate packet with the data. Additionally, the thermostat is very sensitive to receiving too many packets at once, which can cause it to ignore some, send errors, or in a worst-case scenario lock up entirely. So, sending these serially here is in my opinion the best approach, though it definitely is not the cleanest from a code perspective.
There was a problem hiding this comment.
This looks good now @chamberlain2007 👍
Just a few minor comments, then this is good to go.
Please also consider this: #95093 (comment)
|
Looks like all the requested changes have been incorporated/resolved? Anything remaining blocking a merge? |
|
@emontnemery @edenhaus @IsakNyberg any chance you could take a look at this and provide next steps for @chamberlain2007 ? |
|
@nberardi Please don't ping others asking for a review. Instead, could you help review other PRs so the total number goes down? |
|
@chamberlain2007 can you look into the failing ruff check please? |
Co-authored-by: Jon Oberheide <506986+jonoberheide@users.noreply.github.com>
|
Apologies for the 11th hour request. I noticed that the ‘humidifier’ entity isn’t being used for the Humidifier or Dehumidifier integrations. And the ‘weather’ entity isn’t being used for the outside temp and humidity sensors. The Ecobee integration is a great example of both integrations. It would be great if these were added in for the initial release, or at the very least a fast follow to reduce as many migrations as possible farther down the line. @chamberlain2007 Other integrations such as Aux/Emergency heat can be added in later without worrying about migration of entities later. |
The Home Assistant team prefers starting an integration with a minimal set of functionality, hence this only containing the climate entity. If you use the other entities, I would recommend staying with the unofficial version until everything is implemented here. |
Proposed change
Add a new integration to support Aprilaire smart thermostats.
This has been requested/discussed in the community forums: https://community.home-assistant.io/t/aprilaire-thermostat-8800-any-modbus-experts
This integration has been available on HACS for some time and has been tested by some community members: https://github.com/chamberlain2007/aprilaire-ha
This initial pull request adds the climate entity with core functionality. I expect additional releases to include the rest of the functionality currently available in the custom repo.
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.To help with the load of incoming pull requests: