-
Notifications
You must be signed in to change notification settings - Fork 260
Add timeouts to get_ringtone and get_canned_messages #810
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
Add timeouts to get_ringtone and get_canned_messages #810
Conversation
Seems reasonable enough for now to get things working again for those cases. A 2s limits feels like it'd be a bit low for doing this over remote admin is my main concern. Would it be possible to move this handling into the export-config part, rather than within the node.py functions? A timeout isn't really a big deal with the individual commands the way it is with export, so I think it'd be fine if only that side has the extra handling. |
In my (limited) testing --get-ringtone and --get-canned-messages would just hang with no indication of what was going on if the modules aren't installed, so maybe it's useful to keep the timeout and log message in node? I've not tested with remote admin at all. What do you think a good delay before timeout would be? |
Tested the fix as is with my t-echo and it seems to fix issues/814. |
Thanks for testing! For clarification, I believe the issue revolves around the modules being not included at all for for some targets. It doesn't matter if the module is on or off, only whether it's present. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #810 +/- ##
==========================================
- Coverage 60.11% 59.97% -0.15%
==========================================
Files 24 24
Lines 4217 4227 +10
==========================================
Hits 2535 2535
- Misses 1682 1692 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Digging in a little deeper I find we have access to |
Closing in favor of #818 |
When firmware is built without these modules these requests just hang. This PR adds a 2 second delay before moving on.