-
Notifications
You must be signed in to change notification settings - Fork 131
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
Second change synchronization #103
base: master
Are you sure you want to change the base?
Conversation
Sorry for the trouble and making you wait, here's a single-commit pull request 🙂 |
Please delete 101 and 102 |
The behavior now triggers for every whole second delay >= 1 |
hi sorry to be annoying about this, but could you provide a screenshot or video showing the before and after behavior of this? here's an example of what it currently looks like on my machine: ... which you correctly identified as incorrect. it's actually pretty hard to get the timing right for the screenshot (or, to put it another way, it's easy to make a screenshot that doesn't show the bug!), so a video is better: Peek.11-01-2022.10-41.mp4i captured this with peek by moving the capture window over the right area and hitting control-alt-r twice(because the start/stop button was hidden). here we can see that xclock and my status bar (py3status) are perfectly synchronised (as far as we can tell) but that tty-clock is not synchronized with itself or xclock and the status bar... can you show that your MR fixes that? |
I'll go and arrange my terminal windows 🙂 |
I think I noticed a bug btw... The initial second is rounded, which should only happen after synchronized update. |
screen_recording.mp4 |
Bug verified manually, fixing... |
so one thing that recording is missing is a reference clock, e.g. xclock or your desktop clock. |
Fixed and pushed to PR 103 🙂 |
Oh, I just realized that your video included an external view... Just a moment |
screen_recording_with_date.mp4 |
Bottom right: |
Was slightly visibly behind on first run because 0.1 is definitely visible, second run almost on par with tty-clock, whcih of course has its own tiny lag due to ncurses but probably nothing visible in this app 🙂 |
after_reinstalling_from_homebrew.mp4 |
Re-installed from homebrew and re-started the instances 🙂 |
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 for bearing with me all this time, and for providing the screencast, it does look like an improvement, but i'm not sure it's quite there yet. particularly, i don't understand how the code works so i'm hesitant in merging it as is... comments could help.
also, did you look at the xclock code? it does some pretty interesting arithmetic to figure out the write time to show and the right time to sleep: https://sources.debian.org/src/x11-apps/7.7+6/xclock/Clock.c/#L1251 |
I guess clang is more strict about multiple sources, when gcc would just ignore headers... The gcc frontend of clang, of course, most likely emulates classic gcc's behavior, thus ignores headers being passed as sources 🙂 |
I did indeed lock into xclock's source code as you suggested 🙂 It uses rounded time for display. The code is a bit... No, the code is way less readable than that of tty-clock. But when it comes to displaying, it also just rounds to the closest second, probably due to the same reasoning of when the timer somewhat accurately hits a second change, that's the obvious option. I'll also have a shot at discovering the timer code, which unfortunately is sitting there in a wild mix of all things X Windows and behavior 😂 I've seen much worse, though, because I've worked with the code base of NTP, which seems to have evolved over decades by assigning new students to open issues year after year, all with their own code styles 🙂 |
I hope the last commit makes the implementation more readable 🙂 |
The initial second is now correct while the code seems in fact a bit less complex now |
Reverted and pushed. I will create a separate pull request with a single commit and explanation for removing the header from SRC |
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'm sorry, but i'm still confused by the changes you propose. usually, i'd find answers as to the "why" of things in the commit logs, but those don't seem to provide much more information.
you will notice as well that the commits don't show up quite well in github: the content gets broken up in two, and that's because you are breaking the convention of keeping the first line of the commitlog to 50 characters, with a longer explanation below...
sorry for nitpicking on this, but i am truly confused now of the resulting changes, so it would help to have more clarity in the commit history...
fb4bb27
to
5b71464
Compare
Reduced the branch to a single commit 🙂 |
gotcha. there are still pending comments though. |
Fixed unnecessary changes and added clarifying comment as requested |
0c98681
to
010fd5a
Compare
Just squashed and force pushed, now it's one single commit again after cleanup. |
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.
almost there.
@@ -174,7 +174,6 @@ update_hour(void) | |||
int ihour; | |||
char tmpstr[128]; | |||
|
|||
ttyclock.lt = time(NULL); |
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 think there's still a problem here: we remove the lt
initialization which means lt
will be undefined in the struct, which can lead to undefined behaviour (UB) which we never want. let's set it to something, even if we overwrite it later, it won't hurt, right?
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.
Btw. the reason to not have it here is because without some kind of ugly flag, update_hour doesn't know whether this is the first time the timestamp is fetched, in which case rounding it is generally prohibited because the initial display of time must show the non-rounded time at startup.
@@ -256,7 +255,7 @@ draw_clock(void) | |||
draw_number(ttyclock.date.hour[0], 1, 1); | |||
draw_number(ttyclock.date.hour[1], 1, 8); | |||
chtype dotcolor = COLOR_PAIR(1); | |||
if (ttyclock.option.blink && time(NULL) % 2 == 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.
... specially since we're using it here directly, probably before initialization!
@@ -679,11 +678,89 @@ main(int argc, char **argv) | |||
update_hour(); | |||
draw_clock(); | |||
key_event(); | |||
ttyclock.lt = timestamp(); |
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.
maybe initializing it earlier would get rid of this seemingly stray instruction as well?
{ | ||
struct timespec result; | ||
|
||
// avoiding sleeping for < 1 / 10 of a second |
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.
nice try, but "why?" why do we avoid sleeping for a tenth? not worth it? how do we handle it in that case?
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 amend the comment
If this wasn't a widely used very public project,I would simply refactor it into smaller functions, slice everything differently and be done with it quite quickly 🙂
|
About the 9/ 10ths of a second: there is always a chance of inaccuracies in both sleep and getting the time. |
Two observations about the initialization of ttyclock.lt:
Conclusion: the behavior is well defined and correct but:
|
So I will will make these three changes:
|
I also just discovered a race condition: when starting within the last 1 / 10 of a second, the initial second won't get updated for one second and thus go immediately from e.g. :26 to :28. I will introduce an argument for sleep_time() in order to not skip one second from within the last 1 / 10 when called from init(). I even remember having done that in all these old implementations now 😂 |
so i suspected as much. the point is not to explain to me, here and now, the logic. the point is to explain it in the code, or at least in the commit log. |
i guess i worry about what an empty ttyclock_t evaluates as. :) better to be explicit, IMHO. but i worry about this only because of the change in the code, if there's no change, then there's no problem. :) |
ttyclock.lt is a UNIX timestamp, even on Windows. But I understand your wish to be explicit about initialization, as some day, somebody will certainly compile it against BeOS with some weird POSIX layer that think it conforms to standards but doesnt't because it's maintained by the proverbial single guy in Nebraska. I'll attempt to create a commit that takes all our considerations into account 🙂 But not today, we're going skiing/snowboarding tomorrow, keyboard time is bad for the shoulder muscels, keeping it low today 🙂 Je te souhaite un bon weekend! 🙂 |
On 2022-01-15 09:35:37, Sebastian Bohmann wrote:
> > 1. memset(&ttyclock, 0, sizeof(ttyclock_t));
>
> i guess i worry about what an empty ttyclock_t evaluates as. :) better to be explicit, IMHO. but i worry about this only because of the change in the code, if there's no change, then there's no problem. :)
ttyclock.lt is a UNIX timestamp, even on Windows. But I understand your wish to be explicit about initialization, as some day, somebody will certainly compile it against BeOS with some weird POSIX layer that think it conforms to standards but doesnt't because it's maintained by the proverbial single guy in Nebraska.
I'll attempt to create a commit that takes all our considerations into account 🙂
But not today, we're going skiing/snowboarding tomorrow, keyboard time is bad for the shoulder muscels, keeping it low today 🙂
that sounds like a great idea, take your time. :)
Je te souhaite un bon weekend! 🙂
(Ou est-ce q'on dit "fin-de-semaine" en Quebec? 🤔 Mon francais est entirement Européen, et ici c'est presque toujours "weekend" 🙂)
les deux marchent. :)
…--
Dr. King’s major assumption was that if you are nonviolent, if you
suffer, your opponent will see your suffering and will be moved to
change his heart. He only made one fallacious assumption: In order for
nonviolence to work, your opponent must have a conscience. The United
States has none. - Stokely Carmichael
|
I haven't capitulated btw. the skiing and snowboarding thing just has filled the last couple of weekends 😅 |
awesome, keep going. we'll be around for a while, but maybe not the snow. :) |
Synchronization with second change when delay is one second and nsdelay is zero.
When displaying time under these circumstances, a timestamp rounded to the closest second is used.