RECVQueue.poll is either named incorrectly and does something other then the name suggests or it is used incorrectly

[nimrodVarga]

Currently in the develop branch, in the main loop of the base.run method RECVQueue.poll is used like this:

if self.queue.poll() is not None:

However, I fixed this function a long time ago because it used to return some undocumented not_empty attribute of the standard python Queue class which is if I remember correctly an Exception, however, it is not something that changes depending whether or not there is something in the queue and therefore cannot be used to check if something is in the queue. I fixed this by letting poll return self.empty() which does give you information whether or not there is something in the queue and so I changed the call site to:

if not self.queue.poll():

This does the correct thing, however I agree that the name is misleading and this is why I bring this up again because somebody changed it back to the call site above without changing what poll does. Meaning, the condition is always True.

So, one of these 2 things have to change, either change the poll to have the correct name and correct behaviour or call it correctly.

[nimrodVarga]

On further inspection, this change should be accompanied with a responsiveness test. I assume the ACCompanion was mostly tested with the incorrect code, meaning that it did a blocking wait for MIDI messages.
With a non-blocking wait (which I assume was the intenion of this whole if), the responsiveness of the program is likely to change, however, I am not sure it would be necessarily better since then we would introduce a busy wait maybe antagonizing the OS scheduler. This should be tried out by the capable pianists among us who could judge a change. If it makes the responsiveness worse, we should remove the if.

On the other hand, the non-blocking wait does provide the opportunity to sneak in work to do while waiting, however, that is something to be discussed in the future.

[CarlosCancino-Chacon]

So.... I tested the system with both variants (each variant at least 5 times with the Brahms piece), and there does not seem to be that many differences. That being said, there was one time where there was a weird spot using not self.queue.poll() (the tempo/synchronization were not quite right in bars 39-40), but then the system continued correctly.

My gut feeling is that self.queue.poll() is not None is slightly better, but to test this we would need to do some blind testing: the soloists do not know which version they are playing and need to say whether the behavior was ok. Then we can do quantitative tests (i.e., in terms of synchronization) and/or qualitative tests (in terms of selecting which versions sound better).

But for the moment, I would say that we should keep using self.queue.poll() is not None.

[CarlosCancino-Chacon]

I will keep this issue open for the time being, until we do more systematic tests.

[nimrodVarga]

Ok, but then I suggest we just we remove that branch, as I explained, poll returns True or False, meaning it is never None, so the branch is ALWAYS taken.

So, I wouldn't frame this as not self.queue.poll() vs self.queue.poll() is not None,
but rather blocking (no branch) vs. non-blocking (with CORRECT branch) MIDI message reading.

[nimrodVarga]

I am not sure if this should be an additional issue, so for now I am putting this here:

We recently noticed that Ctrl-C doesn't interrupt the program execution properly again and this issue is related with this:
in the correct non-blocking previous version KeyboardInterrupts got handled by the interpreter, but with the incorrect version which ends up being blocking, KeyboardInterrupts DO NOT get handled while the interpreter is blocked on waiting for a MIDI message.

This issue is documented here and here

Meaning, we can't have it both ways, we can't use Queue.get in a blocking way AND use KeyboardInterrupts

However, there is a middleground which I actually implemented in the MidiFilePlayerInterceptPort.poll method, periodically using Queue.get with a timeout gives us a quasi-blocking poll that can be interrupted.