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

Remove errornous assert on srcAddr16 lookup #1207

Merged
merged 2 commits into from
Sep 30, 2024

Conversation

schrluka
Copy link
Contributor

@Nerivec introduced an assert(srcAddr) in https://github.com/Koenkk/zigbee-herdsman/blame/5d02efe44826401521b85259dde5e3ca4421e312/src/adapter/deconz/adapter/deconzAdapter.ts#L665

This prevents some devices from joining. During join some device devices might emit messages which do not yet have the short (network) address set. So asserting when the lookup fails is not acceptable. All other code must deal with messages that provide a 64 bit address.

It looks like there was some restructuring of code here (ZDO stuff). I did not check all of this if it works when srcAddr is undefined.

Unfortunately I don't have access to a Conbee3 at the moment, so I can't test this. I will try to test this next week and let you know.

BR,
Lukas

@fjumeaux
Copy link

Good evening. Thank you for taking time on the problem. Unfortunately, it still doesn't work. Attached, my logs between permit to join and end of permit to join.

The device : Sonoff Zbmini 2 (Extreme).

logs.txt

Idem with an Ikea [Valhornn.

@schrluka
Copy link
Contributor Author

@fjumeaux Sorry, my message might have been missleading: did you replace the package zigbee-herdsman inside your zigbee2mqtt with the version I forked and selected the commit referenced in the PR when doing so?
You need to do this to get access to the updated code. I can't push my changes to Koens repos directly. I merely opened a PR so he can have a look and merge it into herdsman...

@fjumeaux
Copy link

Aie. Only does a pull.

I’m under docker. Don’t have access to the code

@schrluka
Copy link
Contributor Author

... that's what I assumed. If you can wait a couple more days, it might be easiest to wait until I have tested it and Koen has pulled it.

@fjumeaux
Copy link

I should survive … 🤪

@Nerivec
Copy link
Collaborator

Nerivec commented Sep 28, 2024

@Koenkk did that part of the PR, tested it too. Sounds like an edge-case of some sort?
But if I'm reading this right, it looks like that value (resp.srcAddr16) is only used if the srcAddrMode is not 0x03, so, I assume it would be valid according to the mode. At first glance, I'd say the entire else is not needed (which would be a lot better too since it invokes upstream logic that has no place in the driver)?
As for srcAddr, it is only used for the waitress resolving, which means, the network address should always be valid, (except for the ZDO network address response, which has the special srcEUI64 instead), otherwise, it means the waitress is not expecting a resolve anyway (hence the validity check before resolving: (typeof req.addr === 'number' ? srcAddr !== undefined && req.addr === srcAddr : srcEUI64 && req.addr === srcEUI64)).

@Koenkk I'll let you take a closer look and test all this, since you have the adapter.

@@ -724,7 +723,7 @@ class DeconzAdapter extends Adapter {
clusterID: resp.clusterId,
header,
data: resp.asduPayload,
address: resp.destAddrMode === 0x03 ? `0x${resp.srcAddr64!}` : resp.srcAddr16!,
address: resp.srcAddrMode === 0x03 ? `0x${resp.srcAddr64!}` : resp.srcAddr16!,
Copy link
Owner

Choose a reason for hiding this comment

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

Was this changed on purpose? (because this was not changed in 98e3384#diff-e287278735156a983a1ca18677f8f6580622520aa743a7a9c191db0ed111f20d)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sry, forgot to mention this. Using destAddrMode for srcAddr looks like a typo. Changed this without thinking much.

@fjumeaux
Copy link

No improvement on my side. Test with sonoff zbmini 2 and Ikea Vallhorn. Still unable to pair.

@schrluka
Copy link
Contributor Author

@Koenkk Just tested it with my Conbee 3 and several devices:
zb-mini2, hue color lamp (9290022166), ikea dimmable (LED1934G3) & a hue dimmer switch (929002398602)
All of them joined just fine. I formed a network with a total of 9 devices.

@fjumeaux
Copy link

We are talking about release 58c856b that I installed and tested yesterday in Docker ?

@schrluka
Copy link
Contributor Author

Can talk for Koen, but I tested only my changes. I guess they are not jet in your container.

@Koenkk Koenkk merged commit 3e7bfaa into Koenkk:master Sep 30, 2024
1 check passed
@Koenkk
Copy link
Owner

Koenkk commented Sep 30, 2024

Thanks!

@fjumeaux
Copy link

fjumeaux commented Oct 1, 2024

How can I do this update ? I'm on the dev branch and on docker with the "latest-dev" image.

Thanks in advance for the info

@Nerivec
Copy link
Collaborator

Nerivec commented Oct 1, 2024

Wait until #1210 is merged, and zigbee2mqtt dev is updated to it.

@Koenkk
Copy link
Owner

Koenkk commented Oct 1, 2024

Fix will be available in todays release.

@fjumeaux
Copy link

fjumeaux commented Oct 1, 2024

Container updated, Ikea Vallhorn Ok, Sonoff zbmini2 Ok, Sonoff SNZP-06P Ok !

Merci beaucoup !!

@Schebberle
Copy link

Schebberle commented Oct 2, 2024

thanks for the fix. 2 weeks ago i tried to pair an SNZB-02D for several hours but returned it afterwards as defective. (probably it was fine, i just read today about the problem/fix)
this week i tried to pair an Ikea 30W LED Driver: ICPSHC24-10EU-IL-2. also spend several hours by trying to pair it with no success. just installed the update today and worked like charm.

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.

5 participants