Retry admin channel setting retrieval and add configurable timeout#665
Retry admin channel setting retrieval and add configurable timeout#665ianmcorvidae merged 15 commits intomeshtastic:masterfrom
Conversation
Attempts multiple times to fetch things over the admin channel before giving up.
ianmcorvidae
left a comment
ianmcorvidae
left a comment
There was a problem hiding this comment.
I think I like your thinking here in general. Got a couple comments/recommendations to get CI a bit happier, hopefully.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #665 +/- ##
==========================================
+ Coverage 60.70% 60.95% +0.25%
==========================================
Files 24 24
Lines 3667 3683 +16
==========================================
+ Hits 2226 2245 +19
+ Misses 1441 1438 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Otherwise, semantically, it's off-by-one.
For example, 0x0 will generate an unhandled ValueError. This was caught during a random unit test run for 3.9, so I figure I'd fix it. This is unrelated to the PR otherwise.
| elif valstr.startswith("0x"): | ||
| # if needed convert to string with asBytes.decode('utf-8') | ||
| val = bytes.fromhex(valstr[2:]) | ||
| val = bytes.fromhex(valstr[2:].zfill(2)) |
There was a problem hiding this comment.
Added this because fromStr failed on 0x0 as fuzzed input. This is unrelated to the other changes, so I can pull this out if necessary.
There was a problem hiding this comment.
Glad to accept some test fixes alongside other stuff, so no worries there for sure!
|
Thanks @ianmcorvidae for the hints on the CI failures -- made it easy to get familiar with the CI stack on this repo. |
ianmcorvidae
left a comment
ianmcorvidae
left a comment
There was a problem hiding this comment.
I think this looks good to bring in. Thanks for the updates! I know you mentioned in Discord that you've been having fewer issues with 2.5, which is great, but I'm sure this'll still be valuable.
| elif valstr.startswith("0x"): | ||
| # if needed convert to string with asBytes.decode('utf-8') | ||
| val = bytes.fromhex(valstr[2:]) | ||
| val = bytes.fromhex(valstr[2:].zfill(2)) |
There was a problem hiding this comment.
Glad to accept some test fixes alongside other stuff, so no worries there for sure!
2 participants
Couple things I noticed while working with the admin channel with the Python CLI:
I'm not 100% on the wording of the help messages -- it's not my strong suit. I've tested these changes manually,
but didn't make any changes to testsand added (hopefully enough) test coverage. I feel reasonably confident about the changes, but the changes add complexity that wasn't there.I'll note that in 2.5.x, the admin channel, especially with channel info retrieval, seems to be a lot more reliable, so this might be a lot more complexity that could be unnecessary. If you think that's the case, I think just customizing the timeout might be nice, since five minutes is a pretty long period of time.