Add WhiteLamp SmartCam module for outdoor cameras (e.g. C325WB)#1713
Add WhiteLamp SmartCam module for outdoor cameras (e.g. C325WB)#1713ijardillier wants to merge 2 commits into
Conversation
- New WhiteLamp module under kasa/smartcam/modules/ - Exposes white_lamp_state (on/off toggle via reverseWhitelampStatus) and white_lamp_brightness (1-100, only if device supports it) - Adds Module.SmartCamWhiteLamp and Module.WhiteLamp module names - Adds fixture for C325WB(EU) 1.0 / 1.4.4 - Adds unit tests in tests/smartcam/modules/test_whitelamp.py - Fixes fake protocol to handle reverseWhitelampStatus toggle command
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1713 +/- ##
==========================================
+ Coverage 93.22% 93.24% +0.01%
==========================================
Files 157 158 +1
Lines 9815 9854 +39
Branches 1003 1005 +2
==========================================
+ Hits 9150 9188 +38
Misses 472 472
- Partials 193 194 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
to match the class name used as key in dev.modules - Add SmartCamModule.SmartCamWhiteLamp assertion in test_whitelamp_is_on - Add test_whitelamp_no_brightness to cover the case where brightness is not supported by the device"
rytilahti
left a comment
rytilahti
left a comment
There was a problem hiding this comment.
Thanks for the PR @ijardillier, I have done a quick review on it. The module looks very straightforward, so this can be merged rather quickly after the issues mentioned in the comments are fixed.
| @@ -0,0 +1,96 @@ | |||
| """Module for white lamp (floodlight) controls on Tapo outdoor cameras.""" | |||
There was a problem hiding this comment.
If it is a floodlight, feel free to name the class also like that, no need to keep the internal namings :-) The name of the class and the file could be simply FloodLight and floodlight.py respectively. If you want to keep the internal naming available in docstrings, you can put that in parentheses.
| """Implementation of white lamp controls for outdoor cameras (e.g. C325WB). | ||
|
|
||
| The white lamp is the built-in floodlight that can be triggered manually | ||
| or by motion events. It is distinct from the LED status indicator. |
There was a problem hiding this comment.
| """Implementation of white lamp controls for outdoor cameras (e.g. C325WB). | |
| The white lamp is the built-in floodlight that can be triggered manually | |
| or by motion events. It is distinct from the LED status indicator. | |
| """Implementation of floodlight controls for outdoor cameras. |
| @@ -0,0 +1,96 @@ | |||
| """Module for white lamp (floodlight) controls on Tapo outdoor cameras.""" | |||
There was a problem hiding this comment.
| """Module for white lamp (floodlight) controls on Tapo outdoor cameras.""" | |
| """Module for floodlight controls ("whitelamp" module) on Tapo outdoor cameras.""" |
If it is a floodlight, feel free to name the class also like that, no need to keep the internal namings :-) The name of the class and the file could be simply FloodLight and floodlight.py respectively. If you want to keep the internal naming available in docstrings, you can put that in parentheses.
| Feature( | ||
| self._device, | ||
| id="white_lamp_state", | ||
| name="White lamp state", |
There was a problem hiding this comment.
See my comment above, these are user-interfacing names.
There was a problem hiding this comment.
I am not sure what is going on here, newline separator changes?
| _LOGGER = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class WhiteLamp(SmartCamModule): |
There was a problem hiding this comment.
| class WhiteLamp(SmartCamModule): | |
| class WhiteLamp(SmartCamModule, LightInterface): |
It would be nice to inherit from the lightinterface like other light modules do. This will make it also easier to integrate elsewhere.
2 participants
New WhiteLamp module under kasa/smartcam/modules/
Pre-commit OK
Tests : OK
Also tested with python-kasa CLI and my own Home Assistant instance with 2 C325WB cameras.
Hope all is compliant with your guidelines and nothing missing.