pfps / yoga-laptop

Systems and information to make Lenovo Yoga laptops work better
GNU General Public License v3.0
156 stars 30 forks source link

Possible Error in code of orientation #36

Open z0mb0lix opened 9 years ago

z0mb0lix commented 9 years ago

Hey pfps.

I am almost certain I found a small error in your orientation.c code that prevents the auto rotation:

406 if (previous_orientation == orientation /* only rotate when stable */ && orientation != 407 screen_orientation && orientation != FLAT && !orientation_lock) { 408 rotate_to(orientation); 409 previous_orientation = orientation; 410 411 }

In line 406 the dependencies to execute the rotate_to - section are defined as

if (previous_orientation == orientation /* only rotate when stable */ && orientation != screen_orientation && orientation != FLAT && !orientation_lock)

If I understand it correct, the value of previous_orientation is defined as "-1" in the header section and gets a new value at two positions: 1) after the dependencies above are met and rotate_to sequence is executed (Line 409) it gets the same value as orientation, 2) during the sigusr_callback_handler sequence (Line 225) it gets the same value as screen_orientation.

BUT, here comes the clue: screen_orientation also only gets his value in the rotate_to sequence. But that prevents the section from actually running because the algorithms to set the values to fulfil the dependencies are only executed AFTER the dependencies are fulfilled. It SHOULD (imo) set the value of previous_orientation after each time the check orientation process is finished.

So.. long story short: I think the previous_orientation=orientation command should be taken out of the sequence above and placed as a dependencie-free command behind the rest

Something like this: 406 if (previous_orientation == orientation /* only rotate when stable */ && orientation != 407 screen_orientation && orientation != FLAT && !orientation_lock) { 408 rotate_to(orientation); 409 410 } 411 previous_orientation = orientation;

I tried it with this code and if "run sequence once" - write previous_orientation at the end after all other commands - "run sequence again" - previous_orientation=orientation at the beginning - Hurray!

Greetings

Felix

pfps commented 9 years ago

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1

I'll take a look shortly.

peter

On 04/16/2015 05:03 AM, z0mb0lix wrote:

Hey pfps.

I am almost certain I found a small error in your orientation.c code that prevents the auto rotation:

406 if (previous_orientation == orientation /* only rotate when stable */ && 407 orientation != screen_orientation && orientation != FLAT && 408 !orientation_lock) { 409 rotate_to(orientation); 410 previous_orientation = orientation; 411 }

In line 406 the dependencies to execute the rotate_to - section are defined as

if (previous_orientation == orientation /* only rotate when stable */ && orientation != screen_orientation && orientation != FLAT && !orientation_lock)

If I understand it correct, the value of previous_orientation is defined as "-1" in the header section and gets a new value at two positions: 1) after the dependencies above are met and rotate_to sequence is executed (Line 409) it gets the same value as orientation, 2) during the sigusr_callback_handler sequence (Line 225) it gets the same value as screen_orientation.

BUT, here comes the clue: screen_orientation also only gets his value in the rotate_to sequence. But that prevents the section from actually running because the algorithms to set the values to fulfil the dependencies are only executed AFTER the dependencies are fulfilled. It SHOULD (imo) set the value of previous_orientation after each time the check orientation process is finished.

So.. long story short: I think the previous_orientation=orientation command should be taken out of the sequence above and placed as a dependencie-free command behind the rest

Something like this: 406 if (previous_orientation == orientation /* only rotate when stable */ && 407 orientation != screen_orientation && orientation != FLAT && 408 !orientation_lock) { 409 rotate_to(orientation); 410 } 411 previous_orientation = orientation;

I tried it with this code and if "run sequence once" - write previous_orientation at the end after all other commands - "run sequence again" - previous_orientation=orientation at the beginning - Hurray!

Greetings

Felix

— Reply to this email directly or view it on GitHub https://github.com/pfps/yoga-laptop/issues/36.

-----BEGIN PGP SIGNATURE----- Version: GnuPG v2

iQEcBAEBAgAGBQJVL8WtAAoJECjN6+QThfjzSFMH/2B2Dst2s2UtsHw2aqid/WE2 RvFAa7nuIj+3BkutoFJ5AfP8jcHwSf7ViL8yo64ytsOHjQhN1Tc68ejJZWKonCIS JCIFJQDCRF/4Z1gtUDPoQJT+a8jZkX2dXuwy/y+acBf8XUlGh4zTscnGqHQUvjDT f1he7/F+VBuqpS1iWkjYJbSzBqZBi8+0EA/cOvjozj/6GLzFQKIRzogRUm85mYgV rf64mQwvhsn+ZJpXauOJG90dPTV2iSa95UaIvFHHa9bltyLiUsD4z84Z7zcFO3w5 CyevGNpwH7Ze5dSru27ADobBvN3qyhTBSS7FGHoc/iG5fjCDSJUrbPhE3cZ7qOo= =yyez -----END PGP SIGNATURE-----

pfps commented 9 years ago

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1

Yes, your analysis is correct.

I wonder how that bug got into the code.

I made the change, and pushed it back to the repository.

Thanks for doing the hard work to figure out what was going wrong.

I also pushed a change to where some keyboard stuff is put, as Fedora 21 changed its location.

peter

On 04/16/2015 05:03 AM, z0mb0lix wrote:

Hey pfps.

I am almost certain I found a small error in your orientation.c code that prevents the auto rotation:

406 if (previous_orientation == orientation /* only rotate when stable */ && 407 orientation != screen_orientation && orientation != FLAT && 408 !orientation_lock) { 409 rotate_to(orientation); 410 previous_orientation = orientation; 411 }

In line 406 the dependencies to execute the rotate_to - section are defined as

if (previous_orientation == orientation /* only rotate when stable */ && orientation != screen_orientation && orientation != FLAT && !orientation_lock)

If I understand it correct, the value of previous_orientation is defined as "-1" in the header section and gets a new value at two positions: 1) after the dependencies above are met and rotate_to sequence is executed (Line 409) it gets the same value as orientation, 2) during the sigusr_callback_handler sequence (Line 225) it gets the same value as screen_orientation.

BUT, here comes the clue: screen_orientation also only gets his value in the rotate_to sequence. But that prevents the section from actually running because the algorithms to set the values to fulfil the dependencies are only executed AFTER the dependencies are fulfilled. It SHOULD (imo) set the value of previous_orientation after each time the check orientation process is finished.

So.. long story short: I think the previous_orientation=orientation command should be taken out of the sequence above and placed as a dependencie-free command behind the rest

Something like this: 406 if (previous_orientation == orientation /* only rotate when stable */ && 407 orientation != screen_orientation && orientation != FLAT && 408 !orientation_lock) { 409 rotate_to(orientation); 410 } 411 previous_orientation = orientation;

I tried it with this code and if "run sequence once" - write previous_orientation at the end after all other commands - "run sequence again" - previous_orientation=orientation at the beginning - Hurray!

Greetings

Felix

— Reply to this email directly or view it on GitHub https://github.com/pfps/yoga-laptop/issues/36.

-----BEGIN PGP SIGNATURE----- Version: GnuPG v2

iQEcBAEBAgAGBQJVL+E6AAoJECjN6+QThfjzDakIAJve45eau9EvjgaMrOsWu5Vd EhCyoh0El3zuounj7syXILanZBls6mMJScAkiN0MMAlHnvyD11HrVSTD34fY2Iv+ gH0OKm5mEbjASNtFexbGpfDNvtsUb0WLRw0rplKhgwrYsVDUhRMPbPFiaoYLrl14 b93kt6nlQ3oLqrcKJ8V6tKlK9pW+GyaKUSovU0czZnp/OnNl7Yviyo66Z93F+Cva oDGRfMHpMXnS5coXDEnt09QTaiekVkofklRKFIlKeRmQ9PmtKhKpN6ca1EN5BEbn l4jaRsJYQLfk0mPc/ZBtQn3r7Hc53FgNcLIEo7fpr+76NRlGIB0TxMlIHTBPiXg= =n4EF -----END PGP SIGNATURE-----