NRCHKB / node-red-contrib-homekit-bridged

Node-RED Contribution - HomeKit Bridged : Node-RED nodes to simulate Apple HomeKit devices.
https://nrchkb.github.io
MIT License
416 stars 52 forks source link

Deploy Modified Flows causes services to start duplicating behavior #476

Closed lilyball closed 1 year ago

lilyball commented 2 years ago

NRCHKB Plugin Version

1.4.3

Node JS Version

v14.19.0

NPM Version

6.14.16

Node-RED Version

2.2.0

Operating System

Raspberry pi Buster

What happened?

When I Deploy Modified Flows, I start getting bad behavior. Right now what I'm seeing is a single service node starts outputting duplicate messages, though I don't know if that's the full extent of it.

What I'm doing is I have a global config node for the bridge, and a service on that bridge in one of my flows that represents a Switch. If I modify a node on this flow and Deploy Modified Flows, the service node starts spitting out duplicate onSet and onChange messages. I prefer Deploy Modified Flows because I don't want to restart the whole bridge every time I make a change to a node (which happens when deploying everything, and causes a multi-second delay in HomeKit while it waits for the accessory to respond).

Also potentially related, I had my service node set to wait for setup and I had an inject that set nrchkb.setup.characteristicProperties.On, and after one redeploy I got an error message that said

Feb 27 14:14:55 raspberrypi Node-RED[1997]: 27 Feb 14:14:55 - [error] [homekit-service:My Switch] Instead of nrchkb try one of these characteristics: Name, On, Name

I don't recall doing a Modified Nodes deploy, though that might explain it if all injects run after such a deploy (I don't actually know), but it was quite surprising to me. As a result of seeing that I changed the service to stop waiting for setup and just injecting the property alone, as I didn't need to do anything other than set the properties (I do wish the service node supported JSONata for the initial characteristic properties).

How to reproduce?

Global homekit-bridge config node ```json { "id": "6ec32a21d1cf0af0", "type": "homekit-bridge", "bridgeName": "HomeKit Bridge", "pinCode": "redacted", "port": "", "advertiser": "ciao", "allowInsecureRequest": false, "manufacturer": "NRCHKB", "model": "1.4.3", "serialNo": "Default Serial Number", "firmwareRev": "1.4.3", "hardwareRev": "1.4.3", "softwareRev": "1.4.3", "customMdnsConfig": false, "mdnsMulticast": true, "mdnsInterface": "", "mdnsPort": "", "mdnsIp": "", "mdnsTtl": "", "mdnsLoopback": true, "mdnsReuseAddr": true, "allowMessagePassthrough": true } ```
Service node ```json { "id": "a127fb595f217c3f", "type": "homekit-service", "z": "95a77749368cb553", "isParent": true, "hostType": "0", "bridge": "6ec32a21d1cf0af0", "accessoryId": "", "parentService": "", "name": "My Switch", "serviceName": "Switch", "topic": "", "filter": false, "manufacturer": "NRCHKB", "model": "1.4.3", "serialNo": "Default Serial Number", "firmwareRev": "1.4.3", "hardwareRev": "1.4.3", "softwareRev": "1.4.3", "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": 390, "y": 220, "wires": [ [ "f4f545b8ef3e27b0" ], [ "e1e9189927f2e168", "fbd3528887dc802b" ] ] } ```

Steps:

  1. Deploy a flow using these nodes. I have the outputs hooked up to Debug nodes. I also have an inject on startup that sets msg.payload.On though I don't think that matters right now.
  2. Toggle the switch a few times in Home and verify that the expected Debug messages occur.
  3. Modify a node on the flow (including just moving a node).
  4. Deploy Modified Flows
  5. Toggle the switch in Home a few times again

Expected behavior:

After the modified flows deploy, the switch should behave the same as before.

Instead, it started emitting duplicate onSet and onChange messages. Every action (including the initial inject) produced two messages instead of one.

Additional comments?

I expect putting the bridge configuration node into the flow itself would work around this issue (though Deploy Modified Nodes would then still be a problem), but I don't want to do that as I have services on another flow that talk to HomeKit and I don't want to run multiple bridges.

My assumption here is the service node is simply not unregistering itself on stop.

Relevant log output

No response

caitken-com commented 2 years ago

Hi there,

  1. In the config node is an option (tickbox) to prevent duplicate messages. Something like prevent pass through.
  2. Only use the on-change (top) output on each service node. Unless you’re using Television service which you’ll use the bottom output to capture remote button presses.
  3. Use deploy modified nodes only. Deploy modified flow will essentially reset the whole flow.
lilyball commented 2 years ago
  1. In the config node is an option (tickbox) to prevent duplicate messages. Something like prevent pass through.

I don't want to prevent passthrough. Though I'm also not sure why that's a bridge option instead of a per-node option, but it's unrelated to this particular issue as this is not a passthrough issue.

  1. Only use the on-change (top) output on each service node.

I originally did that but had an issue where I wasn't getting any messages at all. So I switched to wiring up both of them, and that worked but was unnecessary. I switched again to just using onSet because what I'm doing here is I'm stuffing the characteristic value into flow context, so duplicate sets aren't a problem, and I figured it was safer to just listen to every event.

But again, not actually related to this issue. When I trigger this issue, both onChange and onSet get duplicated.

  1. Use deploy modified nodes only. Deploy modified flow will essentially reset the whole flow.

Yes, but that's also not a solution because if I touch the service node then that will redeploy it just like deploying the whole flow. I just was using Deploy Modified Flows at the moment because I'm messing about with config and it seemed safer to just redeploy the whole flow instead of worrying about any potential issues caused by not doing so.

caitken-com commented 2 years ago

I’m not sure what you’re trying to achieve, but all three of my points are the solution. Disabling pass through only stops duplicate states, not all messages. Eg with pass through disabled:

  1. Active is false, inject sets active to true, true is passed through.
  2. Active is false, inject sets active to false, nothing is passed through.

Perhaps read our wiki on pass through: https://nrchkb.github.io/wiki/nodes/output-messages/

And again, top output is for characteristic changes and bottom output is only for television service’s button press capture. Eg: repeated navigation presses - which is when you want duplicate messages. Everything else is via top output when you only want to capture state changes in characteristics.

I’ve been using node red and NRHKCB since at least 2017. Never had an issue using only deploy on modified nodes. Resetting a whole flow causes me issues. Just saying 💁‍♂️

caitken-com commented 2 years ago

If you still have both outputs wired up, then that is where you’re getting duplicate messages. Because you’re using it as not intended. Browse the wiki examples for correct usage.

caitken-com commented 2 years ago

Wiki has been updated with info of which output to use: https://nrchkb.github.io/wiki/nodes/output-messages/

GogoVega commented 2 years ago
  1. Do not wire the two outputs, use the top one (the bottom one is used for specific cases) but in no case both.
  2. If you are using Wait for setup you must deploy full in order to initialize the setup.
  3. Your error indicates that the message received does not contain the correct characteristic (probably because of the reason above)
lilyball commented 2 years ago

@caitken-com Thank you for updating the wiki, that is helpful info, though I might also suggest fixing the passthrough documentation that references Home.app, as that makes it sound like third-party HomeKit apps or HomeKit automations won't have hap.session when they will. It should instead just say this is present when the message comes from HomeKit. However, this still is not Passthrough-related. Duplicate messages are fully duplicated, including hap.session[^1], and are not originating from Node-RED (the only input message I have right now is the inject on start, there's no possibility of getting a passthrough past the initial injection).

However this still doesn't solve my problem. When I get duplicate messages, both onChange and onSet are duplicated (I currently have debug nodes hooked up to both, though I did go ahead and change my flow to use onChange since y'all are so adamant about it).

@GogoVega If Wait for setup isn't compatible with Deploy Modified Flows that's a problem and should be fixed or documented. Though I only ever noticed that error message once, so I'm not sure the exact cause. In any case, that's just a side issue, and it would probably be a good idea to at least fix the service node to understand a message that looks like that post-setup and a) treat it as a warning and b) process any included nrchkb.setup.characteristicProperties anyway even if it can't interpret the other configuration. The reason here being that I might be using Wait for setup with nrchkb.setup.characteristicProperties to avoid the possibility of HomeKit being notified that the accessory is in the wrong state (even for a split second) in case that triggers automations.

However, that's orthogonal to my actual issue. I turned off Wait for setup (and just inject the property directly on start as I don't currently have automations tied to this) and my duplication issue still occurs.


I will note that in testing now, deploying modified flows or nodes wasn't triggering the problem, but then when I was trying to debug a separate issue, I deployed modified flows and the issue started occurring again. I'm not sure why it wasn't doing so when I was explicitly trying to reproduce this, but I guess I can say right now that it it usually occurs when deploying modified flows if the flow contains a service node.

[^1]: The whole hap.session is identical, including sessionID and including the remoteAddress/httpPort[^2] pair, so the messages are coming from the same connection. I doubt HomeKit is duplicating the message itself (it has no reason to, and as far as it knows nothing changed about the bridge). The only difference in the messages is the _msgid but AIUI that's a Node-RED thing that tracks messages through the system (though I guess it does prove that the node isn't simply sending the exact same msg object twice).

[^2]: This should probably be called remotePort, I just had to go looking at lsof -i to confirm that this port was the remote side of the connection, not the local side. And probably localPort should be included too, so I can more easily tell if the messages are going to the same bridge/accessory instance or to separate ones. I'd say the remoteAddress and localAddress fields should just be changed to be addr:port pairs instead but I don't know if that would break anyone.

LubosKovar commented 2 years ago

Does not problem still fixed? I confirm exact same behavior as @lilyball describe. Problem persist in new version NR 3.0.2 too.

EDIT: I found one temporary solution. Try "disable" afected service nod - make Deploy - "enable" service nod and again Deploy. Now reset my duplicating. Unfortunately only temporary solution. Next work with service nod cause duplicating again.

caitken-com commented 1 year ago

@lilyball is this resolved?

lilyball commented 1 year ago

I don't know, I haven't touched my node-red setup in over a year (and therefore haven't been deploying anything).

caitken-com commented 1 year ago

Thanks.

GogoVega commented 1 week ago

I suspect #563 is the cause.