Repository navigation
Merge master into production (#minor) - #253
Merged
Merged
Conversation
Each Cluster kept its own transaction sequence number counter starting at 0, so every cluster on a node sent TSN 1, 2, ... and an app restart reused the TSNs it had just sent. Some devices (Aqara Radiator Thermostat E1) acknowledge a write whose TSN they recently saw on any of their clusters but do not apply it. Allocate TSNs from one counter per ZCLNode, shared by all endpoints and clusters, starting at a random value. A standalone Endpoint whose owner has no counter falls back to its own. Cluster.nextSeqNr() stays overridable. Refs #226
The stack can reject sendFrame (e.g. it gave up waiting for the APS ACK and retried) after the device already received the frame and sent its ZCL response. The command then failed even though the response proved delivery. Ignore the send failure once the response has been handled. Refs #226
Endpoint.handleFrame suppressed every reply to a frame with command id 0x0B without checking the frame type. Cluster-specific commands with id 0x0B (e.g. Door Lock setWeekDaySchedule) therefore got no cluster response, no success Default Response and no error Default Response. ZCL R8 2.5.12.2 only forbids answering another Default Response, which is global command 0x0B (frame type 0b00). Cluster-specific command ids are scoped per cluster (2.4.1.4), so 0x0B there is a different command.
handleFrame resolved a pending request with any incoming frame whose TSN matched. Devices number their own unsolicited frames (attribute reports, zone enroll requests, OTA queries) from their own counter, so 1 in 256 such frames was swallowed as the response: the request resolved with wrong args and the frame's own handler never ran. A TSN identifies a transaction (ZCL R8 2.4.1.3), and a response belongs to the command it answers: the declared response command, or a Default Response whose command identifier field names the sent command (2.5.12.1.2). Only those frames now resolve the pending request; any other frame is dispatched to its regular handler.
An unsupported command or a frame for a cluster the endpoint lacks was answered with a Default Response status of FAILURE (0x01). ZCL R8 2.5.12.2 requires UNSUP_COMMAND (0x81, named UNSUP_CLUSTER_COMMAND in the status enum) and UNSUPPORTED_CLUSTER (0xc3); FAILURE is only to be tolerated from devices certified before ZCL revision 6. R8 Table 2-12 deprecates 0x82-0x84 in favor of UNSUP_COMMAND, so the manufacturer specific and general cases use 0x81 too. Throw ZCLError with these statuses instead of plain Errors, so the existing ZCLError path in Endpoint.handleFrame encodes them. Handler errors still map to FAILURE.
The attr.<name> event now passes a second argument with the report meta data, including the ZCL transaction sequence number of the report frame. This allows devices to detect repeated deliveries of the same report.
Makes devices that answer with a Default Response for the wrong command, or an undeclared response command, visible in the debug log.
Endpoint now asks its owner for the next transaction sequence number when the owner provides one, so the type and its comment no longer hold that Endpoint only ever calls sendFrame.
The test is about whether a Default Response is sent, not which error status it carries, so it keeps passing once unsupported commands get UNSUP_COMMAND instead of FAILURE.
Endpoint.handleFrame only suppressed default responses for group-addressed frames. A command received via network broadcast or sent to the broadcast endpoint 0xFF still got a default response (success or error). ZCL R8 2.5.12.2 generates a Default Response only for a unicast command, and 2.3.2.1 says the broadcast endpoint follows that rule. Skip default responses when meta.broadcast is set (Z-Stack wasBroadcast) or meta.dstEndpoint is 0xFF. Cluster-specific responses are still sent.
ZCL R8 2.5.12.2 ties the Default Response to the frame being unicast, not to its destination endpoint, so an APS unicast to endpoint 0xFF still gets one. Only skip it when the stack flags the frame as a broadcast.
getAlarm had no response, so with response matching by command its Get Alarm Response (0x01) no longer resolved the call. Before, it only resolved by TSN, parsed as resetAllAlarms with no args. Status-dependent payload per ZCL R8 3.11.2.5.2.
Apps define commands without a declared response and await a cluster-specific reply with the request's TSN, e.g. Tuya datapoints sent with disableDefaultResponse. Matching by command id would time those out. For such commands any cluster-specific frame with the TSN still counts; reports and other global frames never do.
_addPrototypeMethods defaults a response without a numeric id to the command id. The trx handler recorded undefined instead, which switched such commands to the any-cluster-specific-reply fallback and let an unrelated frame with the same TSN resolve them.
Name the Default Response and frame type checks and split the declared and undeclared response cases into an if/else. No behavior change.
Keep the match fields next to a handle function instead of attaching them to the handler with Object.assign. No behavior change.
A cluster-specific frame carrying a global command's response id, and a Default Response naming another command, must not resolve the pending command.
fix: Share one random-start TSN counter per node
…-type fix: Only skip responses for global Default Response frames
fix: Send UNSUP_COMMAND and UNSUPPORTED_CLUSTER in Default Response
…cast fix: Skip default responses to broadcast frames
…-by-command # Conflicts: # test/testClusterResponseTimeout.js
fix: Match pending responses by command, not only by TSN
Pass a whitelisted object instead of spreading the stack's frame meta, so stack fields do not become part of the public event API, and use the spec name transactionSequenceNumber. Document that the value wraps and can be fixed per device, so it only marks duplicates within a short window.
…umber Emit the transaction sequence number with attribute reports
Run npm update across the dev dependency tree and bump jsdoc to 4, jsdoc-ts-utils to 6 and mocha to 12. This takes npm audit from 49 findings to 19 and removes the only critical one. The jsdoc major is not optional here. On jsdoc 3 the refreshed tree pulls @types/linkify-it 5 next to @types/markdown-it 12, which makes the tsc --noEmit index.d.ts check in CI fail. jsdoc 4 drops that @types chain entirely. Nothing here reaches consumers: the runtime dependencies and the published lib/ and index.d.ts are untouched. npm writes the lockfile as version 3, which is what the bundled npm on the Node version in .nvmrc produces. Still open and deliberately left out: eslint 6 to 10 needs the flat config migration, and sinon 19 to 22 breaks the type check because @types/sinon 17 no longer matches its fake-timers types.
build(deps): Update dev dependencies to clear audit findings
Both @athombv packages this repo depends on, data-types and jsdoc-template, are published to npmjs.org. Most other @athombv packages are published to GitHub Packages instead, so Athom machines commonly set @athombv:registry to npm.pkg.github.com in their user config. npm resolves a registry per scope, not per package, so that user config sends these two to GitHub Packages as well, where jsdoc-template only goes up to 1.6.3. npm install and npm update then fail with ETARGET on jsdoc-template@^1.7.0. Only npm ci keeps working, because the lockfile carries a full tarball URL per package. Pinning the scope here overrides that for this repo. Both packages are public on npmjs, so this needs no credentials and changes nothing for CI, which already defaults to npmjs.
// Mandatory, // Optional and // Conditional describe the attribute or command as a whole. On the opening brace Prettier pulls them inside it, where they read as if they belong to the id field. Above the key they survive the formatter and say what they mean. 77 annotations across five cluster files, no other change.
Pure npm run format:write output, except for the prettier-ignore guard on the enum8Status table in lib/zclTypes.js, which keeps its column alignment.
The annotation moves above the key, which is both what it means and what survives Prettier. Prettier also lowercases hex digits, so the id examples follow.
The format:check script was added alongside Prettier but nothing ran it, so the formatting was advisory: one commit formatted by hand and the next npm run format:write is a hundred-file diff again, which undoes the point of the reformat.
The three EventEmitter imports now read node:events. The prefix cannot be shadowed by a local events.js and says outright that the module is built in. Only lib/ here; the test files are converted alongside the node:test migration they belong to.
chore: Add Prettier and reformat
The generated docs are a static site that works straight from disk: no fetch, no XHR, no module scripts, only relative classic script tags. Checked over file:// in Chrome and Safari, with stylesheets, the sidebar filter, dark mode, syntax highlighting and deep links all working, and console output identical to serving it over http. So serve, watch and concurrently go without a replacement, along with build:clean and build:watch. Together they pulled in 148 transitive dependencies and 5 audit findings for a preview that never runs in CI; concurrently@5 is from 2020 and watch@1.0.2 from 2016. npm run docs builds and opens build/index.html with macOS open. AGENTS.md says how to view the docs elsewhere, and its lint line no longer refers to the eslint-config-athom setup that #244 removed.
node:test covers describe/it/hooks and the mock API covers the four sinon sandbox uses (spy, stub, callsFake, restore), so both packages and @types/sinon can go. Two things node:test does differently: - A callback test must take the context first, function(t, done). With function(done) node:test treats the test as synchronous, passes it and never runs the assertions. All 22 callback tests are updated. - There is no default test timeout, so a done that never fires hangs instead of failing. --test-timeout=5000 restores mocha's safety net. The one test that waited on a real 150ms timer now uses mock.timers.
The test files already require `node:test` on the line above, so the unprefixed `assert` next to it was the odd one out.
The repository supports Node 22 and up, but every workflow ran Node 24 through .nvmrc, so the supported floor was never the version anything was actually checked against. Verified on Node 22.23.4, the current 22.x, at the tip of the open stack: npm ci, 114 tests pass, lint, format:check, the docs build, generate-types with an unchanged index.d.ts, and tsc all succeed. That includes the native type stripping #243 relies on, which needs Node 22.18 or newer; `22` in .nvmrc resolves to the latest 22.x. engines.node stays ">=22": consumers are not restricted to 22, this only fixes what the project itself develops and tests against.
Four unrelated bits of workflow upkeep. Pushes to `testing` ran neither Lint nor Test, while deploy.yml publishes a beta from that same branch. Both workflows now trigger on it. setup-node gets `cache: 'npm'` in the four workflows that lacked it; generate-types.yml already had it. They each re-resolved the whole tree on every run. actions/checkout and actions/setup-node go from v4 to v7, and peaceiris/actions-gh-pages from v3.8.0 to v4. The deploy.yml header still described a Node 12 environment; it has read the version from .nvmrc for a long time.
Two problems the Node 22 pin exposed in deploy.yml. Publishing uses OIDC trusted publishing with --provenance and carries no NPM_AUTH_TOKEN, and that needs npm 11.5.1 or newer. Node 22 bundles npm 10.9.9, and setup-node installs whatever npm ships with the Node version it selects, so both the production and the beta publish would have failed authentication, after the version bump and tags had already been pushed. npm is now upgraded explicitly. npm 11 needs Node >=22.9.0 and `22` in .nvmrc resolves well past that. Adding `testing` to the Lint and Test workflows made the checks run on that branch but did not hold anything back: deploy.yml starts on the same push and neither waits for them nor repeats them, so a `#patch` push could still publish a beta before they finished. The deploy job now runs lint, format:check and the tests itself, before the version bump, so a failure stops the publish instead of racing it.
The docs build is the slowest step in the job and nothing about publishing depends on it, so running lint, format:check and the tests first means a failing check stops the job before that minute is spent. Build and the publish steps that follow are unchanged.
chore: Drop the docs server in favor of opening the files
chore: Replace mocha and sinon with node:test
ci: Pin to Node 22 and tidy the workflows
AGENTS.md now lists format:check, format:write and generate-types next to the other commands. Lint in CI runs format:check, so a change that skips format:write fails there, and the file did not say the command exists. docs.yml asked to add .github to .npmignore. There is no .npmignore and none is needed: files in package.json is an allowlist of index.d.ts and lib, so nothing else is packed.
docs: List the format and type commands, drop a stale npmignore note
Publishing put a version on npm and a tag in the repository, but no GitHub release, so there was nowhere to read what changed. The deploy job now creates one once npm has accepted the version, from GitHub's generated notes. It starts from the previous release rather than the previous tag, because a publish that fails after the version bump leaves a tag behind without a release, and starting from that tag would leave its changes out of the next notes. .github/release.yml groups the notes into breaking changes, new features, fixes and other changes, and leaves internal work out. The groups key off labels, which label-pr.yml sets from the conventional commit prefix of each pull request title: feat, fix and perf, a breaking marker, and build, chore, ci, docs, refactor, style and test as internal. Titles without a prefix land under other changes, so outside contributions stay visible. The release pull request into production is labeled too and excluded. The notes keep each pull request title as is, so the prefix is stripped from every line: the heading above it already says it.
The deploy workflow published betas from a testing branch, and Lint, Test and Generate TypeScript Types listed it as a trigger. That branch no longer exists and npm has no beta dist-tag, so the triggers, the job condition and the beta publish step go. The deploy job now runs on production only, which also makes the per-step production checks on Publish and on the release step redundant. Publish also stops writing package_version to the job environment; nothing read it.
Without an explicit shell, Actions runs a step with bash -e but no pipefail. When the generate-notes call failed, the Node filter after it still exited cleanly on empty input, so the step published a release with empty notes and reported success. That release then became the starting point for the next release's notes, which lost these changes too. shell: bash adds pipefail, so the step now stops instead.
The labeler only reacted to title edits, so retargeting a pull request into or out of production kept its old label: a feature moved into production was not marked as a release, and one moved out stayed excluded from the notes. It now also reacts to base branch edits. A title that lost its conventional prefix kept the label the prefix had set, so a chore that became a user-facing change stayed hidden. That label is now removed. Labels a maintainer set by hand on a pull request without a prefix are still left alone.
Until the first GitHub release exists, the release step fell back to the nearest earlier tag. A publish that fails after the version bump leaves such a tag behind without an npm version, and this repository has three of them. For v3.1.1 the fallback picked v3.1.0, an orphan, so the notes would have left out everything between v3.0.2 and v3.1.0. The fallback now asks npm for the highest version below the one being released, which only counts versions that were really published.
The event payload holds the labels as they were when the event fired. A pull request opened as chore: and retitled to feat: before the first run finished produced two runs that both saw no labels: the first added internal, the second added feature without removing internal, and since internal is excluded from the notes, the feature disappeared from them. Each run now reads the pull request as it is when the run starts, and runs for the same pull request are serialized, so a later run sees the labels an earlier one added.
The README's release section was a single line. It now covers the master to production pull request with its version marker, why it has to be merged with a merge commit or a squash merge, what the Deploy workflow then does, and how the release notes are grouped by the conventional commit prefix of each pull request title. A Changelog section near the top points users to the GitHub releases. The contributing notes mention Prettier and the pull request title convention, and AGENTS.md states that convention and links to the release section, since the title now decides where a change appears.
It named scripts/generate-types.js, which is generate-types.mts, and said types regenerate on pushes to develop, a branch this repository does not have. The workflow runs on any branch other than production, for a wider set of paths than listed, and commits back to that branch.
The labeler kept a label set by hand on a pull request without a conventional prefix, and to tell that apart from one it had set itself, it worked out the label the previous title implied. When runs overtook each other that went wrong: a pull request opened as chore: and retitled twice within one run could keep internal, and with it drop out of the release notes. The five managed labels now follow the current title and base branch and nothing else, so the last run always leaves them right, whichever runs before it were skipped. A title without a prefix gets none of them. To move a pull request to another section, retitle it, which is what the README already said. Other labels are left alone.
ci: Create a GitHub release on publish and drop the testing branch
RobinBol
added
the
release
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.