Migrate Aqara pet feeder ACN001 to quirks v2 and add onboard scheduling - #5294
Migrate Aqara pet feeder ACN001 to quirks v2 and add onboard scheduling#5294iamjoshk wants to merge 12 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #5294 +/- ##
==========================================
- Coverage 92.60% 92.43% -0.17%
==========================================
Files 424 424
Lines 14667 14802 +135
==========================================
+ Hits 13582 13682 +100
- Misses 1085 1120 +35 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
It would be incredible if this could land in ZHA. I currently use an HA automation to trigger the feeder, but I'm always worried about potential flakiness in my Zigbee network, an HA issue, or a power outage causing a missed feeding. |
TheJulianJES
left a comment
There was a problem hiding this comment.
We should ideally try to keep the refactor and new features in separate quirks PRs
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Nice piece of work overall — the unique_id continuity story here is the best part and it is done right, but the schedule write path has concrete on-wire defects that need fixing before this leaves draft. Marking this as comment-only because the PR is still a draft (it would otherwise be request-changes).
What is solid (verified locally): every migrated entity keeps its existing identity. All eleven unique_id_suffix="64704-<attr>" values reproduce the ZHA-native unique_ids exactly ({ieee}-1-64704-<suffix>), matching the _unique_id_suffix on AqaraPetFeederLEDIndicator, AqaraPetFeederChildLock, AqaraPetFeederLastFeedingSource/LastFeedingSize, PortionsDispensed, WeightDispensed, ServingSize, PortionWeight, AqaraPetFeederMode, AqaraPetFeederErrorDetected and AqaraPetFeederFeedButton. And because ZHA's Device._add_pending_entities() drops a pending entity whose (platform, unique_id) key is already taken, and native ZCL discovery is yielded before the quirks-v2 entities, the eleven v2 declarations are simply inert on today's ZHA — only the new schedule sensor is actually created — and they take over cleanly once zigpy/zha#883 removes the native classes. That is exactly the right shape for this migration.
Blockers
1. The schedule payload's length byte is hard-coded and is only correct for exactly three entries. _encode_schedule() builds header + b" " + data, where b" " (0x20 = 32) sits in the position the Aqara framing uses for the payload length. The joined ASCII payload is 11 * n - 1 bytes for n entries: 10, 21, 32, 43, 54 for one to five entries. Only n == 3 equals 32 — which is exactly the three-entry example in the PR description and the three-entry frame in test_aqara_feeder_schedule_sensor. Every other schedule size sends a wrong length. The class already has _build_feeder_attribute(), which computes this correctly and also emits a proper sequence byte; please route the schedule through it (self._build_feeder_attribute(SCHEDULING_STRING, payload, len(payload))) rather than hand-rolling a second framing.
2. The ZCL type and the Python value disagree, and the payload gets truncated by one byte on the wire. tv.type = 0x41 is octstr (single-byte length prefix), but tv.value = types.LongOctetString(packet) serialises with a two-byte prefix: types.LongOctetString(b"abc").serialize() == b"\x03\x00abc". For the three-entry example the frame becomes 41 27 00 05 15 08 00 08 c8 20 …, so a receiver honouring the declared octstr reads length 0x27 = 39 and consumes 00 05 15 08 00 08 c8 20 plus only 31 of the 32 ASCII characters — the final character is dropped, and one byte dangles past the record. The accidental leading 00 from the high length byte is also what makes the current header "work" at all, since header is one byte short of the 3+4 prefix the framing needs. Please use types.LVBytes (or DataType.from_python_type(...) off the declared feeder_attr type) so the type and value agree, and stop relying on that byte.
3. Clearing the schedule silently does nothing on the device but reports success. When the written value is blank, the code sets the cached attribute to [] and returns a SUCCESS status record without sending anything to the feeder. The user sees an emptied Schedule sensor while the device keeps its old schedule and keeps dispensing food. Z2M's converter does send a clear for an empty list. Either send a real clear frame or fail the write.
Must-address
Duplicate and mutually-inconsistent zha_events. OppleCluster now inherits EventableCluster, which already emits attribute_updated for every attribute update, while _parse_feeder_attribute() and write_attributes() also fire explicit ZHA_SEND_EVENT calls for the same attributes. I replayed a real feeding report (1c 5f 11 7d 0a f1 ff 41 0c 00 05 d0 04 15 02 bc 04 30 32 30 33) against the built quirk: it produces four events for last_feeding_source — two carrying <FeedingSource.HomeAssistant: 2> and two carrying the string "HomeAssistant" — and four for last_feeding_size. Same attribute_id, different value types, in one report. An automation trigger on that event cannot be written reliably. Please pick one emitter: either drop EventableCluster and keep the explicit events, or drop the explicit listener_event calls and let EventableCluster handle it.
The Schedule sensor's state can exceed Home Assistant's 255-character state limit. _parse_schedule() publishes compact JSON, which is 266 characters for five workdays entries and 276 for five everyday entries with two-digit hour/minute — over MAX_LENGTH_STATE_STATE (255) in HA core, so HA rejects the state and the sensor goes stale. Five entries is exactly the maximum _encode_schedule() allows, so this is reachable through the documented interface. Consider a more compact representation (or moving the schedule out of the state and into attributes).
The optimistic cache update ignores the write result. In write_attributes() the cache update and the zha_event both fire before _write_attributes() is awaited, and the returned status is never inspected. A rejected write still leaves HA showing the new schedule, which also means the screenshot in the PR description does not actually demonstrate that the feeder accepted the frame. Please update the cache only after a successful status (or let the device's own report drive it, since _parse_schedule() already handles the echo).
Schedule days are silently coerced or silently dropped. _encode_schedule() uses DAYS_MAP.get(days, 0x7F), so a typo like "weekdays" or "Everyday" becomes everyday — for a device that dispenses food, a silent widening is the wrong default; please reject unknown values. On the read side _parse_schedule() keeps an entry only when DAYS_REVERSE_MAP.get(days_mask) resolves, so any mask the map does not know is dropped without trace and the sensor under-reports the real schedule. Z2M's feederDaysLookup additionally carries 85 (mon-wed-fri-sun) and 42 (tue-thu-sat), and the device can presumably emit arbitrary bitmasks; consider falling back to the raw mask instead of discarding the entry.
Tests do not constrain the encoding, and the validation path is untested. test_aqara_feeder_write_schedule computes its expectation with expected_bytes = opple_cluster._encode_schedule(schedule_str) — the encoder is compared against itself, so it passes for any header or length byte, including the two above. The sibling test_aqara_feeder_write_attrs already does this the right way with literal expected byte strings; please assert literal bytes here too, and cover at least one, two, four and five-entry schedules. Codecov reports 77.4% patch coverage; running pytest tests/test_xiaomi.py --cov=zhaquirks.xiaomi.aqara.feeder_acn001 --cov-report=term-missing locally shows the uncovered block is essentially all of _encode_schedule's validation and error handling (lines 435-497) — the only guard against writing a bad schedule to a pet feeder.
Diagnostics. The checklist ticks "Device diagnostics data has been attached", but the Device diagnostics section of the PR body is empty, and there is no aqara.feeder.acn001 snapshot in ZHA's tests/data/devices/ (I checked all 873 files). That means these entity changes have zero CI coverage on the ZHA side. Please attach a fresh diagnostics download (taken after a re-pair or a Reconfigure, so the cached values are current) so a snapshot can be added.
A multi-attribute write containing schedule drops the other attributes. The schedule branch returns unconditionally, so write_attributes({"schedule": ..., "child_lock": True}) transmits only the schedule and silently discards the rest. ZHA writes one attribute at a time today, so this is latent rather than user-visible, but it is a cheap fix — handle the schedule and then fall through to the generic loop for whatever else was requested.
Magic numbers. tv.type = 0x41 should be the foundation.DataTypeId constant (or derived from the attribute definition), and the eleven unique_id_suffix="64704-…" literals would be safer and self-documenting as f"{OppleCluster.cluster_id}-<attr>" — they are load-bearing for entity continuity, so a typo in one of them silently orphans an entity.
Questions for the maintainers
Is a JSON blob in a synthetic ZCL CharacterString attribute, written through zha.set_zigbee_cluster_attribute, an acceptable interface? There is no prior art for it in this repo — feeder_acn001.py would be the only quirk that calls json.dumps. It gives users no HA entity to set the schedule with, no validation feedback in the UI, and a read-only sensor that mirrors it. This is a policy call rather than a code defect, but it is worth settling before the pattern gets copied.
Release ordering. This PR is coupled to zigpy/zha#883 (also a draft), which removes the native feeder entity classes. Until that lands the eleven v2 declarations do nothing, so the two need to be sequenced deliberately.
Notes and nits
.friendly_name(manufacturer="Aqara", model="aqara.feeder.acn001")keeping the raw model string is load-bearing: ZHA's native feeder entities match onmodels=frozenset({"aqara.feeder.acn001"})against the resolved model, so changing this to a marketing name would immediately unmatch them. Worth a short comment so nobody "improves" it later. (Conversely, if you ever wanted the v2 entities to take effect without zigpy/zha#883, that is the lever — probably too subtle to rely on.)- The PR title reads better with a leading verb, matching the repo's changelog-line convention: "Migrate Aqara pet feeder ACN001 to quirks v2 and add onboard scheduling".
FeederTimeCluster.handle_read_attribute_time()deliberately returns local time in thetimeattribute, which ZCL defines as UTC (zigpy's baseTimecluster already exposes correctly-adjustedlocal_time,standard_timeandtime_zone). Given Z2M does the same thing, this is presumably what the firmware needs — a one-line comment saying the device ignorestime_zonewould save the next reader the trip.- Nit:
isinstance(event.value, (bytes, types.LVBytes))—LVBytesis abytessubclass, so the second member is redundant. - Nit:
LOGGER.exception("Failed sanitizing event value: %s", exc)—.exception()already logs the traceback; the explicitexcargument is redundant. - Nit: the three broad
except Exception/contextlib.suppress(Exception)guards in the parse path could name the exceptions they actually expect. - Where does the five-entry cap in
_encode_schedule()come from? Z2M imposes no limit; if it is a device constraint, a comment citing it would help.
Verified (12 checks)
- Reviewed at head 87ef4c9; merge-base is current
dev(6a3822c), so no rebase was needed.pre-commit run --all-filesclean,pytest tests/test_xiaomi.py181 passed. - Confirmed each ZHA-native
_unique_id_suffixinzha/application/platforms/{switch,sensor,number,select,binary_sensor,button}.pyagainst the quirk'sunique_id_suffixvalues. - Read
Device._add_pending_entities()inzha/zigbee/device.py— duplicate(platform, unique_id)keys are dropped, andQuirkV2Device.discover_entities()yields native ZCL entities before quirk v2 ones. types.LongOctetString(b"abc").serialize()isb"\x03\x00abc";DataTypeId(0x41)isoctstrandDataTypeId(0x43)isoctstr16.- Compared the encoder against Z2M's
lumi_feederconvertSetinsrc/lib/tuya-adjacentsrc/lib/lumi.ts(sendAttruses00 02 <seq>+ int32BE attr + uint8 length + value, and appends a trailing0x00to the schedule payload). - Replayed a feeding report through the built quirk to count the emitted
zha_send_eventcalls (four per attribute, two value shapes). - Measured the compact-JSON schedule length for three, four and five entries (166 / 221 / 276 characters) against
MAX_LENGTH_STATE_STATE = 255in HA core. - Confirmed
attribute_initialized_from_cachedefaults toTrue, so the newschedulesensor is not read on startup and will not be marked unsupported. - Confirmed
EventableCluster._handle_attribute_reportis a real base method, so the quirk's override is live rather than dead code. - Checked zigpy's
Timecluster:handle_read_attribute_timeis a real hook, so the local-time override is wired correctly. - Swept every open PR's file list via the pulls files API: no other open PR touches
zhaquirks/xiaomi/aqara/feeder_acn001.py(the two that appear are stale-base artifacts), and #4499 explicitly excludes this file. - Second opinion from Copilot (GPT-5.6 Sol): it independently reached six of the findings above — the type/value mismatch, the hard-coded length byte, the dropped day masks, the 255-character state limit, the duplicate events and the premature cache update — and contributed the multi-attribute-write one. The first attempt with GPT-5.5 failed with an API error, so the run was repeated on Sol.
| parts.append(f"{days_mask:02X}{hour:02X}{minute:02X}{portions:02X}00") | ||
| data = ",".join(parts) | ||
| header = bytes([0x05, 0x15, 0x08, 0x00, 0x08, 0xC8]) | ||
| return header + b" " + data.encode() |
There was a problem hiding this comment.
Blocker: b" " is 0x20 = 32, and it lands in the byte position the Aqara framing uses for the payload length. The joined ASCII payload is 11 * n - 1 bytes for n entries — 10, 21, 32, 43, 54 — so 32 is correct only for exactly three entries, which happens to be both the example in the PR description and the frame in test_aqara_feeder_schedule_sensor. Every other schedule size ships a wrong length.
The class already has _build_feeder_attribute(), which computes the length and emits a proper sequence byte; please route the schedule through it instead of hand-rolling a second framing:
payload = data.encode() + b"\x00"
return self._build_feeder_attribute(SCHEDULING_STRING, payload, len(payload))That also matches Z2M's lumi_feeder converter, which uses 00 02 <seq> + int32BE attribute id + uint8 length + value and appends a trailing 0x00 to the schedule payload.
| ) | ||
| tv = foundation.TypeValue() | ||
| tv.type = 0x41 | ||
| tv.value = types.LongOctetString(packet) |
There was a problem hiding this comment.
Blocker: the declared type and the serialised value disagree. tv.type = 0x41 on the line above is octstr, a single-byte length prefix, but LongOctetString writes a two-byte one — types.LongOctetString(b"abc").serialize() is b"\x03\x00abc".
For the three-entry example the record becomes 41 27 00 05 15 08 00 08 c8 20 …: a receiver honouring octstr reads length 0x27 = 39, consumes the eight header bytes plus only 31 of the 32 ASCII characters, and leaves one byte dangling past the record. The final schedule character is dropped on the wire.
The spurious leading 00 from the high length byte is also what currently supplies the first byte of the Aqara header, since header is one byte short of the 3+4 prefix. Please use types.LVBytes (matching the declared feeder_attr type) and derive the type id from the attribute definition rather than hard-coding 0x41.
| ] | ||
| ] | ||
| else: | ||
| self._update_attribute(ZCL_SCHEDULE, "[]") |
There was a problem hiding this comment.
Blocker: writing a blank schedule sets the cache to [] and returns SUCCESS without sending anything to the feeder. The user sees an emptied Schedule sensor while the device keeps its old schedule and keeps dispensing food — the worst failure direction for this device.
Z2M's converter does send a clear for an empty list (empty payload plus the trailing 0x00). Please either send a real clear frame or return a failure status so the user knows nothing happened.
| if schedule_val.strip(): | ||
| packet = self._encode_schedule(schedule_val) | ||
| if packet: | ||
| self._update_attribute(ZCL_SCHEDULE, schedule_val) |
There was a problem hiding this comment.
The cache update and the zha_event below both fire before _write_attributes() is awaited, and the returned status is never inspected — so a rejected or timed-out write still leaves HA showing the new schedule. This also means the screenshot in the PR description does not actually demonstrate that the feeder accepted the frame; it only shows this optimistic update.
Please update the cache after a successful status, or drop the optimistic update entirely and let the device's own report drive it (_parse_schedule() already handles the echo).
|
|
||
|
|
||
| class OppleCluster(XiaomiAqaraE1Cluster): | ||
| class OppleCluster(XiaomiAqaraE1Cluster, EventableCluster): |
There was a problem hiding this comment.
Adding EventableCluster here duplicates events, because _parse_feeder_attribute() and write_attributes() still fire their own explicit ZHA_SEND_EVENT calls for the same attributes.
I replayed a real feeding report (1c 5f 11 7d 0a f1 ff 41 0c 00 05 d0 04 15 02 bc 04 30 32 30 33) through the built quirk: it emits four attribute_updated events for last_feeding_source — two carrying <FeedingSource.HomeAssistant: 2> and two carrying the string "HomeAssistant" — and four for last_feeding_size. Same attribute_id, different value types, from one report; an automation trigger on that event can't be written reliably.
Please pick one emitter: keep EventableCluster and drop the explicit listener_event calls, or keep the explicit ones and drop the base class.
| ) | ||
| return | ||
| self._update_attribute( | ||
| ZCL_SCHEDULE, json.dumps(schedules, separators=(",", ":")) |
There was a problem hiding this comment.
This state can exceed Home Assistant's 255-character limit (MAX_LENGTH_STATE_STATE in HA core). Compact JSON for five entries is 266 characters for workdays and 276 for everyday with two-digit hour/minute, so HA rejects the state and the sensor goes stale. Five entries is exactly the maximum _encode_schedule() accepts, so it is reachable through the documented interface.
A more compact encoding (or moving the schedule off the state and onto entity attributes) would keep the maximum-size schedule representable.
| portions, | ||
| ) | ||
| return None | ||
| days_mask = DAYS_MAP.get(days, 0x7F) |
There was a problem hiding this comment.
DAYS_MAP.get(days, 0x7F) silently turns any unrecognised value — a typo like "weekdays", or "Everyday" with the wrong case — into everyday. For a device that dispenses food, silently widening the schedule is the wrong default; please reject unknown day names with an error return instead.
| minute = int(part[4:6], 16) | ||
| portions = int(part[6:8], 16) | ||
| day_name = DAYS_REVERSE_MAP.get(days_mask) | ||
| if day_name and hour < 24 and minute < 60 and portions > 0: |
There was a problem hiding this comment.
An entry whose mask isn't in DAYS_REVERSE_MAP is dropped without any trace, so the Schedule sensor under-reports what the feeder is actually doing (and shows nothing at all if every entry uses such a mask). Z2M's feederDaysLookup additionally carries 85 (mon-wed-fri-sun) and 42 (tue-thu-sat), and the device can presumably emit arbitrary bitmasks set from the Aqara app.
Consider falling back to the raw mask (or a derived day list) rather than discarding the entry, and logging when that happens.
| tv = foundation.TypeValue() | ||
| tv.type = 0x41 | ||
| tv.value = types.LongOctetString(packet) | ||
| return await self._write_attributes( |
There was a problem hiding this comment.
This returns unconditionally, so a write that also carries other attributes — write_attributes({"schedule": ..., "child_lock": True}) — silently discards everything except the schedule. ZHA writes one attribute at a time today, so it's latent rather than user-visible, but handling the schedule and then falling through to the generic loop below would close it cheaply.
|
|
||
| await opple_cluster.write_attributes({"schedule": schedule_str}) | ||
|
|
||
| expected_bytes = opple_cluster._encode_schedule(schedule_str) |
There was a problem hiding this comment.
This computes the expectation with the very function under test, so the assertion below holds for any header or length byte — including both of the encoding blockers flagged in _encode_schedule. The sibling test_aqara_feeder_write_attrs gets this right by parametrising literal expected byte strings; please do the same here, and cover one, two, four and five-entry schedules so the length byte is actually constrained.
Related: local pytest tests/test_xiaomi.py --cov=zhaquirks.xiaomi.aqara.feeder_acn001 --cov-report=term-missing shows lines 435-497 uncovered — essentially all of _encode_schedule's input validation, which is the only guard against writing a bad schedule to the feeder.
| .sensor( | ||
| attribute_name=OppleCluster.AttributeDefs.schedule.name, | ||
| cluster_id=OppleCluster.cluster_id, | ||
| entity_type=EntityType.DIAGNOSTIC, | ||
| unique_id_suffix="64704-schedule", | ||
| translation_key="schedule", | ||
| fallback_name="Schedule", | ||
| ) |
There was a problem hiding this comment.
I don't think HA Core allows us to have a sensor with JSON text. We can add that "fake" attribute, so it can be read/written via the cluster dialog (and service/action calls to set attributes) but I don't think we're allowed to expose such a schedule sensor entity as pure JSON.
There was a problem hiding this comment.
I think in the future we may have a separate "states/valeus" tab on that "Manage Zigbee device" page, somewhat similar to what Z-Wave has, where we can expose certain settings that do not fit nicely into HA entities, but still make them easier to change, compared to just raw ZCL/custom attributes.
There was a problem hiding this comment.
That would be cool. I can remove this. I just liked the idea of having the schedule available. In the meantime, a trigger-based template sensor helper can be created to parse the event data to track the schedule.
|
@TheJulianJES do you want me to open a separate PR for just the v2 migration? That should be relatively easy. That would also enable zigpy/zha#883. Then I can work on refactoring this separately, even in a new PR to keep it cleaner. There is a lot to address. 😅 |
|
I think a separate PR makes sense, yes. Thanks for working on this! |
|
v2 quirk PR: #5296 |
Proposed change
This PR updates the Aqara Pet Feeder ACN001 to a quirk V2. There are several improvements to the existing quirk.
The quirk v2:
FeedingSourceclass to includeScheduleand renamesRemotetoHomeAssistant.Additional information
Updating the onboard schedule can be done using the
zha.set_zigbee_cluster_attributelike this:When the schedule has been set successfully, it will update the

Schedulesensor. For example:This quirk v2 is also prepared for changes required in ZHA mentioned here:
zigpy/zha#705
PR for ZHA "legacy" quirks: zigpy/zha#883
After removing the entities from ZHA, when a feeding has been provided from the onboard schedule the

Last feeding sourcesensor will displayScheduleinstead ofundefined_0x00:Device diagnostics
Checklist
pre-commitchecks pass / the code has been formatted using Black