Node-red-contrib-homekit-bridged: [BUG] Fundamental Change in NRCHKB has broken every single one of my HomeKit devices

[TheOriginalMrWolf]

Hi,

I recently upgraded to NRCHKB 0.7.1 from an older version (maybe three minor revisions behind) and, unfortunately, it has broken _every single one_ of my (35+) HomeKit devices exposed by NRCHKB.

Two things have changed which represent a _fundamental_ difference from previous implementations. If intended, then NRCHKB should have a major version change.

Issues:

1. When an NRCHKB node gets a command from a HomeKit client (ie: iPhone/Home app turns a device on/off), the NRCHKB node output message no longer contains the hap object (ie msg.hap no longer exists, so checking for msg.hap.context fails with "_[error] [function:send to xyz] TypeError: Cannot read property 'context' of undefined_")
2. When the NRCHKB node receives a message from Node Red (ie: from an NR timer or other external event), this message no longer passes through the NRCHKB node (ie: 'old' functionality was _NR msg -> NRCHKB node -> next_node_, and _next_node_ could tell if this was originated from HomeKit or something else upstream by checking _msg.hap.context_). Instead, externally originated msg just stops at the NRCHKB node.

To me this makes sense & simplifies things considerably, but...

• If this was intentional then it should have been a _major_ version change, and documentation should be updated (eg: https://github.com/NRCHKB/node-red-contrib-homekit-bridged#i-only-want-to-get-messages-when-something-has-been-changed-in-the-home-app-but-also-all-messages-i-send-into-the-homekit-node-get-forwarded-too-how-do-i-stop-that)
• If this was not intentional then it is a major bug... or opportunity to reconsider the design...
• Also, I _really_ dislike nodes that just drop the message. It 'feels' very counter to NR philosophy. I _much_ prefer the 'older' approach - pass the message regardless but supply sufficient information so that the person using the node can decide how to deal with the various scenarios

Could you please have a look and let us know what the story is?

[sjorge]

I noticed this yesterday too and pointed it out on the roadmap issue, so far it seems like this is an upstream change. But it looks like it is still being investigated.

[Shaquu]

Question is if it's broken by nrchkb or hap-nodejs or node-red.
I can't tell right now.

[Shaquu]

TLDR. Change was not intended. I have no idea what caused this and I regret that I published new version before more tests.
My question. What was exactly in hap.context?

[sjorge]

I never used it other than checking if it was a forwarded message or one originatin form home.app, but IIRC it had the old and new state attached inside or was missing when it originated from home.app.

[Shaquu]

Okay I have found a problem regarding missing hap.context. It is our code.
Change introduced here:
https://github.com/NRCHKB/node-red-contrib-homekit-bridged/commit/abd952a63c8556737c40a62045d78a107dd94d62#diff-96c00c5cf5fffc7270072441b83664f8

I can try to revert it, but it will not be one liner.

Let's vote:
revert: thumbs up
live it how it is: thumbs down

@sjorge @radokristof @TheOriginalMrWolf @NorthernMan54 @NRCHKB/test @NRCHKB/dev @NRCHKB/documentation

[radokristof]

@Shaquu and what changed the behaviour of the outputs? I mean now when the command is not orignated from it's HomeKit node, it won't output anything.
I think the best would be to bring back the hap message and leave the output handling how it is now.
That way who has removed the hap checks from the function nodes, will still work. Other who has these checks will also work, because every time the hap will not be undefined.

[sjorge]

Keep it as it is now, the change in the node-red flows was rather easy to do.

Also it looks like a lot of us were trying to filter out the passthrough packets anyway.

Edit or better: redirect the passthroigh message to a different output. So we have best of both worlds.

[Shaquu]

@radokristof use voting button please.
I can add hap.context but it will be always filled with data since output will be always from HomeKit.
So the question is if that won't break anything more? Do we have to only return hap to fix issues with "undefined" but it's not required for logic anymore since there is no output for no HomeKit inputs?

[radokristof]

@Shaquu Yes you are right. This was a proposal from me, so this way editing the functions (removing these checks) is not required.

[Shaquu]

@radokristof @sjorge @TheOriginalMrWolf Please check current dev if it's enough for you to be consider as a fix to current issue. I don't want to force people to change flows because we broke compatibility.

[sjorge]

The msg.hap.context is now back, I had to import a backup of my old states before I changed them to get everything back to work.

{"payload":{"On":true},"hap":{"context":{"keepalive":true,"124.10":true,"124.11":true,"124.12":true,"124.13":true,"122.10":true,"126.10":true,"126.11":true,"126.12":true,"126.15":true,"126.18":true,"127.10":true,"127.16":true,"127.17":true,"127.18":true,"127.13":true,"125.10":true,"125.11":true,"125.12":true,"125.13":true}},"name":"Desk Lamp","topic":"zigbee2mqtt/bedroom/desk_lamp","_msgid":"42457c34.32aff4"}

Although the content is not what I remember it to be, this content is pretty useless. It used to have a diff of the old and new state I think.

[Shaquu]

@sjorge please check dev

• I think now in msg.hap there should be all as old: oldValue, newValue, context
• if you inject context on node input it should appear in output the old way

About context.

• If Active==0 and you inject context, and Active=0 then there will be no output
• If Active==1 and you inject context, and Active=0 then there will be output
• If Active==0 and you inject Active=1 then there will be output with empty hap

[sjorge]

Info inside there but it still doesn’t seem to match the old behavior for me, looks like my states are now broken again (old config import)

[Shaquu]

What is broken now?

[MGKQB]

Please do not pass messages through! It makes all flows much easier.

Yes, that requires a one time change in flows, but it save a lot of time and makes life much simpler! I hated the additional effort to filter.

And regarding breaking compatibility. This is a 0.x release to breaking changes are to be expected if they improve the software!

[Shaquu]

Oh my, it's difficult to decide here.
I can disable passthrough but msg.hap will be less precise (we won't have msg.hap.oldValue I think)

[MGKQB]

If you cannot restore the old functionality anyway there is not much point in this discussion anyhow.

Please make it at least a setting in the bridge node. And disable pass through by default, this make the node a LOT easier to use for new users!

[sjorge]

I’m not sure what exactly broke, I’m on my phone so did not spend a whole lot of time digging deeper.

But as @MGKQB said, I personally found the new behavior so much more pleasant.
I was caught a bit of guard on the initial 0.7.0 update because it wasn’t mentioned anywhere, but that was about it.

The passthroigh is IMHO a pain in the ass to deal with.

[Shaquu]

Updated dev branch.
Bridge config has new field called Allow Message Passthrough (default true).

If Allow Message Passthrough is true then all input msg should appear on output.
If Allow Message Passthrough is false then only msg containing context should appear on output (so those from Home.app and those with injected context).

@sjorge @MGKQB @radokristof @TheOriginalMrWolf please test and comment if it's better and if it is not breaking any functionals.

[MGKQB]

Why did you enable it by default? Everyone seems to hate the pass through! (Me included)
IMHO this should be disabled by default since only advanced users would ever need this.
Make life easier for new users, I was pulling my hair out when I started.

Btw I am traveling and cannot test this week, sorry.

[Shaquu]

It’s enabled by default because people used to create flows based on this. We can wait a while and change it to false in next release maybe.
On 13 Oct 2019, 23:08 +0200, MGK [e-mail], wrote:

Why did you enable it by default? Everyone seems to hate the pass through! (Me included)
IMHO this should be disabled by default since only advanced users would ever need this.
Make life easier for new users, I was pulling my hair out when I started.
Btw I am traveling and cannot test this week, sorry.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.

[radokristof]

I agree with you. This makes it a seamless update. You can disable it with one click now...

Sent from my iPhone

On 2019. Oct 13., at 23:09, Shaq [e-mail] wrote:

It’s enabled by default because people used to create flows based on this. We can wait a while and change it to false in next release maybe.
On 13 Oct 2019, 23:08 +0200, MGK [e-mail], wrote:

Why did you enable it by default? Everyone seems to hate the pass through! (Me included)
IMHO this should be disabled by default since only advanced users would ever need this.
Make life easier for new users, I was pulling my hair out when I started.
Btw I am traveling and cannot test this week, sorry.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.

[Shaquu]

Can you test it @radokristof ?
On 13 Oct 2019, 23:12 +0200, Kristof Rado [e-mail], wrote:

I agree with you. This makes it a seamless update. You can disable it with one click now...

Sent from my iPhone

On 2019. Oct 13., at 23:09, Shaq [e-mail] wrote:

It’s enabled by default because people used to create flows based on this. We can wait a while and change it to false in next release maybe.
On 13 Oct 2019, 23:08 +0200, MGK [e-mail], wrote:

Why did you enable it by default? Everyone seems to hate the pass through! (Me included)
IMHO this should be disabled by default since only advanced users would ever need this.
Make life easier for new users, I was pulling my hair out when I started.
Btw I am traveling and cannot test this week, sorry.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.

[radokristof]

Will test on my test bench in an hour.

Sent from my iPhone

On 2019. Oct 13., at 23:17, Shaq [e-mail] wrote:

Can you test it @radokristof ?
On 13 Oct 2019, 23:12 +0200, Kristof Rado [e-mail], wrote:

I agree with you. This makes it a seamless update. You can disable it with one click now...

Sent from my iPhone

On 2019. Oct 13., at 23:09, Shaq [e-mail] wrote:

It’s enabled by default because people used to create flows based on this. We can wait a while and change it to false in next release maybe.
On 13 Oct 2019, 23:08 +0200, MGK [e-mail], wrote:

Why did you enable it by default? Everyone seems to hate the pass through! (Me included)
IMHO this should be disabled by default since only advanced users would ever need this.
Make life easier for new users, I was pulling my hair out when I started.
Btw I am traveling and cannot test this week, sorry.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.

[radokristof]

It seems Ok for me.

[Shaquu]

Merged into master as 0.8.0
Enjoy.

[sjorge]

I was asleep, 0.8.0 works fine.

• msg.hap has the old and new values on change
• passthrough can be disabled making the checks unneeded.

So best of both the old and unintentional new behavior.

[cacherocks]

I use nodes that receive data from a homekit (NRCHKB) node to control lights, switches, covers, etc.
At 0.8.0 dimming of light, the RGB and white spectrum palette stopped working.

It all worked on 0.6.2, otherwise everything seems to work, I continue to test.

[cacherocks]

At 0.8.0 dimming of light, the RGB and white spectrum palette stopped working.

my mistake, the Accessory Category type was not selected, works fine.

[Shaquu]

@cacherocks what was correct category and for what service it was to make it work correctly?

[debouttek]

0.8.0 works again as before.

[cacherocks]

@cacherocks what was correct category and for what service it was to make it work correctly?

After updating to 0.8.0, a new item "Accessory category" appeared with the indication "Choose..."

For example, in light bulbs, if you do not select the "Accessory category" Lightbulb, then dimming, choosing RGB and white spectrum does not work.

This is not a problem for me, it’s just not quite obvious in a quick search for a problem.

[Shaquu]

@cacherocks thanks, I will solve it in next minor release.

[cacherocks]

On a branch, I found that linked nodes that fall into the Accessory Category "OTHER" (such as InputSource, BatteryService)/"PROGRAMMABLE_SWITCH" (such as StatelessProgrammableSwitch) are marked as erroneous. In version 0.8.0, such errors did not occur.

[Shaquu]

@cacherocks try again on newest dev and if error still occurs then please share flow (only part that is making error)

[cacherocks]

After updating to the last dev branch, the problem with the attached devices remained (red triangles are still in place), but it is eliminated if you open the NRCHKB node and click done. Magic =)

[cacherocks]

[Shaquu]

Well, If you have original flow from 0.8.0 and you could import it into latest dev it would be good to do it and look if there is 'open done' step required.

In latest dev I fixed accessory category automatic mechanism.

[Shaquu]

Is it resolved?

[Shaquu]

Time to close it :)