-
Notifications
You must be signed in to change notification settings - Fork 104
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
[feat] Add FFI RTC interface #969
Conversation
This will allow for scheduling wakeups. Prerequisite for <koreader/koreader#5335>.
wake.enabled = enabled and 1 or 0 | ||
|
||
local err | ||
local rtc0 = C.open("/dev/rtc0", bor(C.O_RDONLY, C.O_NONBLOCK)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, I forgot to make these configurable. Tbh it may just be a waste of time to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rtc path, you mean?
Yeah, probably not worth it ;p.
ffi/rtc.lua
Outdated
|
||
local RTC = { | ||
_wakeup_scheduled = false, | ||
_wakeup_scheduled_ptm = nil, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ptm = ? (I guess it's a pointer to a "tm" :) but some comment might help getting into all that (I know nothing about that, so... discovering/learning)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do. Btw, it's clearer if you look at rtc_h.lua.
ffi/rtc.lua
Outdated
end | ||
|
||
if re == 0 then | ||
if enabled then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you go on doing things above even if the first one failed? expecting all the others will fail so you still get re==-1
in the end?
I would expect a return nil, re, err
in each of the ifs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although technically the close doesn't necessarily mean it's not scheduled. (Not that the current code makes that distinction.)
wake.time.tm_yday = -1 | ||
wake.time.tm_isdst = -1 | ||
|
||
wake.enabled = enabled and 1 or 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to disable it, you can provide any time value, including 0? (= it doesn't matter?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, setting it to a time in the past would effectively disable it, and I don't think I actually tried what happens when you combine disabled with a time in the future. It can probably be removed.
end | ||
|
||
--[[-- | ||
Get RTC wakealarm from system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does that mean?
Is it an alternative/different alarm?
Or just to get a previously set alarm, by another process or a previous run of KOReader?
And do we need to clear any alarm set on exit/crash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RTC stands for Real Time Clock. It's the physical crystal-based, battery-powered ticking clock responsible for a good part of the time-keeping (as far as "wall-clock", i.e., real word time) is concerned ;).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I knew the thing :)
I was wondering about the notion of "system alarm" (the one we fetch from rtc0) vs "our alarm" (that we store as an attribute) here :) which I ended up understanding after reading the whole file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add some explanation in the function description (+ internal references).
if not (alarm_epoch == alarm_sys_epoch) then return end | ||
|
||
-- If our stored alarm and the provided task alarm don't match, | ||
-- we're not talking about the same task. This should never happen. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless 2 features/plugins set each an alarm. This will happen to the oldest plugin that did, no?
(if task_alarm_epoch should be the same used to set the alarm, and that the plugins should store so they can provide it to this function later?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could happen if a rogue plugin avoided the WakeupMgr:addTask/removeTask mechanism, in which case the check could prevent WakeupMgr from running its top task. (However, I haven't currently implemented that in front.)
I'll merge this now [edit: after fixing the luacheck issues] since this syscall stuff should be fairly stable, plus I've gone over several nights of sleep since first writing it. @NiLuJe also directly assisted with the implementation of the first iteration, from which it hasn't substantially changed. Then if you could please direct your critical eye to koreader/koreader#5335 I'd be much obliged. :-) |
koreader/koreader-base#971 Fixes <#5347 (comment)>. Also includes: * [feat] Add FFI RTC interface (<koreader/koreader-base#969>) * Bump thirdparty/djvulibre to current master (<koreader/koreader-base#970>)
function RTC:setWakeupAlarm(epoch, enabled) | ||
enabled = (enabled ~= nil) and enabled or true | ||
|
||
local ptm = C.gmtime(ffi.new("int[1]", epoch)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NiLuJe This was my new simplified approach (without a whole t = time_t etc.) but apparently I accidentally made the module ARM-specific because on my computer it errors saying it should be uint_64. 🤦♂️
Anyway, not an issue for the initial Kobo implementation but I figured I'd leave a note. I'll (hopefully) look into recomplexifying it after the finishing touches on the frontend implementation. Which I expected to have finished already but I had to rush that MuPDF patch and stuff. :-P
@Frenzie: That's because time_t is actually a long int (c.f., What you're doing here should work just fine with |
@NiLuJe I think there might've been TZ complications. Either way, I won't look at it today. |
I don't see how that could be the case, there's no magic involved, time_t is just a typedef: typedef long int __time_t;
typedef __time_t time_t; So local ptm = C.gmtime(ffi.new("time_t[1]", epoch)) is essentially doing time_t foo = epoch;
struct tm* ptm = gmtime(&foo); (Actual storage for what ptm points to is allocated as a static by the libc, i.e., subsequent calls will re-use it, hence the _r variant for thread-safety where it's the caller's job to pass a storage location, not that it's relevant here ;p). |
Cf. <koreader#969 (comment)>. Otherwise the interface won't work on… platforms where it doesn't matter, but it's better to be correct. ;-)
Cf. <#969 (comment)>. Otherwise the interface won't work on… platforms where it doesn't matter, but it's better to be correct. ;-)
koreader/koreader-base#971 Fixes <koreader#5347 (comment)>. Also includes: * [feat] Add FFI RTC interface (<koreader/koreader-base#969>) * Bump thirdparty/djvulibre to current master (<koreader/koreader-base#970>)
This will allow for scheduling wakeups.
Prerequisite for koreader/koreader#5335.