Skip to content

Reconnect if multiple CoAP PDUs go unanswered#247

Open
roysjosh wants to merge 1 commit into
Jc2k:mainfrom
roysjosh:fix-unavailable
Open

Reconnect if multiple CoAP PDUs go unanswered#247
roysjosh wants to merge 1 commit into
Jc2k:mainfrom
roysjosh:fix-unavailable

Conversation

@roysjosh

@roysjosh roysjosh commented Nov 9, 2022

Copy link
Copy Markdown
Contributor

No description provided.

@roysjosh

roysjosh commented Nov 9, 2022

Copy link
Copy Markdown
Contributor Author

Might help with "unresponsive" accessories, as mentioned in home-assistant/core#80131 and the forums

@codecov

codecov Bot commented Nov 9, 2022

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 12.50000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.65%. Comparing base (4b02e0d) to head (7ba5f1d).
⚠️ Report is 276 commits behind head on main.

Files with missing lines Patch % Lines
aiohomekit/controller/coap/connection.py 12.50% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #247      +/-   ##
==========================================
- Coverage   66.74%   66.65%   -0.09%     
==========================================
  Files          71       71              
  Lines        6697     6703       +6     
==========================================
- Hits         4470     4468       -2     
- Misses       2227     2235       +8     
Flag Coverage Δ
unittests 66.65% <12.50%> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Jc2k

Jc2k commented Nov 9, 2022

Copy link
Copy Markdown
Owner

Will this look like the device is disconnected when an operation is in flight?

@roysjosh

roysjosh commented Nov 9, 2022

Copy link
Copy Markdown
Contributor Author

I've seen some accessories go unavailable and never recover. This is a guard against that. The PDU send/receive counters should be in sync except for the window of time between a request and response.

@Jc2k

Jc2k commented Nov 9, 2022

Copy link
Copy Markdown
Owner

Sure I get what this fixes, I'm just trying to imagine what could happen between the send and receive - could we solve one bug and introduce some races. E.g. if I toggle a button too quickly could the 2nd write to the device happen in parallel, see the device is disconnected, and kill the session, and then we need to re-auth.

Is there a potential for the user to see devices go unavailable randomly for a second, just because there is a request in flight?

I think we already have a timeout on post_bytes - maybe that should explicitly kill the session?

And maybe we should use an operation lock like BLE does to stop concurrent access?

@roysjosh

roysjosh commented Nov 9, 2022

Copy link
Copy Markdown
Contributor Author

Ahh I see, good point. Hmm. Maybe the post_bytes exception handler would be a better location.

We could add that to be safe. Even if I found that Nanoleaf bulbs could handle >1 outstanding request, that may not be the same case for battery-powered devices.

@roysjosh roysjosh marked this pull request as draft November 9, 2022 15:51
@roysjosh roysjosh marked this pull request as ready for review December 21, 2022 19:40
@roysjosh

Copy link
Copy Markdown
Contributor Author

Approach this a different way. Network errors or timeouts increment a counter which counts towards being considered disconnected. This should solve situations where HAP+Thread accessories end up unavailable and never recover.

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