Various Wi-Fi QoL improvements#6424
Conversation
…t auto_restore_wifi
(NetworkListener is instantiated by the FM & Reader apps).
(This is absolutely not based on the AutoSuspend plugin, no siree :D).
30s was a weee bit short ;p. i.e., it could conceivably be tripped while reading a Wikipedia blurb, a point during which you'd probably expect to not have to suddenly re-enable WiFi ;).
different sessions (Meaning the first check will be bogus and will quite likely kill WiFi instantly).
Prevents spamming the logs with bogus attempts if wifi_was_on is true, but WiFi is no longer enabled.
handlers... (i.e., gestures/profiles).
|
The noise margin is still low enough to make an idle SSH session (w/ KeepAlive) enough to keep Wi-Fi up ;). |
poire-z
left a comment
There was a problem hiding this comment.
I'm not really familiar with all that, but looks alright.
| self:beforeWifiAction(callback) | ||
| return true | ||
| end | ||
|
|
There was a problem hiding this comment.
(nitpick, but unnecessary blank line - here and above)
There was a problem hiding this comment.
That one was on purpose, to separate the actual core logic from the menu/UI shenanigans that come later.
The one in networkListener was done with the same kind of thing in mind, but doesn't really make sense because there are event handlers above AND below it ;p.
pazos
left a comment
There was a problem hiding this comment.
(untested) lgtm. If something crashes on Cervantes we can fix it later.
BTW, Does make sense to invest time to simulate the same auto down when idle behaviour on the (linux) emulator?
It shouldn't be too hard to fake it like we fake wifi state. Given that /sys/class/iface/ can be changed to something like /tmp/. Anyways, nothing to do with this PR.
Very good job 👍
(Generally, to separate logical units of code).
|
@pazos: I don't expect much trouble on Cervantes, FWIW. Might want to resync the Wi-Fi scripts at one point, but that's it ;). I will run a test on Kindle later today, though :). |
Do we even handle Wi-Fi at all on the emulator? (i.e., a NetworkManager backend or something?). |
It more-or-less behaves like the prompt action on Kobo, but with way less UI feedback (i.e., we essentially hang with no feedback until the connection is successful, at which point the action fires).
wifi-async kill to only run it on devices that have one.
|
Okay, made it not regress and mostly usable on Kindle, I'll leave UI fine-tuning to others if necessary ;). At a quick glance, on PB, I'm hoping the InkView stuff is blocking, so we should be okay. On reMarkable, err, do we even support controlling the Wi-Fi at all? And on Android, the callbacks are never forwarded, so it shouldn't regress. |
Haha no, nor should we. (Unless it's necessary for some platform I suppose, in which case we might as well I guess.) |
|
Nope, no Wi-Fi handled on the emulator, but there's a fake networkManager instance in https://github.com/koreader/koreader/blob/master/frontend/device/sdl/device.lua#L313-L334, which is useful to test things that rely on network state changes.
Nope, reMarkable has Wi-Fi but we don't manage it. It should be pretty similar to Nickel: if we kill the blob running then WiFi will stay alive until the battery is empty. |
Frenzie
left a comment
There was a problem hiding this comment.
Looks like I forgot to press submit on the review thing (but I hadn't finished reading the diff yet)
|
|
||
| function NetworkMgr:getPowersaveMenuTable() | ||
| return { | ||
| text = _("Kill Wi-Fi connection when inactive"), |
There was a problem hiding this comment.
Kill sounds a bit aggressive perhaps ^_^
| text = _("Kill Wi-Fi connection when inactive"), | |
| text = _("Automatically disable Wi-Fi connection"), |
| text = _("Kill Wi-Fi connection when inactive"), | |
| text = _("Disable Wi-Fi connection when inactive"), |
There was a problem hiding this comment.
I'll admit that was somewhat colored by rfkill buttons, where the term is already used to make it exceedingly clear that the modem is actually turned off ;).
(It's also short, which was a plus ;p).
* Revamped most actions that require an internet connection to a new/fixed backend that allows forwarding the initial action and running it automatically once connected. (i.e., it'll allow you to set "Action when Wi-Fi is off" to "turn_on", and whatch stuff connect and do what you wanted automatically without having to re-click anywhere instead of showing you a Wi-Fi prompt and then not doing anything without any other feedback). * Speaking of, fixed the "turn_on" beforeWifi action to, well, actually work. It's no longer marked as experimental. * Consistently use "Wi-Fi" everywhere. * On Kobo/Cervantes/Sony, implemented a "Kill Wi-Fi connection when inactive" system that will automatically disconnect from Wi-Fi after sustained *network* inactivity (i.e., you can keep reading, it'll eventually turn off on its own). This should be smart and flexible enough not to murder Wi-Fi while you need it, while still not keeping it uselessly on and murdering your battery. (i.e., enable that + turn Wi-Fi on when off and enjoy never having to bother about Wi-Fi ever again). * Made sending `NetworkConnected` / `NetworkDisconnected` events consistent (they were only being sent... sometimes, which made relying on 'em somewhat problematic). * restoreWifiAsync is now only run when really needed (i.e., we no longer stomp on an existing working connection just for the hell of it). * We no longer attempt to kill a bogus non-existent Wi-Fi connection when going to suspend, we only do it when it's actually needed. * Every method of enabling Wi-Fi will now properly tear down Wi-Fi on failure, instead of leaving it in an undefined state. * Fixed an issue in the fancy crash screen on Kobo/reMarkable that could sometime lead to the log excerpt being missing. * Worked-around a number of sneaky issues related to low-level Wi-Fi/DHCP/DNS handling on Kobo (see the lengthy comments [below](koreader#6424 (comment)) for details). Fix koreader#6421 Incidentally, this should also fix the inconsistencies experienced re: Wi-Fi behavior in Nickel when toggling between KOReader and Nickel (use NM/KFMon, and run a current FW for best results). * For developers, this involves various cleanups around NetworkMgr and NetworkListener. Documentation is in-line, above the concerned functions.
* Minor cosmetic code cleanup * String tweak
* Simplify logic, and more detailed debug logging * Kill Nickel's FIFO on startup Avoids udev/udhcpc scripts hanging when trying to open() it.
(i.e., enable that + turn Wi-Fi on when off and enjoy never having to bother about Wi-Fi ever again).
NetworkConnected/NetworkDisconnectedevents consistent (they were only being sent... sometimes, which made relying on 'em somewhat problematic).Incidentally, this should also fix the inconsistencies experienced re: Wi-Fi behavior in Nickel when toggling between KOReader and Nickel (use NM/KFMon, and run a current FW for best results).
Original message:
Possibly a bit heuristic-y, but seems to do the job well enough on my H2O.
Limited to
hasWifiManagerdevices (i.e., Kobo/Cervantes/Sony).Could possibly work on a few other pure-Linux devices with a sane
turnOffWifi? (Kindle? [EDIT: Nope, see below])(This would require implementing
NetworkMgr:getNetworkInterfaceName()there, too).This change is