Skip to content
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

RTC: Ignore obviously bogus alarm reads on dodgy RTCs #1669

Merged
merged 5 commits into from
Oct 16, 2023

Conversation

NiLuJe
Copy link
Member

@NiLuJe NiLuJe commented Oct 14, 2023

We've had a couple reports over the years of broken alarm reads on old NTX boards (in... every sense of the word; this only seems to affect old i.MX RTCs, true, but more specifically really old devices, with possibly dying batteries).

e.g., koreader/koreader#7994 & koreader/koreader#10996

While there is an ioctl that is supposed to help with this sort of stuff by reporting on the state of RTC's battery voltage, in my own testing on much less broken RTCs, it was extremely unreliable (especially when it matters most, i.e., right after a wakeup), so, that's kind of a no-go.

Thankfully, when this occurs, the returned alarm is extremely obviously bogus: 1, as in, Epoch.

TL;DR: Just ignore such return values and assume the alarm did indeed fire properly, we already validate against both the task and the current time, double-checking the actual alarm is just a defensive and pedantic guard against... something... setting alarms behind our back, which should never really happen in the first place, least of all on the affected platform (Kobo) ;).

Also actually implement honoring WakeupMgr's character device selection, in case we ever need to actually use something other than rtc0 ;).


This change is Reviewable

ffi/rtc.lua Outdated
-- On dodgy RTCs, some aging batteries are (supposedly) causing reads to report a bogus value...
-- c.f., https://github.com/koreader/koreader/issues/7994 & https://github.com/koreader/koreader/issues/10996
if self.dodgy_rtc and alarm_sys <= 1 then
print("Read back a bogus alarm value from a dodgy RTC, assuming our previously set alarm fired as expected anyway")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The phrasing seems a little unclear to me (if I saw it in the logs). Maybe the other way around? "A dodgy RTC…"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done ;).

FWIW, it's never shown in a vacuum, it's part of the whole validate... block, e.g.,

validateWakeupAlarmByProximity:	
task              @ 1697239968 (2023-10-14 01:32:48 +0200)	
last set alarm    @ 1697239968 (2023-10-14 01:32:48 +0200)	
current rtc alarm @ 1 (1970-01-01 01:00:01 +0100)	
current rtc time is 1697239983 (2023-10-14 01:33:03 +0200)	
current time is     1697239983 (2023-10-14 01:33:03 +0200)
Read back a bogus alarm value from a dodgy RTC, assuming our previously set alarm fired as expected anyway

(That was yet another older version of said message, though ;p).

@yparitcher
Copy link
Member

Did you test that this doesn't interfere with the kindle mock rtc implementation.

@NiLuJe
Copy link
Member Author

NiLuJe commented Oct 15, 2023

Did you test that this doesn't interfere with the kindle mock rtc implementation.

Untested, but it shouldn't be affected, as it has its own validateWakeupAlarmByProximity implementation (and the extra fields being set in koreader/koreader#11010 ought to be harmless, too) ;).
That's the only method with actual functional changes, the rest is mostly cosmetics ;).

(Said changes also happen to be gated behind a dodgy_rtc flag, which is only ever set on (a subset of) Kobo, so, even without a custom imp, it wouldn't be affected).

Copy link
Member Author

@NiLuJe NiLuJe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Frenzie)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants