Fix Hanging On Input Read#15
Open
Kovak wants to merge 5 commits into
Open
Conversation
Collaborator
thbar
reviewed
Jun 22, 2019
| messages = do_read(stream, @buffer_size) | ||
| Server.new_messages(server, messages) | ||
| case do_read(stream, @buffer_size) do | ||
| {:error, reason} -> Logger.debug("Error Reading Midi: #{reason}") |
Collaborator
There was a problem hiding this comment.
Just a thought - it would be nice to design a way to let the caller specify the behaviour when such a message happens (e.g. a configurable callback of some sort), maybe with a default implementation?
|
I use Kovak's version. not that I really tested it but it seems to be more stable. what holds you back from merging the pull request? |
This was referenced May 9, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Just started working with this lib for a project I was on and was having segfaults, buffer overflows, and general hanging when trying to read my keyboard. Turns out there were a couple things wrong:
When reading from midi, bufferSize was incorrectly being looked up as the third arg in a two arg list. In addition it was being decoded using the c -> erl encoding (enif_make_*) instead of the erl -> c (enif_get_*). The type was also being used incorrectly, should be a long instead of an int I believe.
Pm_Read was being used incorrectly. It can return negative values if it errors out, these were not being handled and were just being used as if they were positive integers, resulting in massive allocation requests and segfaults when the array was then created with a near max int number (from the negative number wrapping around as an unsigned int). These errors are now properly caught and passed out to be logged.
Even when the above 2 issues were fixed, the lack of a sleep in between polling loop was locking up my device. I saw another PR for this but I didn't like how it did the sleep in C-space as A. the usleep function is not guaranteed to be in every platform, and B. It makes the nif more complicated and instead of just exposing the portmidi API introduced client-side logic to it. I have instead used elixir's :timer.sleep and exposed the amount to sleep as configurable. In my testing even a 1 ms sleep seems adequate.