Handle start before DHCP resolution #583
Conversation
|
Note: this was copied from #433 |
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #583 +/- ##
==========================================
- Coverage 51.50% 50.90% -0.60%
==========================================
Files 24 25 +1
Lines 5677 5663 -14
==========================================
- Hits 2924 2883 -41
- Misses 2753 2780 +27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I'd like to test a thing or two on a real AmpliPi but long story short my AmpliPi is down due to not being able to build the web app. This looks like a good improvement so far though. Unfortunately this does not solve #579 as-is.
|
Alright I think this thing is ready! Here's the new changes:
This was tested on the following sequence:
On each new connection the advertisement was able to identify its change of state and advertise its new ip address (if it had one) if it didnt have one it failed and waited 30 seconds to retry |
|
@rtertiaer Can you check out how I used logging here? Also make sure the interface resolution make sense? |
amplipi/mdns.py
Outdated
| if ok(): | ||
| new_ip_addr, _, new_iface = _find_best_iface(ok) | ||
| if new_ip_addr != ip_addr: | ||
| log.info(f'IP address changed from {ip_addr} ({good_iface}) to {new_ip_addr} ({new_iface})') |
There was a problem hiding this comment.
pylint is warning about f-strings in logging. Seems like the TLDR is it's more performant to pass log.info('Message %s', variable) since then if the logging level is changed the f-string doesn't have to be evaluated. It also means if an error occurs during evaluation of the f-string it gets logged. I suspect these aren't good enough reasons for us to have a second method of string formatting to be remembered, so I added disable=logging-fstring-interpolation to our .pylintrc. Feel free to remove this override and fix the logging strings if you feel otherwise. See here for some of the discussion on this: pylint-dev/pylint#1788
There was a problem hiding this comment.
so I added
disable[...]
👏
amplipi/mdns.py
Outdated
| # TEST: use logging in this module | ||
| log = logging.getLogger(__name__) | ||
| log.setLevel(logging.INFO) | ||
| sh = logging.StreamHandler(sys.stderr) | ||
| sh.setFormatter(logging.Formatter('%(name)s: %(levelname)s - %(message)s')) | ||
| log.addHandler(sh) |
amplipi/mdns.py
Outdated
| if ok(): | ||
| new_ip_addr, _, new_iface = _find_best_iface(ok) | ||
| if new_ip_addr != ip_addr: | ||
| log.info(f'IP address changed from {ip_addr} ({good_iface}) to {new_ip_addr} ({new_iface})') |
There was a problem hiding this comment.
so I added
disable[...]
👏
amplipi/zeroconf.py
Outdated
|
|
||
| # TEST: use logging in this module | ||
| log = logging.getLogger(__name__) | ||
| log.setLevel(logging.INFO) |
There was a problem hiding this comment.
nitpick: I think about the only thing I might change about the logging would be to either:
- not set the log level at all. this defaults to
logging.WARNING, but permits us to set the log level elsewhere/at the root logger (notably during app startup). Should we fully migrate to ussingloggingat some point, we'll probably poke this then, which is why this is a nitpick, OR - use an environment variable to expose this straight away and set a default to
logging.INFOwhen it is unset. I like this less, but gives us some flexibility straight away.
personally? I'd probably just leave this as-is for now til we migrate all print statements, and then opt for option 1.
Lohrer
force-pushed
the
issue-432
branch
2 times, most recently
from
fbd42b9 to
9d18f30
Compare
- use the interface connected to the default gateway This avoids hardcoding the pi's interfaces and should make the MDNS advertisement test work more reliably on our test computers
Fixes #432
Checklist
./scripts/testAlso try and include #579