Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support Z-Wave Long Range #6401

Merged
merged 45 commits into from
Jan 19, 2024

Conversation

jtbraun
Copy link
Contributor

@jtbraun jtbraun commented Oct 16, 2023

This PR adds support for Z-Wave Long Range

fixes: #5253
fixes: #6158

@jtbraun jtbraun changed the title Feature/longrange feat(longrange): support for long range controllers/devices Oct 16, 2023
@jtbraun jtbraun marked this pull request as ready for review October 16, 2023 02:03
Copy link
Contributor Author

@jtbraun jtbraun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a bunch of places where I could use some feedback, suggestions, etc.

Comment on lines 97 to 116
const nodeIdSize = (sendingNodeId < 256 && destination < 256) ? 1 : 2;
const ret = Buffer.allocUnsafe(2*nodeIdSize + 6 + unencryptedPayload.length);
let offset = 0;
if (nodeIdSize == 1) {
ret[offset++] = sendingNodeId;
ret[offset++] = destination;

} else {
ret.writeUint16BE(sendingNodeId, offset);
offset += 2;
ret.writeUint16BE(destination, offset);
offset += 2;
}

ret.writeUInt32BE(homeId, offset);
offset += 4;
ret.writeUInt16BE(commandLength, offset);
offset += 2;
// This includes the sequence number and all unencrypted extensions
unencryptedPayload.copy(ret, 8, 0);
unencryptedPayload.copy(ret, offset, 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If either nodeid is 256+, you have to use uint16BE to encode both.

nwiHomeId: Buffer;
}

// BUGBUG: this is exactly equal to the ZWaveProtocolCommand.SmartStartIncludedNodeInformation, can we reuse/inherit that somehow?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As comment, how much sharing do we want to do between these classes (or not)?

@@ -0,0 +1,238 @@
import {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this command class is defined in the specs, It's not exposed that I saw in any command class list, so I'm not sure where/how this would be used. Maybe if you had a secondary LR controller in the network? Unknown. We may not need to include this at all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be used in our testing framework, but unless you're sure about these things, I'd leave it out for now.

Comment on lines 48 to 49
// BUGBUG: how much of this can we share with existing stuff? Can we use a ZWaveProtocolCCNodeInformationFrameOptions field to do the `isLongRange` stuff?
// BUGBUG: how much can we share also with the Smart Start things below that are VERY close to this stuff?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, can we leverage base classes and combine a lot of this with the existing ZWaveProtocolCC?

export enum ZWaveLRProtocolCommand {
NOP = 0x00,

// BUGBUG: all defined above, can they be shared?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These have the same # assignments, but are in a separate namespace. Add the missing ones to the list above, or leave these separate (or maybe just reference Foo = ZWaveProtocolCommand.Foo.value or something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave separate IMO, or leave it out completely together with the CC

private listStartNode(): number {
return 256 + NUM_LR_NODES_PER_SEGMENT * 8 * this.listStartOffset128;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The equations given in the spec for this say 255 +, but the given examples result in this being 256 +. What's here is correct experimentally. Maybe the spec was using 1-based math and I missed it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 256 makes more sense. Not the first time that the specs are wrong.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The * 8 seems wrong though?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The * 8 seems wrong though?

Yeah, I think that was a holdover from when I had "NUM BYTES PER SEGMENT" there instead.

Comment on lines 93 to 98
// BUGBUG: move someplace common, see Spec 4.2.5
//
// Although, that section refers to a "callback message", and SetZWaveLongRangeChannel doesn't define a callback message...
export enum ResponseStatus {
FAILED = 0x00,
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ReportStatus here is supposed to be a common thing, but in looking through other things that use it, I didn't see one that was implemented. It's supposed to be used with messages that then have a callback message, so that a failure can stop the callback from occuring, but this message doesn't have a callback message...

Seems like a case where this should have just been pass/fail on it's own, and someone in the spec committee decided to re-use a status field that wasn't necessarily applicable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's supposed to be used with messages that then have a callback message, so that a failure can stop the callback from occuring, but this message doesn't have a callback message...

In that case the response indicates the final result of the command flow, not the callback.

I don't think we need the enum value or a separate field. Just do this like we do here for example:
https://github.com/jtbraun/node-zwave-js/blob/d1e23fc6a2e58c15eff86d31cf78951d8b62476b/packages/zwave-js/src/lib/serialapi/capability/SerialAPISetupMessages.ts#L899-L907

Comment on lines +129 to +130
const LONG_RANGE_SHADOW_NODE_IDS_START = 2002;
const NUM_LONG_RANGE_SHADOW_NODE_IDS = 4;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should be in a header somewhere.

Also, I didn't provide an API to set this stuff. Presumably these are for when you want your LR-controller to show up with a LR nodeId? I don't know.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as the others I suppose -> packages/core/src/consts/index.ts

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO if we have these messages, we should expose an API on the controller for them.

@jtbraun
Copy link
Contributor Author

jtbraun commented Oct 16, 2023

I'm not sure how to request a review to get some feedback? I'm going to assume the maintainers watch for PRs occasionally. @blhoward2 ?

@blhoward2
Copy link
Collaborator

blhoward2 commented Oct 16, 2023

You can summon @AlCalzone, but I'm sure he'd see it in the morning anyway. He keeps a close watch.

@jtbraun
Copy link
Contributor Author

jtbraun commented Oct 16, 2023

I've got a support ticket out to zooz on the "'normal' inclusion doesn't seem to respect the protocol=1 flag" issue. I suspect it's a bug in the ZST39, or undocumented behavior in SILabs' reference design.

The big issues for @AlCalzone right this second are:

  • Opinions on API changes for how to indicate include/exclude longrange/normal.
  • Opinions on names. There's a mix right now of lr*, *LR* and *LongRange*.
  • I haven't taken any stabs at test coverage yet... if you have preference for what and how much, let me know.

There's a ton of BUGBUG/TODO comments right now, I've tried to explain in comments what the questions/issues are.
There's also pre-existing tests that appear to be broken by this. I'll need to look into those.

With the ZST39/ZEN32 I was testing, I have to say "long range" currently looks like an over-promise/under-deliver situation. I got about twice the range of the normal AEOTEC stick in a similar location. While that's good, the star network topology means that the overall network reach is going to be smaller. It's a big shame that they didn't allow for routes, or expand the spec to allow for more than the ~240 nodes in the mesh topology. My signaling environment hasn't been optimized at all, and I am indoors, but I'd hoped for better. 👎

@AlCalzone
Copy link
Member

AlCalzone commented Oct 16, 2023

@jtbraun thanks for this! I'll make sure to take a good look at it soon and give you some feedback, I'm currently dealing with all the weird controller firmware issues that cause problems with the new "unresponsive controller" recovery feature.

Just FYI if it wasn't clear - the ZWaveProtocolCC stuff is mostly used internally and for testing, as these classes are normally used by the controller under the hood. We use it in a few locations to hack our way around missing features in the Serial API.

Copy link
Member

@AlCalzone AlCalzone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this. I've tried to answer most of the questions so you can finally continue. I'll probably have to play with this a bit to see how it all works out before we can merge it.

@@ -0,0 +1,238 @@
import {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be used in our testing framework, but unless you're sure about these things, I'd leave it out for now.

Comment on lines 100 to 109
if (nodeIdSize == 1) {
ret[offset++] = sendingNodeId;
ret[offset++] = destination;

} else {
ret.writeUint16BE(sendingNodeId, offset);
offset += 2;
ret.writeUint16BE(destination, offset);
offset += 2;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (nodeIdSize == 1) {
ret[offset++] = sendingNodeId;
ret[offset++] = destination;
} else {
ret.writeUint16BE(sendingNodeId, offset);
offset += 2;
ret.writeUint16BE(destination, offset);
offset += 2;
}
ret.writeUIntBE(sendingNodeId, offset, nodeIdSize);
offset += nodeIdSize;
ret.writeUIntBE(destination, offset, nodeIdSize);
offset += nodeIdSize;

export enum ZWaveLRProtocolCommand {
NOP = 0x00,

// BUGBUG: all defined above, can they be shared?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave separate IMO, or leave it out completely together with the CC

@@ -130,6 +130,7 @@ export enum CommandClasses {
"Z-Wave Plus Info" = 0x5e,
// Internal CC which is not used directly by applications
"Z-Wave Protocol" = 0x01,
"Z-Wave Long Range Protocol" = 0x04,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The specs call it "Z-Wave Long Range" Command Class, let's stick to that.

// Read either the normal or extended ccId
const { ccId: cc, bytesRead } = parseCCId(payload, offset);
offset += bytesRead;
// CCs before the support/control mark are supported
// CCs after the support/control mark are controlled
// BUGBUG: does the "mark" and support/control convention apply to isLongRange?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseCCId should take care of that.

Comment on lines 93 to 98
// BUGBUG: move someplace common, see Spec 4.2.5
//
// Although, that section refers to a "callback message", and SetZWaveLongRangeChannel doesn't define a callback message...
export enum ResponseStatus {
FAILED = 0x00,
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's supposed to be used with messages that then have a callback message, so that a failure can stop the callback from occuring, but this message doesn't have a callback message...

In that case the response indicates the final result of the command flow, not the callback.

I don't think we need the enum value or a separate field. Just do this like we do here for example:
https://github.com/jtbraun/node-zwave-js/blob/d1e23fc6a2e58c15eff86d31cf78951d8b62476b/packages/zwave-js/src/lib/serialapi/capability/SerialAPISetupMessages.ts#L899-L907

Comment on lines +129 to +130
const LONG_RANGE_SHADOW_NODE_IDS_START = 2002;
const NUM_LONG_RANGE_SHADOW_NODE_IDS = 4;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as the others I suppose -> packages/core/src/consts/index.ts

Comment on lines +129 to +130
const LONG_RANGE_SHADOW_NODE_IDS_START = 2002;
const NUM_LONG_RANGE_SHADOW_NODE_IDS = 4;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO if we have these messages, we should expose an API on the controller for them.

@AlCalzone
Copy link
Member

I've updated the logic to pass the protocol selection using the provisioning entry. LR Inclusion is done exclusively via SmartStart, so this is how to include a LR node:

		driver.controller.provisionSmartStartNode({
			dsk: "...",
			protocol: Protocols.ZWaveLongRange,
			securityClasses: [SecurityClass.S2_Authenticated],
		});

Exclusion works as usual, no changes needed there.

IMO all that's missing now is a bit of cleanup and consistency - I'll do this tonight probably.

Thanks for the amazing head start @jtbraun

@jtbraun
Copy link
Contributor Author

jtbraun commented Jan 16, 2024

I've started taking a look at this again. @jtbraun if you don't mind, I'll take this PR over.
Thanks for the amazing head start @jtbraun

I don't mind at all, and you're very welcome, I'm happy to have helped.

@AlCalzone AlCalzone changed the title feat(longrange): support for long range controllers/devices feat: support Z-Wave Long Range Jan 17, 2024
Copy link
Member

@AlCalzone AlCalzone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO:

  • split the methods in packages/core/src/capabilities/NodeInfo.ts into LR / classic
  • move long-range related methods out of GetSerialApiInitData.ts
  • fix parsing of GetNodeInfoResponse - currently it doesn't know whether the node it is for is ZWLR or classic
  • documentation

Comment on lines 47 to 52
case 0x86: {
// This is _maybe_ a corrupted ACK byte from a ZST39 after a soft reset, transform it and keep it
this.logger?.ACK("inbound");
this.push(MessageHeaders.ACK);
break;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fixed in #6409. I'll need to remove this and rebase here.

Comment on lines 127 to 134
while (offset < payload.length) {
let listEnd = payload.length;
if (isLongRange) {
validatePayload(payload.length >= offset + 1);
const listLength = payload[offset++];
listEnd = offset + listLength;
validatePayload(payload.length >= listEnd);
}
while (offset < listEnd) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, that the length check could be moved to the caller.

@@ -215,34 +230,49 @@ export type NodeInformationFrame =
export function parseNodeProtocolInfo(
buffer: Buffer,
offset: number,
isLongRange: boolean = false,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See specs\Z-Wave Stack Specifications\Z-Wave and Z-Wave Long Range Network Layer Specification.pdf. Long Range added their own Node Information Frame Command in section 6.3.1.2. The non-long-range frame (section 4.3.2.1) has 3 bytes for Basic/Generic/Specific device classes, and then a list of command classes for the remainder of the packet.

The long range frame has Generic and Specific device class bytes (no basic), and then a byte that is the "command list length" (in bytes, as I recall), and then the list of command classes.

Why they felt the need to replace these fields and add the seemingly redundant length, I'm not sure.

If we don't need/want the long range command class in this change (it wasn't needed to get pairing and comms to work). I think this stuff can be backed out.

Comment on lines 1033 to 1039
this._supportsLongRange = resp == RFRegion["USA (Long Range)"];
} else {
this.driver.controllerLog.print(
`Querying the RF region failed!`,
"warn",
);
this._supportsLongRange = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh really? When I was playing around separately with ser2net, soft resetting the controller would cause it to de/re-enumerate over USB. That would cause the file handle ser2net had open to error out, which caused ser2net to close the TCP connection, and then zwave-js/ui would get upset that the ser2net socket disappeared out from under it.

const lrChannelResp = await this.driver.sendMessage<GetLongRangeChannelResponse>(new GetLongRangeChannelRequest(this.driver));
lrChannel = lrChannelResp.longRangeChannel;

// TODO: fetch the long range max payload size and cache it
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so that method probably needs updated to take a isLongRange boolean, and return this value if set?

packages/zwave-js/src/lib/controller/Controller.ts Outdated Show resolved Hide resolved
Comment on lines 1639 to 1647
// BUGBUG: fix me
// if (options.isLongRange && !this.isLongRange()) {
// throw new ZWaveError(
// `Invalid long range inclusion on a non-LR host`,
// ZWaveErrorCodes.Argument_Invalid,
// )
// }
const isLongRange = this.isLongRange();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filling in from elsewhere:

  • yes, LR inclusion MUST be smart start only
  • LR nodes will send both normal and LR prime messages, and its up to the user/controller to respond to one or the other. So the DSK-providing API will need to include a "use LR" boolean that we honor. If the user sets it to a value that's not supported, we should probably just not include the node.

Comment on lines 1654 to 1658
this._inclusionFlags = {
highPower: true,
networkWide: true,
protocolLongRange: isLongRange,
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it hurt to just leave all these here, rather than have many places we set highPower/networkWide to true?

Comment on lines 1671 to 1673
new AddNodeToNetworkRequest(this.driver, {
addNodeType: AddNodeType.Any,
highPower: true,
networkWide: true,
...this._inclusionFlags,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I'll clean this up.

@@ -28,7 +28,7 @@ import { buffer2hex, getEnumMemberName } from "@zwave-js/shared";
export enum ApplicationUpdateTypes {
SmartStart_NodeInfo_Received = 0x86, // An included smart start node has been powered up
SmartStart_HomeId_Received = 0x85, // A smart start node requests inclusion
SmartStart_HomeId_LongRange_Received = 0x87, // A start start long range note requests inclusion
SmartStart_LongRange_HomeId_Received = 0x87, // A start start long range note requests inclusion
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I left a typo in this comment.

@AlCalzone
Copy link
Member

@jtbraun not sure if you just reviewed or Github is acting up - the review comments you supposedly added 6 minutes ago are weeks/months old.
Anyways, just to avoid any conflicts - I'm working on this and will deal with the remaining issues.

@jtbraun
Copy link
Contributor Author

jtbraun commented Jan 17, 2024

I think what happened is:

  • I had started an old review with a LOT of comments detailing questions for other reviewers (you).
  • I never hit the "complete review" button, so you likely never saw any of those comments :-(
  • I saw a typo in some of the code you edited today/yesterday that originated from me, so I commented about it so you'd see it, and closed my review.
  • Presumably because of all the above, you got spammed with a ton of comments from me, instead of the one thing about typos.

Re: typo: Search for "// A start start" in packages/zwave-js/src/lib/serialapi/application/ApplicationUpdateRequest.ts

Copy link
Member

@AlCalzone AlCalzone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most recent PR fixes/refactors the node protocol info parsing. The specs are a bit confusing, but all fields are always there. It's just the actual NIF that's transmitted that doesn't have everything.

However, parsing a GetNodeProtocolInfoResponse currently doesn't know whether it should interpret the contents as ZWLR or ZW classic.

TODO:

  • fix parsing of GetNodeInfoResponse
  • documentation

@AlCalzone AlCalzone changed the base branch from master to release-12.5.x January 19, 2024 12:16
@AlCalzone AlCalzone enabled auto-merge (squash) January 19, 2024 12:20
@AlCalzone AlCalzone merged commit 3fd27bc into zwave-js:release-12.5.x Jan 19, 2024
14 checks passed
AlCalzone added a commit that referenced this pull request Jan 19, 2024
This release adds support for Z-Wave Long Range thanks to the amazing work of @jtbraun. Application developers planning to add support should read [this](https://github.com/zwave-js/node-zwave-js/blob/release-12.5.x/docs/getting-started/long-range.md) to get started.

### Features
* Support Z-Wave Long Range (#6401)

### Config file changes
* Add 2nd product ID for Ring Panic Button Gen2 (#6595)

### Changes under the hood
* Fix compatibility of ESLint plugin with Node.js 18 (#6580)
AlCalzone added a commit that referenced this pull request Apr 2, 2024
This release adds support for Z-Wave Long Range thanks to the amazing work of @jtbraun. Application developers planning to add support should read [this](https://zwave-js.github.io/node-zwave-js/#/getting-started/long-range) to get started.

### Features
* Support Z-Wave Long Range (#6401, #6620)

### Config file changes
* Remove Association Groups 2 & 3 from AEON Labs DSB09 (#6691)
* Correct group 3 label for GE/Enbrighten 26931/ZW4006 (#6703)
* Add new Fingerprint for Ring Contact sensor (#6676)
* Preserve root endpoint in Vision ZL7432 (#6675)
* Add new Product ID to Fibaro Smoke Detector (#6603)
* Add Product ID for Benext Energy Switch FW1.6 (#6602)
* Add fingerprint for Ring Glass Break Sensor EU (#6590)
* Change MH9-CO2 Temperature Reporting Threshold step size to 0.1 (#6581)
* Add new product ID to Fibaro FGS-213 (#6576)
* Add units, improve descriptions for Everspring ST814 (#6712)
* Label and parameter definitions for Sensative Drip 700 (#6514)
* Override supported sensor scales for HELTUN HE-ZW-THERM-FL2 (#6711)
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.

switching to 16-bit node IDs is currently done twice during startup ZWave Long Range Support
5 participants