Koenkk / zigbee-herdsman

A Node.js Zigbee library
MIT License
456 stars 277 forks source link

New ember adapter implementation targeting EZSP 13 and above #918

Closed Nerivec closed 3 months ago

Nerivec commented 3 months ago

EDIT: Follow updates here: https://github.com/Koenkk/zigbee2mqtt/discussions/21462#discussioncomment-8510320

smaller PRs - he said

Alright, ducks, row, quoting one's self... ✔️ ✔️ ✔️

So, let's start with the disclaimer... Don't use this in your live network. It has only been tested by one (me, who'd have guessed!). Expect problems until this is thoroughly tested in a larger number of installations and nasty buggers are fixed. Otherwise, you could break the house, literally...

I'm introducing this as a completely separate adapter (can be select via configuration.yaml as always, with ember as adapter). This is not only because of the complete rewrite, but also because it currently only supports EZSP 13 / 7.4.x. The focus was put on future support, rather than backwards compatibility; firmware updates being very easy these days. This limitation is hardcoded to prevent any misunderstanding; Z2M will not start if your adapter is not at that version and you try to use this. On the same basis, and because backup was not fully supported before, backups from adapters pre EZSP 12 will be rejected for now. Although you can modify the backup file manually to fake the version, if you are 100% sure your backup file is fine, but in doing so, you assume the risk that you might restore a broken network (in case the backup you have is improper/incomplete). This might change in the future if we notice no issue with previous versions (I don't have access to older backups to know what's what...).

Since this is a separate adapter (albeit for the same protocol), and it does not alter any of the old one's logic, nor any of herdsman logic (except for types related to the introduction of the new adapter), it can be safely reviewed, introduced, and tested. Only users that explicitly chose to do so will use this.

So... from here on, you can put a "should" in front of... well, every line...

Features

Full ASH protocol support (per silabs).

Full EZSP protocol support (per silabs).

Actually, there's too much support here, we'll have to cleanup the functions not intended for Trust Center/Coordinator use. It was faster to implement all than to look closely at each of them and determine their usage... In the meantime, it has no impact since they just aren't used (except a few more lines of code).

Adapter

Backup/Restore (gets its own header!)

Support for backup/restore using zigpy/open-coordinator-backup format is implemented. I've tested several combinations, and it restored everything without issue every time. See Z2M-ember-devlogs, I put some logs and dev/debug info in there on this and other stuff. Same as ZStack, backup has to match config for it to be used, otherwise it defaults to forming a network with config and ignores the backup. If config matches the network on the adapter, it, of course, resumes operation and bypasses restoration.

Support for NVM3 tokens backup/restore is also implemented, although Z2M+frontend will require update to support this. Note: I've tested only the backup part. I didn't want to spent hours fixing my test adapter in the middle of all this in case restoring didn't go well... It creates a single buffer containing the tokens in a specific format -similar to silabs' one-, allowing it to be stored in hex format in JSON (size depends on stack config; expected 3k+ bytes, should be much larger with zigbeed).

General

Tested

Tests done with devices from "the trash pile", i.e. devices that always caused trouble in my live network in the past and ended up in the closet. So, if it works with those...

Untested

Dev Stuff

Being no "expert" in NodeJS, it is entirely possible I f'ed up some parts of this (though it is working, so I must have done at least some things right!). If one passes through here, some feedback/upgrades on the implementations of the various Node-specific features (related to queueing, waitressing, handling tick stuff & whatnot) would be fantastic. Promises, promises :wink:

Also, if someone wants to tackle writing some tests for all this... I did the ASH layer (the critical one) & some utilities, but I'm not that familiar with Jest; it is really slow implementing while reading the docs... Also, I'm sure the ones I've written can be improved.

NOTE: For anyone working on the code of the ezsp adapter, make sure to import the proper files. Don't want to import from the wrong folder; names are similar/identical in many cases but definitely different types.

TODOs

Final note

In case anyone is wondering, and to make my next point, here are some stats on the codebase to make this work (in lines):

path files code comment blank total
Total 22 11,685 8,326 2,915 22,926
. 4 1,541 2,732 290 4,563
adapter 6 3,135 1,443 732 5,310
ezsp 4 5,517 3,339 1,491 10,347
uart 6 1,403 749 384 2,536
utils 2 89 63 18 170

My next point: bear with me if errors slipped in there or if I screwed up something... It is more than likely 😅


Unrelated dev stuff

A few things, not related to this implementation (or maybe it is...), that I've noticed while testing this (instead of opening half a dozen issues...):

Nerivec commented 3 months ago

CI: Well, seems that test I mentioned for the Controller does fail "less than randomly". Not related to this PR though, unless my tests affect the timeout for some reason? The fail appeared randomly when I ran them locally, I figured it might be my machine...

@kirovilya As always, if you want to review/fix anything you spot...

kirovilya commented 3 months ago

That's cool! I have no words :)

Koenkk commented 3 months ago

Nice one! I really like that you focus on just EZSP 13, this should simplify testing and the code base a lot.

Implement various configs for ember. @Koenkk if I can get your input on how you'd like this done?

Do you want to expose these to the user already? In my opinion I would say no unless really needed. It's better to have good defaults.

Regarding testing this, I propose to create a discussion to let people test it. I think it should contain at least the following

Also, what's the reason you picked ember as the name here? I don't know, but would something like ezsp_experimental make more sense?

Nerivec commented 3 months ago

Do you want to expose these to the user already? In my opinion I would say no unless really needed. It's better to have good defaults.

The stack config I'd say yes, assuming TCP=zigbeed is not too good long term (too many setups...). The rest I agree with you, we come up with good defaults first, then determine if some setups (likely low RAM/CPU ones) would benefit from "other defaults" :wink:

Perfect with the discussion. I'll let you create it, like before! You can grab excerpts from my post here no problem. You pretty much covered the main goals, I'd also add to try to be more future-proof, even introduce stuff that Z2M hasn't seen yet (as long as the UI guys are okay with it)! :wink:

I was pretty heavy with debug and console in the codebase to get as much info as possible from testers in the beginning, this should also allow less savvy users that want to help get us some feedback (most potential issues will log without herdsman debug ON at the moment). (Can introduce a logger wrapper like you mentioned before, just a matter of replacing occurrences.)

For the firmware, I'd go with these two links (darkxst is the most stable one in my opinion, the other is the official HA link...):

I picked ember to make it clear this is very separate until a full switch can happen (avoid any kind of confusion as to which is what). _For "EmberZNet", the stack's name, and firmware versions being EmberZNet versions; not multiprotocol though, different beast there (of course...). I don't know, sounded better in my head than gecko I guess... (probably best since the gecko SDK is forking apparently, as mentioned in latest release notes...)_

Koenkk commented 3 months ago

I picked ember to make it clear this is very separate until a full switch can happen

But we are targeting the same adapter as with ezsp right? (except we also now force a certain fw version). Wouldn't it make sense to call it ezsp_experimental/ezsp_improved or something?

Nerivec commented 3 months ago

Actually, that question of yours has been making me wonder why the name "ezsp" took over everywhere in packages, since the stack's name is "EmberZNet" (and multiprotocol variants didn't exist back then); it's like using "ZNP" instead of "ZStack". Weird stuff :wink: If anyone knows the answer, I'm curious...

Same targeted stack yes. The config name doesn't quite matter I guess, although it would not match all the names in classes & such... Ember was short enough and didn't clash with Ezsp to prefix everything Z2M-specific (and much of the silabs stuff is already Ember prefixed), which is how/why I started using it, and then it just made sense to name the config the same...

Nerivec commented 3 months ago

Look at that, all green! Tests failing to test... good one. Seems somehow that specific test I added (skipped in last commit), which purposely times out (after 2sec and change), is additive to the Controller test (that also takes a while), and the result is a "sometimes" timeout fail... Isn't the Jest timeout supposed to be per test?

EDIT: Gave a couple of thoughts on the name thing. I can see one good reason to make a clean break from ezsp; to avoid users referring to old issues/discussions to try to figure out configurations/problems. Since the implementation is entirely different, anything from the "old" ezsp adapter is unlikely to match and might even lead to wrong assumptions... What do you think?

Koenkk commented 3 months ago

I agree with you about keeping ember.

About the stack config, from what I currently understand from it, this option is only relevant when using the multi-protocol firmware (??). Given that the multi protocol firmware is still unstable, do we already want to expose this option to users? I think it would be wise to first focus on non multi protocol and default stack config.

Nerivec commented 3 months ago

That config is actually to initialize the NCP stack or in multiprotocol's case, to initialize zigbeed. Since it is very different (because of the host's capabilities vs a lowly adapter), I can only think it would help to have the config, so Zigbeed is not suffering from low specs (might even avoid troubles for those that still decide to use it). As of now, the detection is automatic based on if adapter path is a TCP path; but as mentioned, this is not so good long-term.

However, I fully intend, like you said, to put the focus (as much time as I can spare anyway...) on NCP support (hence why I didn't spend time testing multiprotocol). That combined with the fact that it doesn't appear to be working great with 7.4... Not to mention, adapters cost 25$, so if a user wants a stable Zigbee and a stable Thread, they can get two; it will probably always work better that way anyway... :wink:

Koenkk commented 3 months ago

As of now, the detection is automatic based on if adapter path is a TCP path; but as mentioned, this is not so good long-term.

Why not?

I've created a draft discussion text, what do you think?

Nerivec commented 3 months ago

Why not?

For now, it should not be an issue (I don't believe there currently are any other reason to use TCP other than multiprotocol/zigbeed)..? Except maybe some far-fetched setups... But in the future... zigbeed alternative, etc, might need other configs (though updates on this can come as needed of course).

Koenkk commented 3 months ago

Then let's not expose it for now, focus on default first.

I've tried to test it with my SkyConnect but the startup fails, log

Edit: I think my adapter is broken, let me test with the Dongle-E

Nerivec commented 3 months ago

I've tried to test it with my SkyConnect but the startup fails, log

Enable rtscts with Skyconnect, hopefully that's just it :wink: (I don't have one, I've only tested with Dongle-E...)

EDIT: If you manage to get the Skyconnect to work, I wouldn't mind a debug log (even if no error) if you can let it run for a while with a couple of devices paired to it since I don't have one. I don't expect much to be different (or at all really), but still would be a good reference to have.

Koenkk commented 3 months ago

Also cannot get my Dongle-E to work, after updating, the web-flashers event don't detect it anymore. I will try again later.

wastez commented 3 months ago

Also cannot get my Dongle-E to work, after updating, the web-flashers event don't detect it anymore. I will try again later.

Did you use the firmware from xsp1989 before? If you did and you now have updated to the darkxst firmware you need to clear the nvm3. There is no possibilty else, otherwise the adapter will not show anything (No bootloader or something else). Don't know why this happen..... Had the same problem and took me hours to get the reason.

Don't forget to backup before but i don't think i've to tell you that.

wastez commented 3 months ago

@Nerivec It's really cool you have improved the code of the adapter. Thanks a lot for the work. Will test it as soon as possible.

Mar1usW3 commented 3 months ago

I've tried to test it with my SkyConnect but the startup fails, log

Enable rtscts with Skyconnect, hopefully that's just it 😉 (I don't have one, I've only tested with Dongle-E...)

EDIT: If you manage to get the Skyconnect to work, I wouldn't mind a debug log (even if no error) if you can let it run for a while with a couple of devices paired to it since I don't have one. I don't expect much to be different (or at all really), but still would be a good reference to have.

i could you provide you logs for a skyconnect but if i edit the adapter to ember in the config i still see ezsp in the logs. I use z2m edge in HA.

Nerivec commented 3 months ago

You followed the instructions here, restarted Z2M, and are still seeing the ezsp adapter logs? You should see lines like these in the logs with ember:

======== Ember Adapter Starting ========
======== EZSP starting ========
======== ASH NCP reset ========

PS: Check that the configuration.yaml file properly registered the change (if you used the UI).

Mar1usW3 commented 3 months ago

found the issue. Had to remove the entry in the UI completely. Here is the Log Should i also test something specific?

Nerivec commented 3 months ago

@Mar1usW3 Thanks for the feedback. If you can enable herdsman-debug and post a log after a couple hours of runtime (then you can disable it again to avoid excessive logging), that'd be great (in here to keep the feedback in one thread). See the first post here about what still needs testing, if you can check one of the boxes... otherwise general usage reports, and stability is good😄

Mar1usW3 commented 3 months ago

can do this but this is my test instance. I can add some devices for test purpose for sure but currently no real setup.

Lstick commented 3 months ago

Cannot change adapter type "ezsp" 2 "ember" from Z2M UI. After pushing "Submit" button value resets to "ezsp".

Zigbee2MQTT version 1.35.3-dev commit: [c5c0a8b] Coordinator type EZSP v13 Coordinator revision 7.4.0.0 build 0 Frontend version 0.6.158 zigbee_herdsman_converters_version 18.34.0 zigbee_herdsman_version 0.34.1

Nerivec commented 3 months ago

@Koenkk I missed one of the config validations. For HA users, there seems to be another overriding check done from here, and in each version.

Mar1usW3 commented 3 months ago

Cannot change adapter type "ezsp" 2 "ember" from Z2M UI.

After pushing "Submit" button value resets to "ezsp".

Zigbee2MQTT version

1.35.3-dev commit: [c5c0a8b]

Coordinator type

EZSP v13

Coordinator revision

7.4.0.0 build 0

Frontend version

0.6.158

zigbee_herdsman_converters_version

18.34.0

zigbee_herdsman_version

0.34.1

as a workaround you can remove the serial entry from the ui and set in configuration.yaml

Lstick commented 3 months ago

Cannot change adapter type "ezsp" 2 "ember" from Z2M UI. After pushing "Submit" button value resets to "ezsp". Zigbee2MQTT version 1.35.3-dev commit: [c5c0a8b] Coordinator type EZSP v13 Coordinator revision 7.4.0.0 build 0 Frontend version 0.6.158 zigbee_herdsman_converters_version 18.34.0 zigbee_herdsman_version 0.34.1

as a workaround you can remove the serial entry from the ui and set in configuration.yaml

Nice. So far it's working now.

Koenkk commented 3 months ago

Added the ember adapter for the HA addon!

Nerivec commented 2 months ago

@Mar1usW3 (& others here) I'm keeping track of progress here, in case you want to focus on anything more specific in your tests 😉

Mar1usW3 commented 2 months ago

@Mar1usW3 (& others here) I'm keeping track of progress here, in case you want to focus on anything more specific in your tests 😉

sure, was busy this week. Will test some things this weekend.

mamrai1 commented 2 months ago

OTA checks fail with no response from device

Nerivec commented 2 months ago

@mamrai1 Please try again once PR is merged in dev, if you can. It has now been tested on an actual device, and worked. If it still doesn't work, please provide the device page (on https://www.zigbee2mqtt.io/supported-devices/), and herdsman debug logs.

OTA sure is a long thing to test thoroughly 😉

Hedda commented 2 months ago

@Nerivec slightly off-topic but thought you might also interested in this ASHv2 WIP-work by puddly -> https://github.com/zigpy/bellows/pull/606

Actually, that question of yours has been making me wonder why the name "ezsp" took over everywhere in packages, since the stack's name is "EmberZNet" (and multiprotocol variants didn't exist back then); it's like using "ZNP" instead of "ZStack". Weird stuff 😉 If anyone knows the answer, I'm curious...

EZSP (EmberZNet Serial Protocol) was probably because bellows for zigpy referenced it? -> https://github.com/zigpy/bellows

At least bellows has always referred to EZSP protocol version to indicate which version of EmberZNet firmware is supported.

i.e. https://github.com/SiliconLabs/gecko_sdk/blob/gsdk_4.4/protocol/zigbee/app/util/ezsp/ezsp-protocol.h#L33

com.zsmartsystems.zigbee (openHAB) also call driver for "ember" -> https://github.com/zsmartsystems/com.zsmartsystems.zigbee

PS: A fun fact is that "Ember" is a legacy name as the older Zigbee stack was called that by the company Silabs bought it from:

https://news.silabs.com/2012-07-09-Silicon-Labs-Completes-Acquisition-of-Ember

https://en.wikipedia.org/wiki/Ember_(company)

Hedda commented 2 months ago

@Nerivec also off-topic but perhaps you would also be interested in looking into extending the Open ZigBee Coordinator Backup Format if needed for proper backups and restore migration across different drivers and versions?

https://github.com/zigpy/open-coordinator-backup

Open ZigBee Coordinator Backup Format is an open standard specification jointly used by both zigbee-herdsman (Zigbee2MQTT and IoBroker) as well as zigpy (ZHA integration in Home Assistant and the Zigbee Plugin for Domoticz + the older Jeedom plugin).

Hedda commented 2 months ago

@Nerivec Can I suggest that you consider starting a new issue tracker for "[WIP] ember adapter implementation and testing" similar to the existing issue tracker one for the EZSP adapter? -> https://github.com/Koenkk/zigbee-herdsman/issues/319 (also similar to ZiGate implementation issue -> https://github.com/Koenkk/zigbee-herdsman/issues/242)

That is, while your ember adapter is in early developmet and still listed as experimental on Zigbee2MQTT's Supported Adapters webpage it might be a good idea to have a single dedicated issue in this repo for tracking feedback and input from testers.

PS: If you do then you would need for Koenkk to mark that issue as dont-stale so that it does not automatically close.