Fix SMARTCAM Time module and update tests by ZeliardM · Pull Request #1659 · python-kasa/python-kasa

ZeliardM · 2026-02-23T15:09:02Z

This will update the SMARTCAM Time module so that manually setting the time will function correctly. An important note is that this turns off NTP time updating.

Copilot

Pull request overview

This PR fixes the SmartCam Time module's set_time method to properly set the device time manually. The fix addresses three key issues: formatting local_time in UTC (not machine local time), removing microseconds from the timestamp format, and switching timing_mode from "ntp" to "manual" so devices accept manual time updates.

Changes:

Fixed time formatting to use UTC without microseconds (format: 'YYYY-MM-DD HH:MM:SS')
Added logic to set timing_mode to "manual" before updating clock_status (for devices that support it)
Combined zone_id and timing_mode updates into a single API call for efficiency
Added comprehensive unit tests covering all device types (cameras with timing_mode, hubs without timing_mode)

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
kasa/smartcam/modules/time.py	Rewrote set_time method to format time in UTC, set timing_mode to manual, and handle devices without timing_mode field
tests/test_common_modules.py	Added three comprehensive unit tests verifying UTC formatting, ZoneInfo handling, and H200 hub compatibility
tests/fakeprotocol_smartcam.py	Fixed variable shadowing issue by renaming nested 'section' variable to 'section_data'

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

codecov · 2026-02-23T15:30:14Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.23%. Comparing base (76d9f68) to head (fe59298).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1659   +/-   ##
=======================================
  Coverage   93.22%   93.23%           
=======================================
  Files         157      157           
  Lines        9815     9825   +10     
  Branches     1003     1004    +1     
=======================================
+ Hits         9150     9160   +10     
  Misses        472      472           
  Partials      193      193

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ZeliardM · 2026-02-27T00:45:47Z

Still working on the correct handling of the time zone information here.

ZeliardM · 2026-03-01T20:02:53Z

@rytilahti After further investigation, there appears no way for set_time to directly set the clock time on SMARTCAM devices. The set_time commands have been updated to correctly handle the timezone now and removed the sending of the seconds_from_1970 through the setTimezone function since it does nothing. Confirmed that this is all handled correctly now and that if the device is connected to the internet, it will pull the correct clock time via NTP.

This should be good to go.

kasa/smartcam/modules/time.py

tests/fakeprotocol_smartcam.py

tests/test_cli.py

rytilahti · 2026-03-01T22:56:05Z

tests/test_common_modules.py

        await time_mod.set_time(test_time)
        await dev.update()
-        assert time_mod.time == test_time
+        if isinstance(dev, SmartCamDevice):


I do really like clean code, so adding a separate test for handling the special behavior would be a better approach.

Re-worked the tests here to work with smartcam devices only for testing this.

It would be nicer to add a separate test for the special "logs a warning" case that to make it explicit, and skip this test for smartcam devices.

This way we can cover both paths without having a need for conditionals in tests which are most of the time a bad idea in tests.

tests/test_common_modules.py

ZeliardM · 2026-03-02T22:50:45Z

@rytilahti Yea, I forgot to push the commit, lol I re-worked it yet again. Give it another once over and let me know if this looks cleaner. I made the changes to the fake_smartcamprotocol.py for the multiple calls issue in the tests. Then, modified the tests correctly in the test_smartcamdevice.py, added instance checks in the test_cli.py and test_common_modules.py to handle the specifics that the smartcam/modules/time.py only sets the timezone and not the time and modified those tests accordingly and to catch the warning in the test_common_module.py. This looks cleaner to me now as well but let me know if I'm still missing it.

This reverts commit f9c3446.

This reverts commit 16288b0.

This reverts commit 19ccd8e.

This reverts commit f5c74ee.

This reverts commit af2fd88.

rytilahti

While I was reviewing (see the comments inline) and reading the PR description, I started to give second thoughts on whether we really want to do this, i.e., what's the use case?

If the reason for this is to allow changing the time zone, perhaps we should add an explicit method for doing that & just raise an exception about setting the time is not possible? This way, the interface would keep working on as expected which would keep our tests clean.

What do you think, @ZeliardM?

rytilahti · 2026-04-05T23:19:55Z

tests/test_common_modules.py

        assert time_mod.time != test_time

-        await time_mod.set_time(test_time)
+        caplog.clear()


No need to clear() when the log shouldn't have captured anything yet.

rytilahti · 2026-04-05T23:24:06Z

tests/test_common_modules.py

        await time_mod.set_time(test_time)
        await dev.update()
-        assert time_mod.time == test_time
+        if isinstance(dev, SmartCamDevice):


It would be nicer to add a separate test for the special "logs a warning" case that to make it explicit, and skip this test for smartcam devices.

This way we can cover both paths without having a need for conditionals in tests which are most of the time a bad idea in tests.

rytilahti · 2026-04-05T23:25:14Z

tests/test_common_modules.py

+        if isinstance(dev, SmartCamDevice):
+            assert time_mod.timezone == new_time.tzinfo
+        else:
+            assert time_mod.time == new_time


See my comment above; it's better to add separate tests for the special cases than adding conditonals with isinstance.

rytilahti · 2026-04-05T23:28:57Z

kasa/smartcam/modules/time.py

+    @staticmethod
+    def _format_utc_offset(offset: timedelta | None) -> str:
+        """Format a timedelta offset as UTC+HH:MM/UTC-HH:MM."""
+        if offset is None:
+            offset = timedelta(0)
+
+        total_seconds = int(offset.total_seconds())
+        sign = "+" if total_seconds >= 0 else "-"
+        total_seconds = abs(total_seconds)
+        hours, remainder = divmod(total_seconds, 3600)
+        minutes = remainder // 60
+        return f"UTC{sign}{hours:02d}:{minutes:02d}"


Please add tests for this function to the relevant test_time.py file with a parametrized set of test cases.

rytilahti · 2026-04-05T23:31:44Z

tests/test_cli.py

+    if isinstance(dev, SmartCamDevice):
+        assert time_mod.timezone.utcoffset(time_mod.time) == initial_timezone.utcoffset(
+            time_mod.time
+        )
+    else:
+        assert time_mod.time == dt.replace(tzinfo=time_mod.timezone)


There should be no need for special handling in the cli tests. Or it should be covered with a separate, explicit test.

ZeliardM · 2026-04-06T00:31:14Z

@rytilahti The reason I kept it all as the same paths as the other device types instead of separating out of an explicit method is because the other device types, IOT and SMART, all handle time and timezone under the same method and interface. SMARTCAM is the only one I have seen that cannot do an explicit set time function. So the modifications were to keep it all the same. Is what you're asking if I think we should separate timezone and time for all device types? Now, given that setting the time is not an option and the timezone should be NTP it really could be irrelevant to have this at all for SMARTCAM and just raise the error. Another thing is for people who do not use the App to set up the camera and set things up from scratch with python-kasa and don't allow the camera out to the internet then at that point NTP wouldn't work to set the time or timezone and they would need the option to at least set the timezone but still setting the time is not available. So if it were me, I would probably just raise the error and move on. Either use case, this doesn't really offer much in the way of functionality that would benefit the user in a meaningful way.

Copilot AI review requested due to automatic review settings February 23, 2026 15:09

Copilot started reviewing on behalf of ZeliardM February 23, 2026 15:09 View session

Copilot AI reviewed Feb 23, 2026

View reviewed changes

ZeliardM mentioned this pull request Feb 25, 2026

Out of box Tapo TC40: discovery not working, authentication failure on wifi setup #1628

Open

rytilahti reviewed Mar 1, 2026

View reviewed changes

rytilahti reviewed Mar 2, 2026

View reviewed changes

tests/test_common_modules.py Outdated Show resolved Hide resolved

ZeliardM added 12 commits March 4, 2026 13:33

Fix SMARTCAM Time module and update tests

23d858f

Revert "Fix SMARTCAM Time module and update tests"

b2e8547

This reverts commit f9c3446.

Retry smartcam time module fixes

1a74dcc

Fix tests

36ab4d1

Update test coverage

27f48b1

Fix clock handling for smartcam and tests

30c8bca

Revert "Fix clock handling for smartcam and tests"

39d734c

This reverts commit 16288b0.

Revert "Update test coverage"

cccc701

This reverts commit 19ccd8e.

Revert "Fix tests"

89ec13c

This reverts commit f5c74ee.

Revert "Retry smartcam time module fixes"

ead73f5

This reverts commit af2fd88.

Retry again at time fixes and better test handling

0e5ded6

Fix test coverage

aef9930

ZeliardM force-pushed the feature/device-time branch from a1ea53b to aef9930 Compare March 4, 2026 18:33

Merge branch 'python-kasa:master' into feature/device-time

fe59298

rytilahti requested changes Apr 5, 2026

View reviewed changes

Uh oh!

Conversation

ZeliardM commented Feb 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

codecov bot commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ZeliardM commented Feb 27, 2026

Uh oh!

ZeliardM commented Mar 1, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ZeliardM commented Mar 2, 2026

Uh oh!

rytilahti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZeliardM commented Apr 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Feb 23, 2026 •

edited

Loading