Add preset support for Tapo cameras #1615
Conversation
e75e186 to
c7fe2f9
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1615 +/- ##
==========================================
+ Coverage 92.82% 92.84% +0.02%
==========================================
Files 157 157
Lines 9643 9678 +35
Branches 976 980 +4
==========================================
+ Hits 8951 8986 +35
Misses 492 492
Partials 200 200 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bd856bf to
20e8f10
Compare
20e8f10 to
43e77ce
Compare
There was a problem hiding this comment.
Thanks for the PR @mrwogu! This looks good to me on the surface, and it is ready to go after the tests are improved a bit.
I would suggest exposing this also through the feature interface as that makes it much easier to use in downstream (i.e., in the cli tool and in homeassistant), but I leave it for you to decide.
ee98828 to
c5897ac
Compare
cfc865d to
dc69f12
Compare
dc69f12 to
1eee003
Compare
|
Thanks for the suggestions, @rytilahti ! All changes have been implemented. |
There was a problem hiding this comment.
Thanks for the updates, looks much better! I added some more comments, mostly about making it cleaner and more consistent with the rest of our code base.
kasa/smartcam/modules/pantilt.py
Outdated
| presets_response = await self._device._query_helper( | ||
| "getPresetConfig", {"preset": {"name": ["preset"]}} | ||
| ) |
There was a problem hiding this comment.
There is no need to manually query here, the query() will be used automatically.
You can then use the data property to access the module data like this:
python-kasa/kasa/smartcam/modules/alarm.py
Lines 110 to 113 in 347bf9a
kasa/smartcam/modules/pantilt.py
Outdated
| if self._presets and "preset" not in self._module_features: | ||
|
|
||
| async def set_preset(preset_name: str) -> None: | ||
| preset_id = self._presets.get(preset_name) | ||
| if preset_id: | ||
| await self.goto_preset(preset_id) | ||
|
|
||
| feature = Feature( | ||
| self._device, | ||
| "preset", | ||
| "Preset position", | ||
| container=self, | ||
| attribute_getter=lambda x: next(iter(self._presets.keys()), None), | ||
| attribute_setter=set_preset, | ||
| choices_getter=lambda: list(self._presets.keys()), | ||
| type=Feature.Type.Choice, | ||
| ) | ||
| self._add_feature(feature) |
There was a problem hiding this comment.
Please move this into _initialize_features().
There is no need to check if the feature already exists, as it will be called only once after the first update (and it's _post_update_hook), so you can keep updating the internal _presets in the post update hook.
kasa/smartcam/modules/pantilt.py
Outdated
|
|
||
| feature = Feature( | ||
| self._device, | ||
| "preset", |
There was a problem hiding this comment.
| "preset", | |
| "ptz_preset", |
Perhaps? There are other devices that have presets (e.g., lights), so it would be a good idea to avoid potential naming conflict if support for other presets is added at some point.
| assert preset_feature.value == first_preset_name | ||
|
|
||
| # Mock the protocol query for testing set_value | ||
| # This allows set_preset function body to be executed (lines 110-112) |
There was a problem hiding this comment.
Avoid naming lines as the code will live and these won't match anymore at some point.
| call_args = mock_protocol_query.call_args | ||
| assert "motor" in str(call_args) or "preset" in str(call_args).lower() | ||
|
|
||
| # Reset mock |
There was a problem hiding this comment.
| # Reset mock |
No need for obvious comments, please check other similar comments in the tests, and ask yourself if they bring extra value for the reader :-)
| if pantilt is None: | ||
| pytest.skip("Device does not have PanTilt module") |
There was a problem hiding this comment.
| if pantilt is None: | |
| pytest.skip("Device does not have PanTilt module") |
Instead of doing this check for every separate test, it would be cleaner to define a new parametrization and then use it instead of @device_smartcam, for inspiration see:
python-kasa/tests/smartcam/modules/test_childsetup.py
Lines 12 to 18 in 347bf9a
| try: | ||
| await preset_feature.set_value(invalid_preset_name) | ||
| # goto_preset should NOT be called because preset_id is empty string (falsy) | ||
| mock_query.assert_not_called() |
There was a problem hiding this comment.
Trying to set an invalid value should raise a ValueError, right? You could use with pytest.raises(ValueError) to make sure that is happening.
145a6fa to
73503d6
Compare
|
Thanks again for your valuable feedback, @rytilahti. They helped me understand exactly how this should work. I refactored it. It should be fine now. |
Add preset support for Tapo cameras (firmware 1.4.7+)
Description
This PR adds support for PTZ presets for Tapo cameras running newer firmware versions (e.g., C210 with firmware 1.4.7).
Problem
Newer firmware versions for Tapo cameras (like 1.4.7 for C210) have removed support for the generic
motormodule commands (getanddoonmotorpreset), returning error code-40210(Function not supported). This broke existing integrations that relied on these commands for preset management.Solution
This PR implements the new API methods required by these firmware versions:
getPresetConfig: To retrieve the list of presets.motorMoveToPreset: To move the camera to a specific preset.addMotorPostion: To save a new preset (note: the typo inPostionis part of the official API).The following methods have been added to the
PanTiltmodule:get_presets()goto_preset(preset_id)save_preset(name)Testing
Tested locally with a Tapo C210 running firmware 1.4.7.
get_presets()correctly returns the list of presets.goto_preset()successfully moves the camera.save_preset()successfully saves a new preset.