jasperproject / jasper-client

Client code for Jasper voice computing platform
MIT License
4.53k stars 1.01k forks source link

Signaling #145

Open the01 opened 10 years ago

the01 commented 10 years ago

I am playing around with an idea and wanted to hear your thoughts on this.

The way jasper gets its command is not very flexible (aka Conversation.handleForever() . You first call mic.passiveListen and then mic.activeListen. But what if you had multiple mics or something that only required the activeListen part? You would either have to rewrite handleForever() or write a new mic that wraps around two other mics. So what if we were to use something like signaling? I've only got my windows maschine right now, so I couldnt demonstrate it with jasper directly, but I wrote a gist to show what I had in mind.

Feedback would be greatly appreciated!

Lbatson commented 10 years ago

I like it. Being able to register for signals and then emitting the signals you want as well is a nice way to make a more flexible system. As I'm working on the json-rpc #143 I can't find a convenient way to hook into the existing conversation/brain to send my inputs. Based on this I could start the server in the main (which could be configurable via profile.yml) and then signal the conversation delegateInput to handle this. One thing though is there should probably be a queue of instructions with the signals in case many are being sent simultaneously so they get handled in order. There could be priority for them as well in case one needs to override or happen sooner etc.

the01 commented 10 years ago

json-rpc was primarily what I had in mind when I wrote about two mics. I agree that signals should work in a FIFO-order. Actually Blinker does that. However it is not asynchronous, because Signal.send() blocks. It calls all the receivers one by one and returns, after they have finished, with their return values. It would not be very difficult to put the signal into a Queue or even PriorityQueue, have a thread poll the queue and then have the blocking call to the receivers there. Launching a separate thread for every Signal would be a bit overkill though. The downside is of course that we loose the return values from the receivers. The key question is whether we need this feature or not?

Lbatson commented 10 years ago

Yea i don't know that it would need it necessarily. If Blinker already handles these in order then that was my main concern. The prioritization idea was for something like jasper_module_stop needing to be moved forward in order to kill the other signals and stop everything etc.

the01 commented 10 years ago

I have updated my gist with a simple threaded signal dispatcher implementation

Lbatson commented 10 years ago

awesome. definitely solves some issues. @crm416 have you had a chance to take a look this yet?

Holzhaus commented 10 years ago

@the01 I've had a look at it and it's more or less exactly what I had in mind for this.

Just some points:

  1. I'm not convinced that we need the extra dependency (Blinker). It seems to be unmaintained (It has not been updated in over a year). Can't we archieve the same by making the Dispatcher-class a Singleton that holds the queue and looks up signals inside a dict where every key holds a list of receivers?
  2. There should be some common interface for signal senders/receivers (using abc.ABCMeta as metaclass and abc.abstractmethod as method decorator). Senders/Receivers should inherit (and overwrite) these methods.
  3. I'm not sure how this will work with issue #136.
the01 commented 10 years ago

@Holzhaus

  1. The main point of the gist was just to get my thoughts across and blinker was just the first and easiest library that allowed me to do just that. That being said, after a quick glance at the code, blinker actually does what you are describing. Regarding the inactivity, even though it has been a year, there are no real open issue (The only ones are feature request). I think flask uses it too.
  2. I like interfaces, but I am not exactly sure, which methods you want to make abstract. Like a general send/receive funtion (ala emit())? Or are you thinking of fixed functions for sending/receiving e.g. start/stop/transcribe/handle/..what ever signal? The last one works fine for a limited amount of a few signals, but it probably gets messy soon as the numbers increase. 1./2. Of course we could write something ourselves, but usually it is not the best approach to reinvent the wheel. However it sure is fun and I think I did something similar for another project. Maybe I can dig it up somewhere..
  3. Signals can be used pretty much like functions. The difference (functional) is that it is not a direct call and might go to multiple or no receivers. Instead of the loop, you could have a function with the signal for "event_flag_set" and make the audio stream a class variable. Then either open/close the stream on construction/destruction of the instance or just hook up a "start"/"stop" signal (like jasper_module_stop).
Holzhaus commented 10 years ago

Ok, in this case I'm fine with the extra dependency.

the01 commented 10 years ago

@Holzhaus And in the end we can always swap out blinker for a custom implementation if we need/want to. There will be a wrapper around it anyway, so it should be possible without changing any module code. In that regard, could you explain in more detail how you would propose the inheritance for sender/receiver should look like?

I will wait with implementing this though. There are a couple outstanding pull-requests (mainly https://github.com/jasperproject/jasper-client/pull/124) which will change a lot of code and quite frankly merging is a big hassle :)