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

Another round of Kobo Fixes #3939

Merged
merged 14 commits into from May 10, 2018

Conversation

Projects
None yet
3 participants
@NiLuJe
Member

NiLuJe commented May 9, 2018

Fix hang on suspend when WiFi is on but we're not aware of it (c.f., #3936).
NOTE: It would probably be useful if we could actually simply trust wifi_is_on, but I guess that means some more checks on NetworkManager's init? (And accurate and network-agnostic device-specific ways to do that quickly and cheaply). OTOH, what I went with on Kobo should do the job, and I'm pretty sure we could get something useful out of lipc on Kindles.

A bit of script cleanup after #3920 & #3876

Pave the way for properly handling #3925 by properly identifying Rev2 variants of the H2O² & Aura SE.

NiLuJe added some commits May 9, 2018

Trim unneeded stuff from startup script
I was somehow convinced I'd already done that...
While we're there, explain why we need to siphon those specific vars
Fix a stray eth0
-> $INTERFACE
Make getFirmwareVersion less fragile on Kobo
Not that we actually use it right now, but, still. :D
Actually implement getProductId
Instead of a stray c/p ^^
Properly identify the Rev2/Mark7 variants of existing devices
Namely, the H2O² and Aura SE
Not that the H2O²r2 support is still broken, this just allows us to
implement it cleanyl without breaking handling of the original H2O²
Try harder not to suspend with WiFi on on Kobos
Because otherwise, things go boom. (re #3936)
-- without us being aware of it (i.e., wifi_was_on still unset or false),
-- because suspend will at best fail, and at worst deadlock the system if WiFi is on,
-- regardless of who enabled it!
if network_manager.wifi_was_on or network_manager:isWifiOn() then

This comment has been minimized.

@NiLuJe

NiLuJe May 9, 2018

Member

I'm hoping this doesn't behave stupidly on devices where isWifiOn() isn't set?

This comment has been minimized.

@NiLuJe

NiLuJe May 9, 2018

Member

Because that's basically everywhere except Android and Kobo, so that would be... not good? :D.

This comment has been minimized.

@Frenzie

Frenzie May 9, 2018

Member

It'd just return nil, which is false-y for the sake of the or.

This comment has been minimized.

@Frenzie

Frenzie May 9, 2018

Member

Regarding the comment, did someone change the behavior where we always try turn off frontlight and wifi and all those things that can fail suspend regardless of their status or something? Because if so that was all in there on purpose.

This comment has been minimized.

@NiLuJe

NiLuJe May 9, 2018

Member

FL is properly turned off in the device-specific BeforeSuspend, and as for WiFi, AFAICT, that's the bit that made the decision of whether or not to kill WiFi, except the check is not accurate, because wifi_was_on isn't.

This comment has been minimized.

@NiLuJe

NiLuJe May 9, 2018

Member

Meaning that from what I gathered, in the current state of the codebase, while we were always shutting down the FL, we weren't doing so for WiFi, instead relying on this flawed test.

I can't say if that was always the case, though ;).

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 9, 2018

Not quite sure I understand what's going wrong with the CI, since I'm not familiar with the unit tests... :(

local KoboSnowRev2 = Kobo:new{
model = "Kobo_snow",
hasFrontlight = yes,
touch_snow_protocol = true,

This comment has been minimized.

@Frenzie

Frenzie May 9, 2018

Member

I'd just remove any touch protocol adjustments so that the proper one can be additive in the future. Besides which, we know this config doesn't work and the default just might. :-P

This comment has been minimized.

@NiLuJe

NiLuJe May 9, 2018

Member

Yeah, I nearly tried to just go with the original H2O config, just for kicks...

end
end
function Kobo:getProductId()

This comment has been minimized.

@Frenzie

Frenzie May 9, 2018

Member

Should that be a globally exposed function or just local?

Edit: in this case that's a semi-rhetorical question because it looks like it should be local. ;-)

This comment has been minimized.

@NiLuJe

NiLuJe May 9, 2018

Member

Yup, that should be local indeed, will fix!

@@ -4,16 +4,16 @@
killall udhcpc default.script wpa_supplicant 2>/dev/null
[ "${WIFI_MODULE}" != "8189fs" ] && wlarm_le -i eth0 down
[ "${WIFI_MODULE}" != "8189fs" ] && wlarm_le -i "${INTERFACE}" down
ifconfig "${INTERFACE}" down
# Some sleep in between may avoid system getting hung
# (we test if a module is actually loaded to avoid unneeded sleeps)

This comment has been minimized.

@Frenzie

Frenzie May 9, 2018

Member

Oh right, so it was changed but in a clever way. :-) (Cf. earlier comment)

@@ -3,6 +3,8 @@
# Load wifi modules and enable wifi.
lsmod | grep -q sdio_wifi_pwr || insmod "/drivers/${PLATFORM}/wifi/sdio_wifi_pwr.ko"
# Moar sleep!
usleep 250000

This comment has been minimized.

@Frenzie

Frenzie May 9, 2018

Member

le sigh

This comment has been minimized.

@NiLuJe

NiLuJe May 9, 2018

Member

Not quite sure this one changes anything, but since we sleep after rmmod, I figured, why not sleep after insmod, too :D.

@Frenzie

This comment has been minimized.

Member

Frenzie commented May 9, 2018

I don't have time to look today. Most are fairly self-explanatory if you just look at the relevant test code though. ;-)

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 9, 2018

I have basically no idea why it falls on its ass on a getenv (that, granted, is supposed to return nil in this case, but, still).

Any insights on my isWifiOn() query? What happens on devices where it's not set? Does the blank stub evaluates as true or false or nil in lua?

@Frenzie

This comment has been minimized.

Member

Frenzie commented May 9, 2018

As nil. The default return value of a function is always nil. ;-)

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 9, 2018

Okay, so that won't blow up in weird and horrible ways then, good ^^.

NiLuJe added some commits May 9, 2018

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 9, 2018

There, tamed the unit test ;p.

AFAICT, the current failure is not mine, so, :?

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 9, 2018

Random remarks: there's a NetworkMgr:isWifiOn() check in frontend/apps/reader/modules/readerfooter.lua:107 for the WiFi status in the footer, so that's broken by design on everything except Android (and now Kobo).

wifi_was_on being inaccurate is most likely the root cause of the various reports of auto restore wifi not working correctly.
It's possibly also vaguely related to the inaccuracies of the connection status checkbox in the UI, too?

@Frenzie

This comment has been minimized.

Member

Frenzie commented May 9, 2018

Yup, by design, only on Android it's non-blocking.

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 9, 2018

And it's already gated behind an isAndroid() check, now that I actually checked, so 👍 :).

@Frenzie

This comment has been minimized.

Member

Frenzie commented May 10, 2018

@NiLuJe

AFAICT, the current failure is not mine, so, :?

Should you not have permissions to "rebuild"? There is a minor issue that very rarely shows its ugly face. I'm not 100% sure this is it, but the tl;dr is that there's a function that produces Unix time (in seconds) where in rare circumstances the lack of precision causes a test failure. I still haven't made up my mind whether it indicates a deeper problem or not, in the sense that you could at least conceivably tap/execute that function a few times a second in real life just like happens in case of test failure. It's unlikely but not unthinkable.

Do you want to do some manual rebasing into meaningful commits or just squash & merge?

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 10, 2018

@Frenzie Frenzie merged commit e3b7524 into koreader:master May 10, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@cramoisi

This comment has been minimized.

Contributor

cramoisi commented May 16, 2018

@NiLuJe : enabling wifi in KOReader then exit to KSM with the wifi up (then doing things), then with wifi still up returning back to KOReader : the wifi should still be up but the wifi checkbox is unchecked.

But if I'm not wrong, the reverse is not true : enabling wifi in KSM, going to KOReader : the wifi is still up.

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 16, 2018

@cramoisi : We don't actively do anything to network status on startup, we only kill it on exit (and even then, that's only in the 'restart nickel' codepath, which shouldn't be taken w/ KSM, IIRC).
So, at first glance, what you're saying tracks.

I can't vouch for what happens or not with KSM, since I've never used it, but I have sometimes seen the Wi-Fi connection checkbox looses its marbles...

Strange thing is, it doesn't depend at all on the wifi_was_on flag, it instead does a DNS resolution, so it really should always be accurate, unless there's a real temporary network issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment