-
Notifications
You must be signed in to change notification settings - Fork 302
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
New ember adapter implementation targeting EZSP 13 and above #918
Conversation
CI: Well, seems that test I mentioned for the @kirovilya As always, if you want to review/fix anything you spot... |
That's cool! I have no words :) |
Nice one! I really like that you focus on just EZSP 13, this should simplify testing and the code base a lot.
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 |
The stack config I'd say yes, assuming TCP=zigbeed is not too good long term (too many setups...). Perfect with the discussion. I'll let you create it, like before! You can grab excerpts from my post here no problem. I was pretty heavy with 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 |
But we are targeting the same adapter as with |
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". Same targeted stack yes. The config name doesn't quite matter I guess, although it would not match all the names in classes & such... |
Look at that, all green! EDIT: Gave a couple of thoughts on the name thing. I can see one good reason to make a clean break from |
I agree with you about keeping 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 |
That config is actually to initialize the 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... 😉 |
Why not? I've created a draft discussion text, what do you think? |
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... |
Then let's not expose it for now, focus on 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 |
Enable 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. |
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? Don't forget to backup before but i don't think i've to tell you that. |
@Nerivec |
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. |
You followed the instructions here, restarted Z2M, and are still seeing the
PS: Check that the |
found the issue. Had to remove the entry in the UI completely. |
@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😄 |
can do this but this is my test instance. I can add some devices for test purpose for sure but currently no real setup. |
Cannot change adapter type "ezsp" 2 "ember" from Z2M UI. Zigbee2MQTT version |
as a workaround you can remove the serial entry from the ui and set in configuration.yaml |
Nice. So far it's working now. |
Added the |
OTA checks fail with no response from device |
@mamrai1 Please try again once PR is merged in dev, if you can. It has now been tested on an actual device, and worked. OTA sure is a long thing to test thoroughly 😉 |
@Nerivec slightly off-topic but thought you might also interested in this ASHv2 WIP-work by puddly -> zigpy/bellows#606
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. 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 |
@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). |
@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? -> #319 (also similar to ZiGate implementation issue -> #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 |
This reverts commit eb66e0a.
This reverts commit 33fbd72.
EDIT: Follow updates here: Koenkk/zigbee2mqtt#21462 (comment)
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, withember
asadapter
). 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
ember
adapter part pretty well if needed (I zipped a version matching this PR and put it in the repo linked above -didn't check if it got everything though, as always, read the code...-).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 😉
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
zigbee2mqtt
repo will need to be updated to supportember
adapter, else will fail to validate config on start (@Koenkk I'll let you do that). Should be:adapter?: 'deconz' | 'zstack' | 'ezsp' | 'zigate' | 'ember',
"enum": ["deconz", "zstack", "zigate", "ezsp", "ember", "auto"],
EmberAdapter
toAdapter.create
ember
toSerialPortOptions
ember
to Controller test (error detection that needs all adapters listed)fromUnifiedBackup
to include EZSP-specific stuff.ember
. @Koenkk if I can get your input on how you'd like this done? Here's the list:default
orzigbeed
available at the moment).F
's, uint32_t) will have to be dealt with eventually. With backup supported, I'm sure a few users with larger networks will hit it within a year (according to my guesstimates...). I've disabled the code to broadcast a new network key switch for now, since Z2M would not support it (one-way config at the adapter level). Also, I haven't tested it; and some more research needs to be done on impact, timings, etc. Until then, it will give the user a warning, if they get "too close to the sun".ENABLE_ROUTE_DISCOVERY
in APS options while using source routing (even though it would seem to make sense not to use it with source routing...); for now I left it in there, doesn't seem to hurt; can remove if troubles arise from it.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):
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...):
Call controller constructor options mixed with default options
seems to fail rather randomly with Jest timeout. Might need a bit of tweaking. (I probably only noticed because I ran the darn tests so many times...)ezsp
adapter backup for v13 is broken (although not enabled, so not a problem per say). The wrong keys would be exported in that code path (wrong enum used, the newerexportKey
command doesn't useEmberKeyType
; they use the Sec Man one).