-
-
Notifications
You must be signed in to change notification settings - Fork 221
Add ADC Value to PIR Enabled Switches #1263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ryenitcher
commented
Nov 15, 2024
- Add: ADC value reporting to PIR enabled switches, so that it may be used in polling automations.
- Add: ADC trigger state value reporting, for simpler automations.
- Add: Ability for features to use custom value parsers.
Example outputs for KS200M:Nothing triggering PIR:
Hand in front of switch Triggering PIR:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1263 +/- ##
==========================================
+ Coverage 92.57% 92.61% +0.03%
==========================================
Files 147 147
Lines 9185 9288 +103
Branches 938 940 +2
==========================================
+ Hits 8503 8602 +99
- Misses 492 494 +2
- Partials 190 192 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
What I've managed to learn:
Here is a sample csv with time, adc_value readings for reference.
|
I'm not sure about the complication with the range value parsing. Wouldn't it be simpler to just have two different features If this needs to query every second to be effective there should be a separate async function to get the current adc. This way a consumer can run that query every second in isolation from the rest of the device queries. Could you please separate out the (If you update to latest master the |
I agree with you that the complication on the range value parsing is far from optimal. The goal there was to maintain the existing behavior where the range could be set to either a number or a range enum value, but I never really liked that feature to begin with. If you are good with removing it, since I doubt anyone has even used it yet, I can strip it out and simplify the logic so that the range class and range value are independent entities that don't require the advanced parsing. |
The other remaining question I had is if it's wise to expose a simulated pir_triggered value or not given that the exact logic of the switch is not known, and we cannot simulate it exactly. I would put it as a debug feature but then I would worry that someone in Home Assistant would find it and then get confused as to why it is seemingly broken. I personally find value in it, but I mostly understand the logic and limitations of the pir_triggered value. |
821a358
to
786cd93
Compare
https://github.com/python-kasa/python-kasa/blob/master/kasa/smart/modules/energy.py#L76 |
I've added an Let me know if there is anything else I should change for this. I'm still missing some coverage over parts of the CLI parameter handling, but I'm not sure if it needs the additional tests, and/or if the additional tests should go in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nearly there with just a few nits left. I think the test coverage is ok btw.
payload = {"index": range.value} | ||
return await self.call("set_trigger_sens", payload) | ||
|
||
def _parse_range_value(self, value: str) -> Range: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that the library is python >= 3.11 could this all be made simpler by converting Range
to StrEnum
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can tell, StrEnum
will not improve this use case unfortunately, as it still does not define a proper Enum.get(key, default)
.
The enum is also a mapping of the integer returned by trigger_index
, so perhaps we could use IntEnum
? But then it comes down to how you want the range to be presented to the user, as 0, 1, 2, 3
or FAR, NEAR, MID, CUSTOM
. Currently it will accept the range enum names, in upper or lower case, but of course that relies on the range classes not changing on future devices.
It's cool to learn about the new StrEnum
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't completely follow the Enum.get(key, default)
. Can't you just catch a value error? StrEnum would mean the method could take str | StrEnum
which could mean more flexibility when we have a common interface across devices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the Enum.get()
is kind of pointless now that I think about it more, since in a failure case an exception will be thrown anyways, so there is no point in avoiding the underlying exception to begin with.
StrEnum
is a subclass of str
so I don't think that StringEnum gains us anything here, since that would still require a mapping to the index IDs of the level.
Hopefully this latest revision makes more sense?
@@ -30,6 +93,11 @@ def _initialize_features(self) -> None: | |||
if "get_config" not in self.data: | |||
return | |||
|
|||
# Require that ADC value is also present. | |||
if "get_adc_value" not in self.data: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the expectation that all PIR devices will have get_adc_value
or could some only have get_config
causing this line to impact those devices?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect that all PIR devices will have the get_adc_value
, but of course I have no way of actually proving that for future devices. It's possible an updated model could come out with a digital/event driven PIR sensor instead, but then I would expect the get_config
to take on a different form than what is currently supported as well.
I can remove the check if that's better for device support.
Sorry for the delay in updating this one, I think it should be good to go now, minus anything new you find. |
@sdb9696 do you have any thoughts on this latest version? |
I started to review it today but had to put it on pause. Will endeavour to finish tomorrow am. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few suggestions that are hopefully not too onerous.
kasa/iot/modules/motion.py
Outdated
async def get_pir_state(self) -> PIRStatus: | ||
"""Return real-time PIR status.""" | ||
current = await self.call("get_adc_value") | ||
return PIRStatus(current["value"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this update the internal data
before returning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be resolved with the latest commit.
payload = {"index": range.value} | ||
return await self.call("set_trigger_sens", payload) | ||
|
||
def _parse_range_value(self, value: str) -> Range: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't completely follow the Enum.get(key, default)
. Can't you just catch a value error? StrEnum would mean the method could take str | StrEnum
which could mean more flexibility when we have a common interface across devices.
|
||
|
||
@dataclass | ||
class PIRStatus: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should take PIRConfig
in it's constructor as init_only
and all the get_
methods can then attributes. If you include the mashumaro DataClassMixin
the data class will then get to_dict()
method and you can set all the attributes in post init. I think that is more intuitive than returning a status that then needs to be provided config to get more values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree updating the PIRStatus to be more of a standalone makes sense. I had originally written it thinking that there might be cases where the ADC value is read and then the config is updated, but that's rather unlikely now that I think about it.
Hopefully using properties is agreeable to you, in doing so we can avoid computing values that are never used, and slowing down polling of get_pir_state()
.
Hey @ryenitcher, any plans to finish this off before we cut the next release in the next day or so? There's not much left to do afaict. |
- Add: ADC value reporting to PIR enabled switches, so that it may be used in polling automations. - Add: ADC trigger state value reporting, for simpler automations. - Add: Ability for features to use custom value parsers.
- Fix: Read PIR as ADC value centered around ADC midpoint/zeropoint.
- Fix: Back out custom value parser logic. - Fix: Remove overloaded range set functionality. - Fix: Improve error messages when user forgets quotes around cli arguments.
- Add: Async function to get the current state of the ADC PIR sensor. - Fix: Move ADC PIR state and config into proper dataclasses. - Add: Add CLI test for passing bad quotes to feature set.
- Fix: Decrease log message severity in `feature.py`. - Fix: Rename `_set_range_cli` to `_set_range_from_str`. - Add: Basic tests for string to Motion Range value converter function.
- Fix: Make PIRStatus into proper dataclass. - Fix: Update internal PIR state when async polling.
Sorry for the delay in getting back to this, the end of the year is always far more busy than I expect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good with the latest updates, thanks @ryenitcher!
## [0.10.0](https://github.com/python-kasa/python-kasa/tree/0.10.0) (2025-01-26) [Full Changelog](0.9.1...0.10.0) **Release summary:** This release brings support for many new devices, including completely new device types: - Support for Tapo robot vacuums. Special thanks to @steveredden, @MAXIGAMESSUPPER, and veep60 for helping to get this implemented! - Support for hub attached cameras and doorbells (H200) - Improved support for hubs (including pairing & better chime controls) - Support for many new camera and doorbell device models, including C220, C720, D100C, D130, and D230 Many thanks to testers and new contributors - @steveredden, @DawidPietrykowski, @Obbay2, @andrewome, @ryenitcher and @etmmvdp! **Breaking changes:** - `uses_http` is now a readonly property of device config. Consumers that relied on `uses_http` to be persisted with `DeviceConfig.to_dict()` will need to store the value separately. - `is_color`, `is_dimmable`, `is_variable_color_temp`, `valid_temperate_range`, and `has_effects` attributes from the `Light` module are deprecated, consumers should use `has_feature("hsv")`, `has_feature("brightness")`, `has_feature("color_temp")`, `get_feature("color_temp").range`, and `Module.LightEffect in dev.modules` respectively. Calling the deprecated attributes will emit a `DeprecationWarning` and type checkers will fail them. - `alarm_volume` on the `smart.Alarm` module is changed from `str` to `int` **Breaking changes:** - Make uses\_http a readonly property of device config [\#1449](#1449) (@sdb9696) - Allow passing alarm parameter overrides [\#1340](#1340) (@rytilahti) - Deprecate legacy light module is\_capability checks [\#1297](#1297) (@sdb9696) **Implemented enhancements:** - Expose more battery sensors for D230 [\#1451](#1451) - dumping HTTP POST Body for Tapo Vacuum \(RV30 Plus\) [\#937](#937) - Add common alarm interface [\#1479](#1479) (@sdb9696) - Add common childsetup interface [\#1470](#1470) (@sdb9696) - Add childsetup module to smartcam hubs [\#1469](#1469) (@sdb9696) - Add smartcam pet detection toggle module [\#1465](#1465) (@DawidPietrykowski) - Only log one warning per unknown clean error code and status [\#1462](#1462) (@rytilahti) - Add childlock module for vacuums [\#1461](#1461) (@rytilahti) - Add ultra mode \(fanspeed = 5\) for vacuums [\#1459](#1459) (@rytilahti) - Add setting to change carpet clean mode [\#1458](#1458) (@rytilahti) - Add setting to change clean count [\#1457](#1457) (@rytilahti) - Add mop module [\#1456](#1456) (@rytilahti) - Enable dynamic hub child creation and deletion on update [\#1454](#1454) (@sdb9696) - Expose current cleaning information [\#1453](#1453) (@rytilahti) - Add battery module to smartcam devices [\#1452](#1452) (@sdb9696) - Allow update of camera modules after setting values [\#1450](#1450) (@sdb9696) - Update hub children on first update and delay subsequent updates [\#1438](#1438) (@sdb9696) - Add support for doorbells and chimes [\#1435](#1435) (@steveredden) - Implement vacuum dustbin module \(dust\_bucket\) [\#1423](#1423) (@rytilahti) - Allow https for klaptransport [\#1415](#1415) (@rytilahti) - Add smartcam child device support for smartcam hubs [\#1413](#1413) (@sdb9696) - Add powerprotection module [\#1337](#1337) (@rytilahti) - Add vacuum speaker controls [\#1332](#1332) (@rytilahti) - Add consumables module for vacuums [\#1327](#1327) (@rytilahti) - Add ADC Value to PIR Enabled Switches [\#1263](#1263) (@ryenitcher) - Add support for cleaning records [\#945](#945) (@rytilahti) - Initial support for vacuums \(clean module\) [\#944](#944) (@rytilahti) - Add support for pairing devices with hubs [\#859](#859) (@rytilahti) **Fixed bugs:** - TP-Link HS300 Wi-Fi Power-Strip - "Parent On/Off" not functioning. [\#637](#637) - Convert carpet\_clean\_mode to carpet\_boost switch [\#1486](#1486) (@rytilahti) - Change category for empty dustbin feature from Primary to Config [\#1485](#1485) (@rytilahti) - Report 0 for instead of None for zero current and voltage [\#1483](#1483) (@ryenitcher) - Disable iot camera creation until more complete [\#1480](#1480) (@sdb9696) - ssltransport: use debug logger for sending requests [\#1443](#1443) (@rytilahti) - Fix discover cli command with host [\#1437](#1437) (@sdb9696) - Fallback to is\_low for batterysensor's battery\_low [\#1420](#1420) (@rytilahti) - Fix iot strip turn on and off from parent [\#639](#639) (@Obbay2) **Added support for devices:** - Add D130\(US\) 1.0 1.1.9 fixture [\#1476](#1476) (@sdb9696) - Add D100C\(US\) 1.0 1.1.3 fixture [\#1475](#1475) (@sdb9696) - Add C220\(EU\) 1.0 1.2.2 camera fixture [\#1466](#1466) (@DawidPietrykowski) - Add D230\(EU\) 1.20 1.1.19 fixture [\#1448](#1448) (@sdb9696) - Add fixture for C720 camera [\#1433](#1433) (@steveredden) **Project maintenance:** - Update ruff to 0.9 [\#1482](#1482) (@sdb9696) - Cancel in progress CI workflows after new pushes [\#1481](#1481) (@sdb9696) - Update test framework to support smartcam device discovery. [\#1477](#1477) (@sdb9696) - Add error code 7 for clean module [\#1474](#1474) (@rytilahti) - Enable CI workflow on PRs to feat/ fix/ and janitor/ [\#1471](#1471) (@sdb9696) - Add commit-hook to prettify JSON files [\#1455](#1455) (@rytilahti) - Add required sphinx.configuration [\#1446](#1446) (@rytilahti) - Add more redactors for smartcams [\#1439](#1439) (@sdb9696) - Add KS230\(US\) 2.0 1.0.11 IOT Fixture [\#1430](#1430) (@ZeliardM) - Add tests for dump\_devinfo parent/child smartcam fixture generation [\#1428](#1428) (@sdb9696) - Raise errors on single smartcam child requests [\#1427](#1427) (@sdb9696)