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

Update timespec overflow implementation #81

Merged
merged 3 commits into from
Dec 7, 2021

Conversation

LinusCDE
Copy link
Contributor

This was raised by @fenollp when reviewing the rust-impl of this client.

canselcik/libremarkable#78 (comment)

The code wouldn't work properly if SEM_WAIT_TIMEOUT would be greater
than one second, since only one overflow is accounted for with an "if".

Using modulo here instead of while so no branching is needed and it
would scale linearly not not require one loop per overflown second.

(I didn't use 1e9 since it's a double and I'm not sure if it would be perfectly accurate in C++, feel free to change if you don't mind.)

This was raised by @fenollp when reviewing the rust-impl of this client.

canselcik/libremarkable#78 (comment)

The code wouldn't work properly if SEM_WAIT_TIMEOUT would be greater
than one second, since only one overflow is accounted for with an "if".

Using modulo here instead of while so no branching is needed and it
would scale linearly not not require one loop per overflown second.
@LinusCDE
Copy link
Contributor Author

Never mind, changed it back to using 1e9 with casting.

I'm a bit confused as of which impl is used. If glibc/frida's is used over the default one, it should be fine.

screenshot4328

The default one according to my (x86_64) linux headers seems like it would be 32 bit on

grafik

This would still cause a native overflow issue for delays bigger than ~4.2s as this overflows the 32 bit long on the device.

grafik

We will probably not run into an issue in the foreseeable future, but it should still be noted as a possible problem in the implementation.

@LinusCDE
Copy link
Contributor Author

Edit: The glib/frida impl (glong) actually uses a normal long as well (32 bit) and hence would cause the problem as well.

@LinusCDE
Copy link
Contributor Author

LinusCDE commented Nov 30, 2021

(The issue could also affect the rust impl as well since it seems that c_long is used for tv_nsec on 32 bit non x86_64 systems)

grafik

@alistair23
Copy link

tv_nsec should always be a long, see https://linux.die.net/man/3/clock_gettime

The x32 ABI is a strange exception that other architectures don't use.

@alistair23
Copy link

Also, it's probably just better to use timeradd to add these: https://man7.org/linux/man-pages/man3/timeradd.3.html

@raisjn raisjn merged commit 1cc972b into ddvk:master Dec 7, 2021
@raisjn
Copy link
Collaborator

raisjn commented Dec 7, 2021

@alistair23: side question: for the rM1, we think that the wacom driver is drawing power while the device is in deep sleep. would you happen to know how to disable / send the wacom driver (on the i2c bus) a suspend signal or otherwise disable it? (or how to confirm if it is drawing power?) thanks!

@alistair23
Copy link

I'm not sure sorry

@LinusCDE
Copy link
Contributor Author

LinusCDE commented Dec 7, 2021

Also, it's probably just better to use timeradd to add these: https://man7.org/linux/man-pages/man3/timeradd.3.html

Sorry for the late response. This seems probably like the best way. But the current solution should be fine for now. Goes to demonstrate how many levels of better exist for such seemingly a simple task.

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.

4 participants