Conversation
|
I think it's good to have a |
|
The consistency with bridge makes a lot of sense. I've worked through the changes and have a few questions - happy to go with whatever approach you prefer. 1. Correlation field naming Nerivec previously flagged that CSM-300ZB has a "transaction" device attribute (0-1000ms enum). If we use flat "transaction" for correlation, those users can't set their device attribute AND use request-response simultaneously - we'd have to strip it before forwarding, breaking their device config. Options:
Preference? 2. Response metadata Bridge responses are minimal. For device commands, I've added fields that frontends/automation can use for better sleepy-device handling and performance monitoring:
these can be wrapped in the z2m structure (item 1 above) or flattened like z2m_transaction_id, z2m_status, z2m_type, etc. Should we keep these, or go minimal to match bridge exactly for now (foregoing some better frontend UX) 3. Request topic (optional) We could add {device}/request/set as an alternative to {device}/set (and similar for get) for clients that want explicit request-response semantics. This would mirror bridge exactly, but introduce extra messages and the slow/painful migration for clients to deprecate {device}/[set|get]/ and replace with {device}/request/[get|set]. Probably unnecessary, but just wanted to check if you wanted to introduce that as well. (FYI, Ecosystem impact: I checked Home Assistant and windfront - neither will break. It seems that HA uses specific topics from discovery (not wildcards on device topics). Windfront uses WebSocket with the backend forwarding all MQTT messages. Both approaches just add new topics; existing /set and /get behavior unchanged.) Let me know your thoughts and I'll adjust accordingly. |
25d57ea to
210e118
Compare
| // Used by `publish.test.ts` to reload regex when changing `mqtt.base_topic`. | ||
| export const loadTopicGetSetRegex = (): void => { | ||
| topicGetSetRegex = new RegExp(`^${settings.get().mqtt.base_topic}/(?!bridge)(.+?)/(get|set)(?:/(.+))?$`); | ||
| topicGetSetRegex = new RegExp(`^${settings.get().mqtt.base_topic}/(?!bridge)(.+?)/(request/)?(get|set)(?:/(.+))?$`); |
There was a problem hiding this comment.
The (request/)? group is optional (?), so this regex still matches legacy /set and /get topics exactly as before. The new capture group shifts subsequent group indices (match[2] → type, match[3] → isRequest, match[4] → attribute). Backwards compatibility is verified by all existing tests passing unchanged, plus an explicit "Should NOT publish response for legacy /set topic" test.
| return; | ||
| } | ||
|
|
||
| // Extract and strip z2m_transaction before forwarding to converters |
There was a problem hiding this comment.
We strip z2m_transaction from the message before forwarding to converters. This is necessary because the CSM-300ZB device (shinasystem.js) has a real device attribute called transaction — if we'd used that name, the converter would consume it as a device setting. We use z2m_transaction to avoid the collision, and strip it here so converters never see it.
| delete message.z2m_transaction; | ||
| } | ||
|
|
||
| // Ping: /request/ topic with empty payload after stripping z2m_transaction |
There was a problem hiding this comment.
Ping pattern: after stripping z2m_transaction, if the payload is empty ({}), we short-circuit with an immediate {data: {}, status: "ok"} response. This lets clients verify the bridge is responsive without generating any Zigbee traffic. Useful for health checks and connection validation.
| // biome-ignore lint/style/noNonNullAssertion: always Error | ||
| logger.debug((error as Error).stack!); | ||
| if (parsedTopic.isRequest) { | ||
| ((error as Error).message?.includes("Request superseded") ? supersededKeys : failedKeys).push(originalKey); |
There was a problem hiding this comment.
Herdsman ≥9.0.5 throws "Request superseded" when a queued command for a sleepy device is replaced by a newer one. We distinguish this from generic failures so clients can tell the difference between "your command was replaced" vs "your command failed." Both map to status: "error" but with different error string prefixes (superseded: vs failed:).
| if (!this.publishedTopics.has(topic)) { | ||
| logger.debug(() => `Received MQTT message on '${topic}' with data '${message.toString()}'`, NS); | ||
| this.eventBus.emitMQTTMessage({topic, message: message.toString()}); | ||
| this.eventBus.emitMQTTMessage({topic, message: message.toString(), qos: /* v8 ignore next */ packet?.qos ?? 0}); |
There was a problem hiding this comment.
Propagates the MQTT QoS level from the incoming packet into the event system, so the response can be published at the same QoS as the request. Previously QoS was discarded at the event boundary. The ?? 0 fallback handles edge cases where packet is undefined (e.g. WebSocket messages via frontend.ts).
| await flushPromises(); | ||
| expect(mockLogger.error).toHaveBeenCalledWith("Entity 'an_unknown_entity' is unknown"); | ||
| }); | ||
|
|
There was a problem hiding this comment.
13 tests covering all transaction response branches: success with/without z2m_transaction, converter failure, superseded error, legacy topic (no response), ping pattern, QoS matching, group commands, attribute-in-topic, endpoint-in-topic, GET response (status only, no data), partial success, and mixed superseded+failed outcomes.
|
@Koenkk, now implemented and rebased onto Quick summary of what's in the updated PR:
I've added inline comments on the key design decisions. Total diff is +187/-10 across 6 files with 13 new tests. Let me know if anything needs adjusting. |
|
Nice! I need to do a in-depth review, @Nerivec do you think this is also useful for the frontend? (to get better error messages when changing stuff through the exposes tab?) |
|
This pull request is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days |
|
Do not stale. ☝️ |
|
Is there something i could help out, to speed up verification of this PR? |
|
This pull request is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days |
Adds request/response support for device commands, mirroring the existing bridge pattern. Clients send to /request/set or /request/get and receive structured responses on /response/set or /response/get. SET responses echo the requested values. GET responses return status only (no data) — actual values arrive on the state topic to avoid a race condition between convertGet() resolving and the cache update. Includes z2m_transaction correlation, superseded command detection, QoS propagation, and ping support.
|
Not stale — still keen to get this in. Rebased onto current For anyone wanting to test end-to-end, the windfront side is kept rebased on current windfront
@Koenkk happy to make any adjustments from the review whenever you get a chance. |
| await flushPromises(); | ||
| expect(mockMQTTPublishAsync).toHaveBeenCalledWith( | ||
| "zigbee2mqtt/bulb_color/response/set", | ||
| stringify({data: {}, error: "superseded:brightness", status: "error", z2m_transaction: "tx3"}), |
There was a problem hiding this comment.
I think we should just put a human readable message here, and next to that add e.g. error_details: {"brightness": "superseded"}
There was a problem hiding this comment.
Changed in 63cc4f6: error is now a human-readable message and error_details maps each failed attribute to its original error message, e.g.
{"data": {}, "status": "error", "error": "Failed to set 'brightness': Request superseded", "error_details": {"brightness": "Request superseded"}}I went with the original message as the error_details value rather than a category word like superseded, since that keeps the backend free of any string matching on herdsman error text and still lets a client detect superseded by looking at the per-attribute message. Happy to switch to a fixed category if you'd rather have that.
While testing this I also found that attributes handled by a converter that already ran for an earlier key (e.g. brightness + color_temp on a light) were dropped from the response entirely; they now inherit that call's outcome in data or error_details.
| await flushPromises(); | ||
| expect(mockMQTTPublishAsync).toHaveBeenCalledWith( | ||
| "zigbee2mqtt/bulb_color/response/set", | ||
| stringify({data: {}, error: "failed:brightness", status: "error", z2m_transaction: "tx2"}), |
There was a problem hiding this comment.
| stringify({data: {}, error: "failed:brightness", status: "error", z2m_transaction: "tx2"}), | |
| stringify({data: {}, error: "failed:brightness", status: "error", "transaction: "tx2"}), |
To make it the same as the bridge API
There was a problem hiding this comment.
Happy to rename this to transaction to match the bridge. One thing I want to double-check before doing it: as @Nerivec flagged earlier (and my question 1 above), the CSM-300ZB (shinasystem) has a real device attribute called transaction, which is why I went with the z2m_transaction prefix.
If we use plain transaction, my plan would be to strip it from the payload before it reaches the converters, but only on /request/set and /request/get. Legacy /set would still pass transaction through to the device, so CSM-300ZB owners keep the attribute there; they just can't set it via the request/response topics.
Is that trade-off OK with you, or would you prefer a different name that can't collide (e.g. transaction_id)? Once the name is settled, the response building can reuse utils.getResponse as you suggested originally.
| logger.error(message); | ||
| // biome-ignore lint/style/noNonNullAssertion: always Error | ||
| logger.debug((error as Error).stack!); | ||
| if (parsedTopic.isRequest) { |
There was a problem hiding this comment.
Would just keep the original error message here
There was a problem hiding this comment.
Done in 63cc4f6: the Request superseded string match is gone and the original error message is passed straight through, same as the bridge does in its catch block.
… responses Review feedback from Koenkk on Koenkk#30774: - `error` is now a human-readable message built from the original converter/herdsman error messages instead of the packed `superseded:a|failed:b` format. - New `error_details` field maps each failed attribute to its original error message, so clients no longer need to parse `error`. - Drop the string match on "Request superseded"; the original message is passed through unchanged, as the bridge API does. - Keys handled by a converter that already ran for an earlier key in the same message (e.g. brightness + color_temp on a light) now inherit that call's outcome instead of silently disappearing from the response.
Closes #30679
What
Adds request/response support for device commands, mirroring the existing bridge pattern (docs).
zigbee2mqtt/FRIENDLY_NAME/request/set→ response onzigbee2mqtt/FRIENDLY_NAME/response/setzigbee2mqtt/FRIENDLY_NAME/request/get→ response onzigbee2mqtt/FRIENDLY_NAME/response/get/setand/gettopics are unchanged — fully backward compatibleResponse shape
/response/set— echoes requested valuesSuccess:
{"data": {"state": "ON", "brightness": 200}, "status": "ok", "z2m_transaction": "my-id"}Error (
erroris human-readable;error_detailsmaps each failed attribute to the original error message):{"data": {}, "status": "error", "error": "Failed to set 'brightness': Request superseded", "error_details": {"brightness": "Request superseded"}, "z2m_transaction": "my-id"}Partial success (attributes that were accepted are still echoed in
data):{"data": {"color_temp_startup": 300}, "status": "error", "error": "Failed to set 'brightness': Request superseded", "error_details": {"brightness": "Request superseded"}, "z2m_transaction": "my-id"}dataechoes the values from the request, not from the device or cache. It confirms what was accepted, not current device state./response/get— status only, no data{"data": {}, "status": "ok", "z2m_transaction": "my-id"}GET responses deliberately omit data values. Actual device values arrive on the state topic (
zigbee2mqtt/FRIENDLY_NAME), which is the authoritative source. This avoids a race condition where the state cache may not yet be updated when the response is built.Common fields
z2m_transaction— optional, echoed back if provided in requesterror— human-readable summary, same style as the bridge APIerror_details—{attribute: original error message}for every failed attribute; original messages are passed through unchanged (e.g. herdsman'sRequest supersededwhen a newer command replaced a queued one on a sleepy device)retain: false(responses are events, not state){"z2m_transaction": "ping1"}(or{}) to/request/set→{"data": {}, "status": "ok"}Why
z2m_transactioninstead oftransactionThe bridge uses
transaction. We usez2m_transactionbecause CSM-300ZB has a real device attribute calledtransaction(shinasystem.js). A flattransactionfield would collide.Diff
Test plan
Frontend branches (windfront)
The earlier windfront PR (Nerivec/zigbee2mqtt-windfront#409) was closed as stale while this backend PR awaits review. A windfront PR will be opened once this is merged. Until then, both branches are kept rebased on current windfront
mainand can be used to test against this PR:Testing together
<device>/response/setand<device>/response/gettransaction-response-apibranch above — consumes responses to show real-time command status