[Feature Request]: Persistent accessory identifier for subflows a.k.a. Subflow support

[kevinkub]

Is your feature request related to a problem? Please describe.
NRCHKB does not properly support the use of accessories in subflows. The reason for that is that an accessory is identified by a unique id that is currently calculated based on the nodes identifier. However for subflows node-red dynamically assigns node ids, as soon as a deployment happens. Within HomeKit this leads to the deletion of the old device and the creation of a new one, which renders subflows unusable for most use-cases.

Describe the solution you'd like
Instead of relying on a nodes identifier, another unique but static identifier should be found, that generates consistent unique ids independently from deployments. To keep backwards compatibility the behavior should only be adapted for accessory nodes in subflows.
I would suggest to use node._flow.type == “subflow” to detect if a node is placed within a subflow and change the unique id generation in that case to use node.alias + node._flow.path instead of node.id. The alias property of node is the original node.id of the node in a subflow that is e. g. stored when saving the subflow and it is as static as a node.id in a regular flow. As a subflow may be used multiple times, that is unfortunately too static and the flow.path property should be appended. That property contains the full hierarchy from the main flow through all subflows and is static per node that represents a subflow.
The proposed changes would make NRCHKB fully subflow compatible without breaking existing flows.

Describe alternatives you've considered
All alternatives I could come up with would break unique ids for existing nodes and I would consider that a breaking change.

Additional context
The only thing that makes me feel a little bad about this request is the use of _flow which is kind of a private variable. Even though as it is just used for reading information about the context and the safe usage of the variable could be ensured at runtime with appropriate checks, the risk of changes to that variable would have the impact on this library that it behaves like it does currently (reassigning ids in subflows on deploy.

I have implemented and successfully tested the changes locally already.

[crxporter]

Related to #298

[crxporter]

Subflow support is currently planned for our 2.0.0 release but if you've got a non breaking way to do it, then maybe we can bump that up to a sooner release.

@Shaquu I'll let you review and comment!

[Shaquu]

Hi, looks great, but one part is missing (let's discuss).
According to Hap docs some fields should be persistent through the life of an Accessory (or Bridge).
It is why it was also planned to use those fields' id creation:

So we have to:

1. Purge Bridge/Accessory if data above changed by generating new id
2. For standard node id we should use accessoryInformation + node.id as node-red has some small lookups for id duplicates in flow
3. For node in a subflow we could use accessoryInformation + node.alias + node._flow.path as node-red generate new node.id every time for nodes in a subflow

Question is:
Do you want to provide subflow only solution or would you like to solve it in general?

[Shaquu]

For the whole solution we need some compatibility marker.
I have created nrchkbConfigCompatibilityOverride but it is not too good, and it should be replaced.
We could probably add a new field in UI called nrchkbVersion or so.
For version 1.4 we could apply new id generation, for missing version (aka 1.3 or less) we apply the old way.

[Shaquu]

Possibly we do not need _alias as _flow.path might be unique already. Can you check @kevinkub ?

[kevinkub]

If I were about to implement it at work, I think I would just implement a bugfix now (meaning consistent behaviour in in flows and subflows). You have an Agenda and you put the breaking change up for a V2, which is perfectly fine from my perspective. But I am basically three days into this community and do not feel to be in the right position to give good advices ;-).

Regarding _alias: The property is required because the same subflow may contain multiple nrchkb service nodes. _path is needed because one subflow may be used multiple times.

[Shaquu]

Okay, @kevinkub for now go with minimal version so adding support for subflow using your idea.
Please make a PR against dev branch.

Thanks!

[kevinkub]

According to Hap docs some fields should be persistent through the life of an Accessory (or Bridge).

I will create a pull request in a second, but I have good news for you @Shaquu. The accessory UUID already changes when name, manufacturer, serial no or model changes. This is already implemented in the current master branch and as far as I can tell in compliance with the HomeKit protocol. So if I am not wrong, applying my pull request should make a breaking change obsolete.

[Shaquu]

@kevinkub possibly you are right for Service Node and it's accessory but not for Host Node (aka Bridge and Standalone Accessory)
Have a look:

node-red-contrib-homekit-bridged/src/lib/HAPHostNode.ts

Lines 79 to 91 in 8fb7267

	self.bridgeUsername = macify(self.id)
	const hostUUID = uuid.generate(self.id)
	const hostTypeName =
	self.hostType == HostType.BRIDGE ? 'Bridge' : 'Standalone Accessory'
	log.debug(`Creating ${hostTypeName} with UUID ${hostUUID}`)
	if (self.hostType == HostType.BRIDGE) {
	self.host = new Bridge(self.name, hostUUID)
	} else {
	self.host = new Accessory(self.name, hostUUID)
	}

[Shaquu]

@crxporter please test on dev

[crxporter]

So yeah, I got distracted with some other projects.

Anyone else testing this? Or is everyone depending on me?

[crxporter]

Alright I have learned enough to go through a first test.

I made a pair of light bulbs. It looks like having them in subflow creates the bridge twice. The way I would expect this to work each device on the subflow would share a bridge. So I can make like "zwave dimmers" subflow and have a single "zwave dimmers" bridge with as many dimmers showing up as however many copies of the subflow I'm using.

Did I do it wrong? Did I do a classic "user thinks differently from developer"?

My setup:

And the flow:

[{"id":"10da0714.abbbd9","type":"subflow","name":"Subflow 1","info":"","category":"","in":[{"x":200,"y":180,"wires":[{"id":"ae2adbd5.93bcb8"}]}],"out":[{"x":580,"y":140,"wires":[{"id":"ae2adbd5.93bcb8","port":0}]},{"x":580,"y":220,"wires":[{"id":"ae2adbd5.93bcb8","port":1}]}],"env":[{"name":"BulbName","type":"str","value":"Bulb","ui":{"type":"input","opts":{"types":["str"]}}}],"color":"#DDAA99"},{"id":"ae2adbd5.93bcb8","type":"homekit-service","z":"10da0714.abbbd9","isParent":true,"hostType":"0","bridge":"332ec063.42443","accessoryId":"","parentService":"","name":"${BulbName}","serviceName":"Lightbulb","topic":"","filter":false,"manufacturer":"NRCHKB","model":"0.140.1","serialNo":"Default Serial Number","firmwareRev":"0.140.1","hardwareRev":"0.140.1","softwareRev":"0.140.1","cameraConfigVideoProcessor":"ffmpeg","cameraConfigSource":"","cameraConfigStillImageSource":"","cameraConfigMaxStreams":2,"cameraConfigMaxWidth":1280,"cameraConfigMaxHeight":720,"cameraConfigMaxFPS":10,"cameraConfigMaxBitrate":300,"cameraConfigVideoCodec":"libx264","cameraConfigAudioCodec":"libfdk_aac","cameraConfigAudio":false,"cameraConfigPacketSize":1316,"cameraConfigVerticalFlip":false,"cameraConfigHorizontalFlip":false,"cameraConfigMapVideo":"0:0","cameraConfigMapAudio":"0:1","cameraConfigVideoFilter":"scale=1280:720","cameraConfigAdditionalCommandLine":"-tune zerolatency","cameraConfigDebug":false,"cameraConfigSnapshotOutput":"disabled","cameraConfigInterfaceName":"","characteristicProperties":"{}","waitForSetupMsg":false,"outputs":2,"x":370,"y":180,"wires":[[],[]]},{"id":"332ec063.42443","type":"homekit-bridge","z":"10da0714.abbbd9","bridgeName":"Subflows","pinCode":"549-29-190","port":"","advertiser":"ciao","allowInsecureRequest":false,"manufacturer":"NRCHKB","model":"0.140.1","serialNo":"Default Serial Number","firmwareRev":"0.140.1","hardwareRev":"0.140.1","softwareRev":"0.140.1","customMdnsConfig":false,"mdnsMulticast":true,"mdnsInterface":"","mdnsPort":"","mdnsIp":"","mdnsTtl":"","mdnsLoopback":true,"mdnsReuseAddr":true,"allowMessagePassthrough":true},{"id":"c588c2de.a782a","type":"subflow:10da0714.abbbd9","z":"81d3dc83.6b5098","name":"","env":[{"name":"BulbName","value":"Bulb 1","type":"str"}],"x":440,"y":200,"wires":[["3126728c.c47e56"],[]]},{"id":"3126728c.c47e56","type":"debug","z":"81d3dc83.6b5098","name":"","active":true,"tosidebar":true,"console":false,"tostatus":false,"complete":"false","statusVal":"","statusType":"auto","x":670,"y":180,"wires":[]},{"id":"d1ec3b2a.d97108","type":"subflow:10da0714.abbbd9","z":"81d3dc83.6b5098","name":"","env":[{"name":"BulbName","value":"Bulb 2","type":"str"}],"x":440,"y":280,"wires":[["722114cb.946a4c"],[]]},{"id":"722114cb.946a4c","type":"debug","z":"81d3dc83.6b5098","name":"","active":true,"tosidebar":true,"console":false,"tostatus":false,"complete":"false","statusVal":"","statusType":"auto","x":670,"y":260,"wires":[]}]

@kevinkub can I get some lessons?

[crxporter]

That being said ... it does work so - good progress!

[crxporter]

Alright, @Shaquu figured it out, he said (on discord pm):

So if you set up Bridge inside the subflow then it will be created for each instance. If you re use one created outside then it will not be duplicated(edited)

If I create the bridge while inside the subflow editor, I get a bridge per subflow. If I use a bridge created "on the outside" in the normal editor, I get a single shared bridge.

[kevinkub]

That is an ugly edge case that I never ran into. What do you think is a good way to fix that? Blocking bridge creation in subflows?

[Shaquu]

For me it looks like a useful case. I would be in favour of keeping it but adding huge warning in a subflow.

Short text with a link to wiki

[kevinkub]

I do not think that it is possible to pair that bridge as you do not see the Code in the UI and I think the bridge would be deleted if the flow is redeployed… the node.id fix from this pull request should be applied to the bridge to make sure that the bridge does not change its uuid upon redeploy.

[crxporter]

I was able to pair both when I had created two subflows each with a bridge. I just had to open the config node for the code.

I see it being useful in the case of a stand alone accessory inside the subflow. Probably not as a bridge.

[crxporter]

PS: Status updates from the flow (showing the pair code) is easy enough. Use status node to pass HK node status to subflow status.

From outside the subflow it looks like this -

[Shaquu]

So my proposal. Let me know if it is not the best idea.

It will only be displayed for Service in a Subflow, with Host Type Bridge.

[Shaquu]

I do not think that it is possible to pair that bridge as you do not see the Code in the UI and I think the bridge would be deleted if the flow is redeployed… the node.id fix from this pull request should be applied to the bridge to make sure that the bridge does not change its uuid upon redeploy.

We can either fix uuid for bridge or go with it as a known bug until.

[crxporter]

Would the warning always appear or only if in a subflow setup? I don't mind if we need to do some user education for people who want to use the new toys.

If it shows always, move it to the bottom I think. Less "in your face" for non subflow users but there if someone's going for subflow setup.

[Shaquu]

It will only be displayed in a subflow.