-
Notifications
You must be signed in to change notification settings - Fork 290
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
Waitress timer is being set at the wrong time #856
Comments
NOTE - I am using the EZSP driver - I understand the timeout and queuing is driver specific. But I think that the EZSP driver follows the same pattern as other drivers, ie.
The order should be:
|
The crashing may already have been fixed by 33fa223 (available in the dev branch) |
@Koenkk It still leaves the situation of, if I have a lot of things to go out at once, the current concurrency queue doesn't work as it should (ie. things can/will still timeout before ever hitting the zigbee device). The concurrency limiter is supposed to delay some actions until there is 'space' available on the zigbee device, and we should only consider the event timed out if it's taken more than <timeout time, currently 10s> after being sent to the zigbee network. So the ticket is still valid, though it'd be great if the crashing was stopped (I use Z2M on TrueNAS, so it's not easy for me to test a dev version of Z2M, so I can't confirm). |
@Nerivec you are currently looking into the EZSP queue, is this problem related to that? |
I fixed a few waiters that were being started too early a while back, that should take care of what you mentioned in your 2nd post. The logic should be as follow everywhere now (unless I missed one):
The actual buffering/draining to the port is handled by serialport+node stream (the adapter should also do its own buffering logic, dictated by firmware config). There hasn't been any need to override that behavior so far... Are you still experiencing issues with latest release (1.35.2)? |
Actually, looking further, this MIGHT already be resolved. The concurrency queue is here: With most commands being send via. the queue here: Which goes here: Notably, the concurrency is happening BEFORE the timer is set in this sequence, which would make this ticket moot.
|
I have still been on 1.35.1, and this tends to be more an issue on startup with high load, which I have obviated by turning logging to warn (which really slows things down with HA autoconfig). But yeah, it's likely your fixes from 3 weeks ago did fix this specific issue already. |
Not specifying the count defaults to 1: zigbee-herdsman/src/utils/queue.ts Line 11 in 43f6ab7
But yes, I'll look into removing it eventually (it's basically holding just to pass to the UART queue). Actually, you might get some feedback on your issues after that PR is merged, since it should then support alerting on NCP-reported issues like lack of buffers, network congestion related stuff (detected by the firmware). I'm guessing you're eating those buffers quite fast with that many devices (assuming most are not far-apart reporting). As for the startup, I'll look if there are any improvements to be made in the number of calls to the NCP, that should help network-wise. There is also room for improvement as far as caching static or near-static values, that would avoid quite a few NCP calls. I'm currently looking into writing a lot more tests for the whole stack though; many changes, many potential mistakes... 😉 |
@corporategoth I believe I read somewhere, your background was C/C++, if you want to take a look at the SDK logic, to see if you spot features that'd improve Z2M's handling... here is the link, if you didn't have it already https://github.com/SiliconLabs/gecko_sdk It works on callbacks, so the logic is a bit of a pain to follow (though my background's not C, so I'm probably far slower than you would). And it is split in multiple parts that interact with each other based on activated plugins/device type... which doesn't really help keep code paths separate... If you need some bedtime reading material...
|
@Nerivec I am primarily a C/C++ developer, this is true. But I know nothing of what you are sending to/from the device, how the device works, or are trying to achieve / what shortfalls there are. The documentation you linked seems quite comprehensive though. As Z2M is in typescript, unless you're having trouble deciphering the docs, C/C++ coding ability won't help you. Reading the actual source is not useful, since what you really want to do is implement the specifications outlined in the docs. The first doc, the gateway protocol seems fairly straightforward, it describes a wire protocol, which is not language specific. I assume this is already implemented in the EZSP implementation in Z2M. The second doc, the reference guide, while giving information using C types, should also be fairly straightforward. Since Z2M is not written in C/C++, you can't use it directly, but it seems to just be listing the basic field names, types and order of the 'data' components referenced in the wire protocol. For Z2M purposes though, this should be fairly easy to interpret, since every field is resolving essentially down to a uint8_t (unsigned 8-bit integer), uint16_t (unsigned16-bit integer, etc).
Although this is all using C++ types/nomenclature, you just need to copy this into typescript to make it work for Z2M. The third document, the uart host interfacing guide, seems fairly useless, it seems to just be describing 'how to talk to a UART device' ... nothing special. The fourth document seems more about how to packet capture on the device, or use their specific IDE to send data to/from the device. Not really useful here, since you already have the serial interface code that is talking to the device, and have bidirectional communication. The last document, while interesting, seems more about how to use their C API. Something useful if you were coding in C, and were using the API they provide directly. But Z2M is not written in C/C++, and doesn't use their C API. What you're actually doing is re-implementing the wire API in Typescript (with the help of docs 1 and 2). So this doc also is useless for Z2M purposes. Though I guess it does describe some of the basics of initialing, sending and receiving, etc to the device - so not completely useless, but stuff that's already implemented in Z2M, otherwise we would have no EZSP driver in Z2M. In short, I'm not really sure what you want my help on? But I am pretty sure if you wanted me to look at this, it would have to be something done real time, to try and answer your questions live, as a github comment is not a good forum for that :) |
@Nerivec if you are on the Z2M discord, you should find me there as PreZ - feel free to reach out to me there. |
Wow, didn't waste any time there! It was more a general suggestion really, in case you saw any interesting "lower-level" features that you thought Z2M could benefit from, the EZSP part anyway. But yes all the base protocol stuff is done, it just needs a bit of cleanup & tweaking, and then the addition/use of newer commands. It does make for exceptionally large (dangerous) commits though; so I'm focusing a bit on automated testing at the moment. I've gone through more silabs docs than I can count, but I'm one of these people who learns faster from reading code than docs 😄 |
@Nerivec I know next-to nothing about how zigbee works, let alone EZSP itself. So I doubt I could provide much insights of any features not being used that would benefit from Z2M. Especially since I don't know what we're already using, or what anything does. If I studied it long enough and looked at the current code, I could probably figure out how Zigbee works, how EZSP is implementing it and what features would be good to use. I did see some stuff around memory management/ram usage that would be cool to use to ensure we either use the EZSP to it's full capacity, but also don't run out of memory or whatever. I saw stuff about callbacks, etc. But I don't know really what it's doing right now, let alone what it could be doing if we only used more functions. |
That said, one thing I would probably have done is built a library specifically for interacting with the EZSP driver that replicates all the reference doc's functionality. So declares all the structures / enums, declares functions for each message that can be sent, taking and returning the appropriate structures, etc. Then wire up all those functions so they send to or receive from the device appropriately. And ensure we can accept unsolicited messages and invoke callbacks for each message we could receive in an unsolicited manner. Once that's done, I'd package that up as the EZSP implementation for typescript/JS. Keeping in mind there would be basically no business logic in there, just implementing the spec, so implementing all possible communications to/from the EZSP device. THEN I would use that TS library in the EZSP driver in Z2M, using the APIs and structures defined in the EZSP library created above. ie. I would remove any/all low-level communication code from Z2M, and make Z2M operate via. an API (that you created by implementing the reference docs). THEN worry about how much of the API could you put to use in Z2M (ie. looking at all API functions, and thinking 'Am I using it? Could I use it more? Do I need it?') and code accordingly. While this would take a little longer, it would be a better solution IMO. And you would then have a JS-based EZSP library that could be used so different projects could interact with EZSP cards, not just Z2M. But JS is not one of the languages I really use often, or well, so doing said implementation is not something I could dive into. I'm actually not a huge fan of implementing low-level functionality in high-level languages actually, even though it's possible. I would love to see Z2M as a C++ app grin - the performance would be off the charts if written well, even if you had a JS front end (UI) tallking to a C++ backend (zigbee/MQTT). But whose got time for that? :) |
I'm sure the implementation could use some love memory-wise, serialization is a bit "bloated" at the moment in my opinion. But same, that carries a lot of risk until we have more tests. |
What are these "tests" you speak of ... never heard of them ;) |
This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 30 days |
The waitress timer for messages is being set at the wrong time.
Specifically, it is being set when a command is requested, NOT when it is put on the zigbee device. This can cause a command to time out (which, at least on EZSP, can cause the whole app to crash) without the command ever having gotten onto the zigbee device.
To test this, create a device with concurrency of 1. Send it 2 commands, both of which will get a response at 2/3rds the timeout period.
The expected behavior is both will succeed, because from the time we sent it to the zigbee device, we got the required response before the timeout.
However, because the timeout is set when the command is requested (ie. before it went into the concurrency-limited queue), NOT when it was sent to the zigbee device, the second command will be timed out, despite the fact it should succeed.
In practical terms, this means that either when there is a lot of commands going out (which hits the concurrency limiter), OR it Z2M is particularly busy (meaning the main loop gets tied up with a lot of MQTT traffic, say, in the case of HA connecting and doing it's auto-configuration), commands can time out before ever hitting the zigbee device.
Suggested fix: Put the waitress timeout on the message ONLY when it's being send to the actual device, NOT when it's enqueued to the concurrency limited queue.
The text was updated successfully, but these errors were encountered: