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

Replace the queued communications with deferred delays. #34

Conversation

gerph
Copy link
Contributor

@gerph gerph commented Sep 1, 2022

The queued communications was, I believe, intended to ensure that
different parts of the system could issues the commands to the
display asynchronously and not have them trample on one another.
However, it was not effective with my flagship device, and almost
never produced an uncorrupted display.

The queue seems unnecessary, and complicates the logic of the
display communications. It has been removed, so that the operations
all happen when they are requested by clients, rather than being
delayed by an arbitrary amount of time by being queued. Instead,
all the operations have been protected by a mutex, similar to the
prior code, but surrounding the entire operation that must be
sequenced in order.

My own investigation implied that there needed to be a delay
between the bitmap data and the next command sent. It's not clear
why, but with this delay in place, the results were never corrupted.

Inserting the delay immediately after the bitmap data send would
work to allow the data to be sent uncorrupted, but it would force
there to always be a delay even when there would be time between
the bitmap and the next command. Instead the delay is deferred
until we try to send the next command - at which point we will
sleep. This means that the delay is amortised into subsequent
work.

The results are solid on my flagship model with a 0.02s delay. It
is possible that this is high, and it could be reduced. It is
possible that this does not work on model A devices, but I cannot
test this at this time. The necessary changes have been made to
the code, but I just have to hope they work.

Here's it working (tested 4 times to be sure):

WorkingTuringDisplay.mp4

The queued communications was, I believe, intended to ensure that
different parts of the system could issues the commands to the
display asynchronously and not have them trample on one another.
However, it was not effective with my flagship device, and almost
never produced an uncorrupted display.

The queue seems unnecessary, and complicates the logic of the
display communications. It has been removed, so that the operations
all happen when they are requested by clients, rather than being
delayed by an arbitrary amount of time by being queued. Instead,
all the operations have been protected by a mutex, similar to the
prior code, but surrounding the entire operation that must be
sequenced in order.

My own investigation implied that there needed to be a delay
between the bitmap data and the next command sent. It's not clear
why, but with this delay in place, the results were never corrupted.

Inserting the delay immediately after the bitmap data send would
work to allow the data to be sent uncorrupted, but it would force
there to always be a delay even when there would be time between
the bitmap and the next command. Instead the delay is deferred
until we try to send the next command - at which point we will
sleep. This means that the delay is amortised into subsequent
work.

The results are solid on my flagship model with a 0.02s delay. It
is possible that this is high, and it could be reduced. It is
possible that this does not work on model A devices, but I cannot
test this at this time. The necessary changes have been made to
the code, but I just have to hope they work.
@gerph gerph force-pushed the replace-queued-data-send-with-immediate-plus-deferred-delay branch from 17abf80 to 8a52204 Compare September 1, 2022 22:33
@mathoudebine
Copy link
Owner

Hi @gerph
Thanks again for this work and for testing it on macOS!
I will test it with different revisions and OS, and hopefully merge it soon.

@mathoudebine mathoudebine changed the base branch from feature/system-monitor to fix/replace-queued-data-send-with-immediate-plus-deferred-delay September 19, 2022 20:24
@mathoudebine mathoudebine merged commit 7a0a881 into mathoudebine:fix/replace-queued-data-send-with-immediate-plus-deferred-delay Oct 10, 2022
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.

2 participants