search

astro-pi / python-sense-hat

Source code for Sense HAT Python library
https://sense-hat.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
508 stars 255 forks source link

Added timeout parameter to wait_for_event in stick.py #56

Closed diningphil closed 8 years ago

diningphil commented 8 years ago

wait_for_event method can set a timeout after which it will return, by taking an additional default parameter called "timeout".

Why is this useful:

davidhoness commented 8 years ago

@waveform80 wasn't there a reason we left this out?

diningphil commented 8 years ago

I opened the pull request because I noticed that since wait() method has been designed to return false if the timeout expires, giving it the possibility to actually return false seems legit to me. Moreover, adding a default parameter does not invalidate everyone's code. But I'm just a student willing to learn from my mistakes, so.. Hope this can help.

waveform80 commented 8 years ago

Ah, I'm afraid the reason this isn't included is because the implementation is not that simple. You've not considered the case where _read returns None because the event in the queue was a non-key event. Walking through your implementation:

  1. We enter the top of the while loop and wait timeout seconds for an event to arrive.
  2. A non-key event arrives which _read() returns.
  3. We return to the top of the loop and again wait timeout seconds for an event to arrive. Note at this point we've potentially waited longer than timeout seconds, and that this can go on for as long as non-key events arrive.

It is possible to add a timeout to this method, but it's non-trivial. Also, as you've pointed out the major use-case for this is to permit threads to avoid blocking indefinitely for user input. However, my thinking was: if you want to use threads, why not use the callback mechanism provided in which case you get the background thread for free? Given the availability of the callback mechanism, and that this was the only major use-case I could come up with for wait-with-timeout, I decided to keep the code simple and exclude it in my original PR.

diningphil commented 8 years ago

You are right, I did not put enough attention to that details. Thanks for your suggestion!