Skip to content

fix: Preserve genBasic attributes when re-read returns undefined#1753

Merged
Koenkk merged 5 commits intoKoenkk:masterfrom
rohankapoorcom:fix/preserve-genbasic-on-undefined-read
May 8, 2026
Merged

fix: Preserve genBasic attributes when re-read returns undefined#1753
Koenkk merged 5 commits intoKoenkk:masterfrom
rohankapoorcom:fix/preserve-genbasic-on-undefined-read

Conversation

@rohankapoorcom
Copy link
Copy Markdown
Contributor

Summary

Device.updateGenBasic previously called Object.assign(this.#genBasic, data), which overwrites known values with undefined when the source object contains the key. That happens whenever a readRsp record comes back with a non-success status (e.g. UNSUPPORTED_ATTRIBUTE): the readRsp parser leaves attrData undefined, ZclFrameConverter.attributeKeyValue still sets payload[attribute.name] = undefined, and the downstream updateGenBasic then erases the existing value.

This change makes updateGenBasic skip undefined values so an absent attribute can never wipe a previously known one. Successful reads with real values still overwrite as before.

Bug scenario (the trigger I hit)

  1. User clicks "Interview" in the zigbee2mqtt frontend.
  2. zigbee2mqtt calls device.zh.interview(true) (ignoreCache=true, introduced in Koenkk/zigbee2mqtt#23328).
  3. interviewInternal runs the genBasic re-read loop:
    for (const endpoint of this._endpoints) {
        ...
        if (endpoint.supportsInputCluster(\"genBasic\")) {
            for (const key of INTERVIEW_GENBASIC_ATTRIBUTES) {
                if (ignoreCache || !this.#genBasic[key]) {
                    ...
                    result = await endpoint.read(\"genBasic\", [key], {sendPolicy: \"immediate\"});
                    ...
                    this.updateGenBasic(result);
  4. On a multi-endpoint device (e.g. Inovelli VZM30/31/32/35/36), every endpoint advertises genBasic per the ZCL spec but only endpoint 1 has a meaningful swBuildId. Endpoint 2/3 reply to swBuildId reads with UNSUPPORTED_ATTRIBUTE.
  5. The endpoint-1 swBuildId is set successfully, then the endpoint-2 read returns { swBuildId: undefined }, and Object.assign wipes the value.
  6. After the interview, device.softwareBuildID is undefined. In zigbee-herdsman-converters this breaks the Inovelli dynamicExposes/configure path (src/lib/inovelli.ts:656,673), which uses softwareBuildID to compute firmware-gated exposes — configure then chunkedReads attributes the firmware doesn't actually support.

Root cause

Two pre-existing facts combined to make the bug visible:

  1. The genBasic interview loop walks every endpoint that advertises genBasic (always has).
  2. attributeKeyValue propagates undefined for non-success records (because the readRsp parser leaves attrData undefined when status !== SUCCESS):
    // src/zspec/zcl/definition/foundation.ts (readRsp.parse)
    if (status === Status.SUCCESS) {
        const dataType = buffalo.readUInt8();
        rec.dataType = dataType;
        rec.attrData = buffalo.read(...);
    }
    // src/controller/helpers/zclFrameConverter.ts (attributeKeyValue)
    const attrData = Zcl.Utils.processAttributePostRead(attribute, item.attrData);
    payload[attribute.name] = attrData; // undefined when non-success

updateGenBasic is the chokepoint where every genBasic mutation funnels through (interview reads, Tuya quick-reads, and runtime readRsp/attributeReport in controller.ts). Fixing it here covers all paths and there's no scenario where overwriting a known value with undefined is desirable.

Change

public updateGenBasic(data: TPartialClusterAttributes<\"genBasic\">): void {
    // Skip `undefined` values so a non-success `readRsp` record (e.g. UNSUPPORTED_ATTRIBUTE on a
    // secondary endpoint) cannot wipe out a previously known value from another endpoint.
    for (const key in data) {
        const value = data[key as keyof typeof data];
        if (value !== undefined) {
            // biome-ignore lint/suspicious/noExplicitAny: indexed write into typed partial
            (this.#genBasic as any)[key] = value;
        }
    }
}

Regression test

test/device.test.ts now pins the contract: a real value is set, a follow-up call with undefined for the same keys does not clobber, and a third call with new real values still overwrites normally.

Validation

  • pnpm run build (tsc) — clean
  • pnpm run check (biome) — clean
  • pnpm test — 1740 passed, 1 skipped (was 1739/1)

Test image (zigbee2mqtt + this ZH branch)

For users who want to verify the fix end-to-end, a custom z2m Docker image bundling this branch is available:

ghcr.io/rohankapoorcom/zigbee2mqtt-custom:rohankapoorcom.zigbee-herdsman-converters-e27e6907__rohankapoorcom.zigbee-herdsman.fix.preser-32814609-20260507-061748

Repro:

  1. Run zigbee2mqtt with the image above and an Inovelli VZM30/31/32/35/36 paired.
  2. From the frontend, trigger "Interview".
  3. Before the fix: the device's "Firmware ID" (softwareBuildID) becomes empty and the Inovelli converter loses firmware-gated exposes / errors during configure.
  4. With this fix: softwareBuildID is preserved and the converter behaves as before.

Test plan

  • CI passes (lint, build, vitest).
  • Manual re-interview of an Inovelli VZM31-SN (or any other multi-endpoint device) leaves softwareBuildID intact.
  • Verify with the test image that the Inovelli converter no longer breaks after a re-interview.

Made with Cursor

`Device.updateGenBasic` used `Object.assign`, which overwrote known
values with `undefined` when a `readRsp` record came back with a
non-success status (e.g. `UNSUPPORTED_ATTRIBUTE`). During an
`interview(true)` re-read, the per-endpoint loop in `interviewInternal`
walks every endpoint that advertises `genBasic`; on multi-endpoint
devices like Inovelli VZM3x, secondary endpoints reply with
unsupported for `swBuildId`, which then wiped the value just read
successfully from endpoint 1. This in turn broke firmware-gated
exposes/configure in zigbee-herdsman-converters.

Skip `undefined` values in `updateGenBasic` so an absent attribute can
never erase a previously known one. Successful reads with real values
still overwrite as before.

Co-authored-by: Cursor <cursoragent@cursor.com>
@Nerivec
Copy link
Copy Markdown
Collaborator

Nerivec commented May 7, 2026

Strange, smells like a regression...
For the non-success status entries, we should prevent these from being returned by

try {
const attrData = Zcl.Utils.processAttributePostRead(attribute, item.attrData);
payload[attribute.name] = attrData;
} catch (error) {
logger.debug(`Ignoring attribute ${attribute.name} from response: ${error}`, NS);
}

wrap the whole try/catch with something like if (!("status" in item) || item.status === Zcl.Status.SUCCESS).
Which would be proper spec behavior (not supposed to assume any value from non-success response).
@Koenkk that would be fine with converters, right?

@Koenkk
Copy link
Copy Markdown
Owner

Koenkk commented May 7, 2026

wrap the whole try/catch with something like if (!("status" in item) || item.status === Zcl.Status.SUCCESS).
Which would be proper spec behavior (not supposed to assume any value from non-success response).

I think this is the proper solution indeed. @rohankapoorcom can you check if that works for this particular device?

rohankapoorcom and others added 3 commits May 7, 2026 11:38
…ned"

This reverts the `updateGenBasic` change in favor of a fix in
`ZclFrameConverter.attributeKeyValue` (per maintainer feedback in Koenkk#1753):
non-success `readRsp` records should be dropped at the converter rather
than masked at the genBasic chokepoint, matching ZCL spec behavior and
preventing the same class of bug for any cluster.

Co-authored-by: Cursor <cursoragent@cursor.com>
Per ZCL spec, a `readRsp` record with `status != SUCCESS` carries no
`dataType`/`attrData`. Previously `attributeKeyValue` would still set
`payload[attribute.name] = undefined` for such records, which then
flowed into `Device.updateGenBasic` (and `saveClusterAttributeKeyValue`)
and could overwrite a previously known value.

This was visible on multi-endpoint devices like Inovelli VZM3x: the
per-endpoint genBasic re-read in `interviewInternal` (triggered by
`device.zh.interview(true)` from the zigbee2mqtt frontend re-interview
path) succeeds on endpoint 1 for `swBuildId`, then endpoint 2 replies
with UNSUPPORTED_ATTRIBUTE, and the resulting `{swBuildId: undefined}`
clobbered the value via `Object.assign`. Downstream this broke
firmware-gated exposes/configure in zigbee-herdsman-converters.

Skip non-success records at the converter so the rest of the stack
never sees a synthetic value for them.

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@rohankapoorcom
Copy link
Copy Markdown
Contributor Author

Tested it on my end, the new fix seems to work fine!

Docker image: ghcr.io/rohankapoorcom/zigbee2mqtt-custom:rohankapoorcom.zigbee-herdsman-converters-e27e6907__rohankapoorcom.zigbee-herdsman.fix.preser-32814609-20260507-184611

@rohankapoorcom rohankapoorcom marked this pull request as ready for review May 7, 2026 19:10
Comment thread src/controller/helpers/zclFrameConverter.ts Outdated
Removes the debug log added in 1f28eb4 and the coverage-only test
record added in 3f8f777. The early `continue` for non-success records
is sufficient on its own; the log was over-instrumentation for what is
spec-defined behavior.

Co-authored-by: Cursor <cursoragent@cursor.com>
@Koenkk
Copy link
Copy Markdown
Owner

Koenkk commented May 8, 2026

Thanks!

@Koenkk Koenkk merged commit 9ab49a1 into Koenkk:master May 8, 2026
1 check passed
@rohankapoorcom rohankapoorcom deleted the fix/preserve-genbasic-on-undefined-read branch May 8, 2026 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants