Skip to content

Conversation

@puddly
Copy link
Contributor

@puddly puddly commented Dec 10, 2023

Zigpy provides a way for radio libraries to limit request concurrency (by default 8). It seems like zigpy-xbee did not use it. You can increase/decrease the default via YAML:

zha:
  zigpy_config:
    max_concurrent_requests: 8  

@Shulyaka this may actually be the only change required for zigpy-xbee to work properly with the watchdog. If you can find a value that works well for your XBee, this may supersede #172.

@codecov
Copy link

codecov bot commented Dec 10, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8314dcd) 100.00% compared to head (2b1b21f) 100.00%.
Report is 1 commits behind head on dev.

❗ Current head 2b1b21f differs from pull request most recent head 6059c1e. Consider uploading reports for the commit 6059c1e to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##               dev      #173   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines          759       760    +1     
=========================================
+ Hits           759       760    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@puddly puddly mentioned this pull request Dec 11, 2023
@Shulyaka
Copy link
Contributor

I've tested this change. Unfortunately it does not solve the problem, because only tx_explicit API frames are limited, but others, such as commands and remote commands, are not.
I think we are better off without watchdog at all.

@puddly puddly force-pushed the puddly/concurrency-limiting-semaphore branch from 2b1b21f to 6059c1e Compare December 15, 2023 22:29
@puddly
Copy link
Contributor Author

puddly commented Dec 15, 2023

I've reimplemented this PR on top of the unreleased zigpy priority lock/queue:

pip install git+https://github.com/puddly/zigpy@zigpy/priority-lock

This should deprioritize transmit commands so that any watchdog polling task would have to wait only for a single command slot to open up, as opposed to the queue being completely vacated.

@Shulyaka
Copy link
Contributor

I think we need to increase the AT_COMMAND_TIMEOUT anyway.

Consider the follow scenario:

  1. A quirk calls a remote AT command for a device that is powered off
  2. Because the queue is currently empty, the command is sent to the device and a timeout of REMOTE_AT_COMMAND_TIMEOUT started
  3. Watchdog sends the VR command
  4. Although the priority is higher, another command is already waiting, so the command is not actually sent to the device
  5. The AT_COMMAND_TIMEOUT expires earlier, so the VR times out and the connection is reset.

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.

2 participants