Add Philips air purifier integration #68714
Conversation
|
Have you tried to contact the original libs developer to either take of maintainance of his lib or be added as a developer on his lib? |
No, I hadn't tried to contact the original developer before. There's an MR on the original repository, where different people have mentioned the author over a longer period of time, with no response. Nevertheless, I've now sent an email to the author, if nothing else, just to let them know what's happening. Thanks for the suggestion! |
| """Return the unique ID for this purifier.""" | ||
| if self._status is None: | ||
| return self._initial_data["unique_id"] | ||
| return self._status.device_id |
There was a problem hiding this comment.
Can this be moved to constructor? Same as name? Just set the _attr_name and unique id there directly.
There was a problem hiding this comment.
I've moved the unique_id to the constructor.
I'd like to keep the name as method, given that the device's name can change over time. This allows showing the current name at least after receiving a status update from the device.
| self._client.stop_observing_status(id(self)) | ||
|
|
||
| def _set_status(self, status: Status | None) -> None: | ||
| self._status = status |
There was a problem hiding this comment.
What does this Status contain? Why just not set _attr_available (or @property available()) when the device cannot be accessed? This should also block the other ones from being retrieved.
There was a problem hiding this comment.
I've removed the if checks. If I don't do any checks, mypy complains about _status being optional, so I've added assertions to make mypy happy. I'm not sure this is the best solution, though.
| return info | ||
|
|
||
| @property | ||
| def is_on(self): |
There was a problem hiding this comment.
Would be nice to try to type your integration fully :). If you want, you can even mark it as strict and fix all mypy errors.
There was a problem hiding this comment.
Done 🎉
Fully typed the package phipsair in strict mode as well.
| if self._status is None: | ||
| return None |
There was a problem hiding this comment.
I am not a huge fan of this self._status check everywhere. Can you see if this is required when you use available (see comment above)?
There was a problem hiding this comment.
You're right, Home Assistant doesn't seem to call the other methods if the device is unavailable. I've removed the checks everywhere except for the name, where the fallback to the value from the config entry makes sense, I think.
To make typing work, I added assertions, though (self._status is not None). Is this the way to go?
|
Thanks for the feedback! I'll have a look and improve things, hopefully in the next few days. |
mfrister
force-pushed
the
philips-air-purifier
branch
4 times, most recently
from
fd21284 to
2ab1a6a
Compare
mfrister
force-pushed
the
philips-air-purifier
branch
2 times, most recently
from
492a961 to
0ec4754
Compare
|
@iMicknl I think I've addressed all your comments, I added a few comments where things were not completely clear. I think this is ready for another round of review. I hope it was ok for me to resolve the review conversations that seemed to be clearly resolved - please let me know if you like another workflow better. Before merging (however far from that we might be 😀), I'd like to have a look at an issue I'm experiencing when running the integration locally in a Docker container, where the device is regularly shown as unavailable when it's turned off. I suspect this might be an issue with NAT on the machine dropping UDP mappings between status updates from the device. The updates are sent a lot less frequently when the device is turned off (every few minutes instead of every few seconds). The issue never appeared on my development machine where HA is running on the host (it's also a Mac and not a Linux box). I think it would be nice if I could work around that (most certainly in the phipsair package), e.g. by regularly sending packets to the device to keep the NAT mapping alive. |
mfrister
force-pushed
the
philips-air-purifier
branch
from
0ec4754 to
129f64b
Compare
mfrister
force-pushed
the
philips-air-purifier
branch
from
5d4a854 to
6dfc413
Compare
|
@iMicknl I've found a solution to the NAT issues - a regular keepalive request to a simple endpoint ensures the NAT keeps the UDP stream mapping. Device availability in a non-host-networking Docker container has been good since this change. As a positive side-effect, this has also improved connection test reliability during the config flow - I've discovered a better endpoint while debugging the NAT issues. I made the changes in the phipsair package, I've only added a commit here bumping the version and I've rebased again onto |
|
@mfrister this looks really promising. I'm currently maintaining https://github.com/kongo09/philips-airpurifier-coap which supports many more models, comes with more entities and brings some corresponding icons as well. If I can be of any help to bring this together, I'm totally happy to abandon the custom integration in favor for proper core support. |
|
The custom integration you're maintaining looks really interesting and I think the integration I wrote here could benefit of a lot from parts implemented there. The main reason why I didn't begin with it (or another fork) is the missing license - all the forks from betaboon/denaun don't have a license, so I don't think we can copy anything into Home Assistant core and will basically have to treat the code as proprietary :/ If some parts come from known sources with known licenses (e.g. the icons), we might be able to reuse them if they're compatible with HA's license and the CLA. Before putting more effort into this, I'd like to see that it's actually viable to get this MR merged - doesn't look easy to get another review so far. Once this is done, let's discuss how we could go forward with extending the core component - there's lots to do. How can I contact you? |
|
I see the problem. I'm happy to contribute whatever I've written myself under whatever license is required, e.g. I've created the icons myself in SVG but the design is obviously done by Philips. Not sure what that means - after all HA is also using brand logos all over the place and I don't think they have a written permission by the brand owners documented anyhwere. In terms of code, I have massively extended the betaboon stuff, so again, I'm happy to bring that in. My username here is the same on gmail, so just shoot my a PM. |
|
@balloob I've seen you review a few of the recently added integrations and I'm not sure how to continue with this MR after having received no feedback for roughly a month, so I hope you can help me with a bit of advice. From my point of view, I've addressed all points raised by @iMicknl, either by improving the code or by replying to them. Can I do anything to move this MR forward (does it have the right labels for it to be picked up for another review?) or do I need to adjust my expectations and just wait for a few more weeks? |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
@github-actions I'd still like to get this merged - waiting for a review. |
We can't query the device on startup and `device_info` is called only directly after adding the device, so there's no status available yet.
A status callback is enough, there's no need for a separate unavailable callback - we can just pass None as status.
Also make a device to HA speed map a constant.
Update the package to 0.4.0 with the reliable client.
Home Assistant doesn't seem to call these methods if the device is unavailable.
mfrister
force-pushed
the
philips-air-purifier
branch
from
6dfc413 to
ac9eca6
Compare
The new phipsair version has type annotations.
Increases reliability when connecting to the purifier via a NAT and makes the connection test more reliable and faster.
mfrister
force-pushed
the
philips-air-purifier
branch
from
ac9eca6 to
f6b83fd
Compare
|
@frenck It looks like this PR has been stuck in the review process somehow - I haven’t received any reply since April after putting in considerable effort to address all feedback I previously got. Could you please help getting this PR going again? |
|
Sorry, but I am up in the montains in Austria with my family enjoying my holiday. Please, Thanks! ../Frenck |
|
I've now attempted to find someone to give more feedback on this PR over the course of 6 months by different ways of communication, without success. Unfortunately, I don't have the patience and motivation to continue working on this with no feedback for such a long time, so I will close the PR for now. |
Proposed change
This adds an integration for Philips Air Purifiers, currently tested with a Philips AC2889/10.
The integration currently supports:
Limitations:
aiocoap, which already seems to already be used at least bypytradfri, which in turn is used in Home Assistant as well. I've removed the more intrusive monkey patch in version 3.0.1 ofphipsairto prevent potential issues with other users of aiocoap. There's one monkey patch left that fixes an exception when shutting down device connections.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.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: