Repository navigation
Conversation
805c62c to
19bbca1
Compare
|
@TheJulianJES would be great if you could have a look at it. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1916 +/- ##
==========================================
- Coverage 99.51% 99.51% -0.01%
==========================================
Files 61 62 +1
Lines 12777 12950 +173
==========================================
+ Hits 12715 12887 +172
- Misses 62 63 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
zigpy-review-bot
left a comment
There was a problem hiding this comment.
The DST computation is correct on every zone I tried, the tests are thorough, and zha and zha-quirks both pass against this branch. Most of what follows is about the ZHA wiring, which is smaller than the PR description suggests but has a trap in it. There is also one default-path behavior change to document, and a suggestion to simplify the async discovery path.
The ZHA follow-up
- zha already has the value.
ZHAConfiguration.local_timezone(zha/application/helpers.py:431) is set by HA fromhass.config.time_zoneand updated on core-config changes (the climate platform already uses it). Wiring it intoControllerApplication.new(local_timezone=...)plus thelocal_timezonesetter on updates is close to a one-liner, and it does not depend on the OS timezone. - The trap: that field defaults to
datetime.UTC. Passed through unchanged, atimezoneinstance takes the fixed-offset branch here (authoritative=True), so a non-HA zha user would advertise UTC withMaster_for_Zone_and_DSTset. That is worse than today's system-local values. The follow-up should passNonewhen nothing is configured. - Once wired, every HA install gets a configured timezone, so
TimeZoneandStandardTimechange from the current offset (DST included, as ondev) to the standard offset. That is spec-correct (ZCL R8 §3.12.2.2.3) and matches zigbee-herdsman. But a device that reads onlyTimeZone/StandardTimeand ignores the DST attributes will be an hour off while DST is active. This is the most visible change the series makes, so please call it out in this PR's description (and the ZHA one). Europe/Dublinfrom the system tzdata comes out asTimeZone=+3600,DstShift=-3600, with the DST window over the winter (2026-10-25 → 2027-03-28). The spec allows a signedDstShift(§3.12.2.2.6) and the local-time arithmetic works out, but negative shifts are unusual for device firmware.
Default path (no timezone configured)
With local_timezone=None, TimeService._snapshot returns authoritative=False, so time_status drops Master_for_Zone_and_DST. On dev, handle_read_attribute_time_status always sets it. The other values are unchanged from dev. That is defensible by §3.12.2.2.2: in this path TimeZone is not the standard offset and the DST attributes are invalid. Still, it is a change from dev and from zigbee-herdsman, which always reports timeStatus: 0b1101. Please mention it in the PR description. Maintainers can decide whether to keep dev's bit for unconfigured setups.
Consider (maintainer call)
The async path looks heavier than the problem. For ZoneInfo zones, _discover_periods takes about 8–17 ms here, and the result stays cached until the UTC year rolls over. Only a synthetic permanent-DST tzinfo (as in test_time_service_unbounded_dst) widens the scan to 2000–2136, which takes seconds. Avoiding that cost costs a fair amount of machinery: the executor offload, the _pending futures under asyncio.shield, the generation counter, and moving every Time Read Attributes onto a task. A synchronous handle_cluster_general_request override could install the application's TimeService.snapshot(now) as _read_snapshot (the cached path the direct handlers already use), call super(), and restore. That keeps the one-instant-per-response property and drops async_snapshot, _pending and _generation, along with their concurrency tests. If the service stays, a public accessor would be cleaner than the cluster reaching into application._time_service.
Other
- This overlaps #1806 (same feature, also open). Maintainers should pick one and close the other.
- Nit:
ZIGBEE_EPOCHinzigpy/zcl/clusters/general.pyis no longer used there and is now defined again inzigpy/time.py. Import one from the other. - codecov/patch flags one uncovered line: the direct-call fallback in
Time._get_time_snapshot(general.py:1143).
Verified (8 checks)
- zigpy suite at 19bbca1: 1394 passed.
zigpy/time.pyhas 100% coverage;general.pymisses only line 1143. - Cross-repo: installed this branch editable into fresh zha and zha-device-handlers worktrees (current
dev). zha: 1458 passed; quirks: 4843 passed, 2 xfailed. - Downstream subclasses: the only quirk subclassing
TimeisDanfossTimeCluster, which writes time to the device and overrides no read handler. No radio lib overridesControllerApplication.new, so the new keyword-onlylocal_timezoneis additive. - zha's
local_timezonefield (defaultdatetime.UTC) is set in HA Core'szha/helpers.pyfromhass.config.time_zoneand refreshed inzha/__init__.pyon config updates. - Probed
TimeService.snapshotacross Europe/Berlin, Australia/Sydney, America/Santiago (cross-year southern-hemisphere seasons), Europe/Dublin (negative DST), Antarctica/Troll (2 h shift), Asia/Tokyo, Europe/Moscow, America/Sao_Paulo and Pacific/Apia (no DST), at four instants in 2026. Start/end/shift were correct each time;DstEndis the last DST second, as the inclusive §3.12.2.2.4 bound requires. async_snapshot: the retry loop runs at most twice, since a second discovery uses the reader's ownnow.remove_pendingruns before the shielded waiter resumes, and a timezone change during discovery doesn't publish to the cache. No issues found.- Within one scan window, zones whose next DST season is more than a year away (e.g. Africa/Cairo before its 2023 reinstatement) report "no DST". Local time stays correct, so this is not raised.
- On
dev,TimeZonefor the system-local path already includes DST:datetime.now().astimezone()gives a fixed-offset tz whosedst()isNone. The PR'szone is Nonepath keeps that and clears the bit, confirmed by probe.
| standard_time=zigbee_time(now + timedelta(seconds=standard_offset)), | ||
| local_time=zigbee_time(now + timedelta(seconds=offset)), | ||
| ) | ||
| if zone is None or values.time == INVALID_TIME: |
There was a problem hiding this comment.
With no timezone configured, this returns authoritative=False, so time_status drops Master_for_Zone_and_DST (set unconditionally on dev). §3.12.2.2.2 supports clearing it, but it's a change from dev and from herdsman's 0b1101. Please note it in the PR description. Also, the ZHA follow-up should pass None here rather than zha's datetime.UTC default, or unconfigured setups would advertise UTC as authoritative. See the review body.
| self._cache = (zone, now.year, periods) | ||
| return self._snapshot(now, zone, periods) | ||
|
|
||
| async def async_snapshot(self, now: datetime) -> TimeSnapshot: |
There was a problem hiding this comment.
For ZoneInfo zones, discovery took 8–17 ms per zone per year in my probes. This executor, _pending and generation machinery, plus putting every Time read on a task in general.py, seems a lot to avoid that. A synchronous cached snapshot installed in the handle_cluster_general_request override would keep the consistent-snapshot property with much less state. Maintainer call.
|
@TheJulianJES thanks for the review. Would you mind looking at it and telling me if I shall resolve all or only parts of it as well as decide with regards to P.S. No pressure from my side! Whenever you have time it would be appreciated. |
Based on #1806 I created this PR.
Implement
dst_start,dst_end, anddst_shiftwhich reads with accurate timezone rules, including cross-year seasons and signed or non-hour shifts.Changes
zigpy/time.py): Add shared, cached DST calculations. Offload uncached discovery to avoid blocking the event loop.zigpy/application.py: Add optional local_timezone configuration and invalidate cached rules when it changes.zigpy/zcl/clusters/general.py: Add DST handlers and consistent per-request snapshots for specification-compliant attribute responses.tests/test_application.py: Verify timezone initialization and updates.tests/test_zcl_clusters.py: Cover DST boundaries, caching, concurrency, cancellation, and response handling.Integration Note
ZHA would require it's own PR to provide the HA configured timezone instead of depending on OS.