Included Audio Manager class #20
Conversation
Signed-off-by: Aitor Miguel Blanco <[email protected]>
pycozmo/audio.py
Outdated
| else: | ||
| raise TypeError('Invalid audio type: {}'.format(type(audio))) | ||
|
|
||
| self.start() |
There was a problem hiding this comment.
I think, you should create and start the thread here, as otherwise, play() will work only for the first call and never afterwards.
There was a problem hiding this comment.
Maybe I should change the name of this call from start() to something else.
I realised this and decided to have a thread object as part of the AudioManager instead from inheriting from Thread directly. Here, self.start() is a function that (if no thread is already running) creates one and launches it (see line 31).
I also thought it would be good to have this functionality on a separate function, to make it possible to stop restart the thread.
I'll change the name so it is not misleading, let me know if you have any ideas for it or if you think I should get the thread creation/start out of that function and place it here.
| @@ -31,6 +32,7 @@ def __init__(self, robot_addr: Optional[Tuple[str, int]] = None, | |||
| protocol_log_messages: Optional[list] = None) -> None: | |||
| super().__init__() | |||
| self.conn = conn.ClientConnection(robot_addr, protocol_log_messages) | |||
| self.audio = audio.AudioManager(self.conn) | |||
|
|
||
| MULAW_MAX = 0x7FFF | ||
| MULAW_BIAS = 132 | ||
| def u_law_encoding(sample): |
There was a problem hiding this comment.
Is this implementing the algorithm described here? https://en.wikipedia.org/wiki/%CE%9C-law_algorithm
There was a problem hiding this comment.
I think so, but the discrete algorithm is not very well described there. I searched some other places and ended up modifying the example here to take 16 bit samples instead of 13 bits (you just need to look into the 12 most significant bits, not the least ones like it happens in that example)
pycozmo/conn.py
Outdated
| self.last_ack = 1 | ||
| self.last_send_timestamp = None | ||
| self.one_time_queue = [] | ||
| self.repeat_queue = {} |
There was a problem hiding this comment.
Would you be willing to leave the resend changes for a separate PR?
While adding packet resending is very needed to achieve robust communication, it is completely separate from the audio changes.
In general, the self.window which is an instance of SendWindow acts as both repeat_queue and to_clear_queue. Implementing resend boils down to resending packets (just the first?) in the current send window after a timeout. This could be done either in a new resend thread or in the current SendThread when self.queue.get() times out.
There was a problem hiding this comment.
For sure, that makes total sense. We should be aware though that without this, there will be problems with audio synchronization. It looks like it is important to keep the Keyframe count to know when the packet has been received. Not including this would make the robot miss frames, producing some clipping sounds. I think that both changes are needed for the sound to work properly.
There was a problem hiding this comment.
Agreed. I am now motivated to get this working :)
pycozmo/conn.py
Outdated
| @@ -259,9 +304,9 @@ def connect(self) -> None: | |||
| except InterruptedError: | |||
| pass | |||
|
|
|||
| def send(self, pkt: Packet) -> None: | |||
| def send(self, pkt: Packet, repeat=False) -> None: | |||
There was a problem hiding this comment.
repeat does not need to be exposed to the application layer.
Cozmo is using a form of the selective-repeat ARQ protocol . It guarantees to the application layer that a sequence of packets that are sent, will be received on the other side in the same order and without duplicates.
There was a problem hiding this comment.
Do we have this already implemented? I'm sorry, I'm a bit clueless about network protocols so I probably missed a ton of stuff that is already there.
It makes a lot of sense that cozmo uses something like that, but I am also wondering, how does cozmo acknowledge that a message is received? I thought first that maybe not all frames are confirmed, which would explain why I saw that relation between the audio and the Keyframe packets, but now looking at this, it would make more sense that in all responses from cozmo there would be an acknowledgement of the received frames, does this happen? Then maybe the Keyframe is a completely unrelated thing. Tbh that would make things much easier for the whole communication. I don't really like the idea of having different behaviours for different packets.
|
|
||
| Args: | ||
| conn (ClientConnection): client managing the communication with the robot | ||
| """ |
There was a problem hiding this comment.
It will be great if you could modify examples/audio.py to use the new Client.play_audio().
Signed-off-by: Aitor Miguel Blanco <[email protected]>
Signed-off-by: Aitor Miguel Blanco <[email protected]>
|
I reverted the changes related to the SendThread, and made some changes to the AudioManager. I would suggest waiting until we have running the resend packages to finish up this PR, if that is okay with you. |
Signed-off-by: Aitor Miguel Blanco <[email protected]>
Signed-off-by: Aitor Miguel Blanco <[email protected]>
pycozmo/client.py
Outdated
| @@ -436,3 +439,7 @@ def get_anim_names(self) -> set: | |||
| @property | |||
| def anim_names(self) -> set: | |||
| return self.get_anim_names() | |||
|
|
|||
| def play_audio(self, audio): | |||
There was a problem hiding this comment.
Does it make sense to rename this to play_file() also?
| pkt = pycozmo.protocol_encoder.OutputAudio(frame) | ||
| cli.conn.send(pkt) | ||
| time.sleep(0.033742) | ||
| cli.play_audio("hello.wav").wait_until_complete() |
|
I finally tried this and it works great! Hats off for figuring audio out. |
|
I'm very happy to see this working too! |
pycozmo/audio.py
Outdated
| self.thread = Thread(target=self.run, name=__class__.__name__) | ||
| self.thread.start() | ||
|
|
||
| def stop(self) -> None: | ||
| self._stop = True | ||
| self.thread.join() |
There was a problem hiding this comment.
Previously, if audio play() had never been called, this line was raising an exception as a thread that has not be started cannot be joined.
| @@ -24,19 +24,21 @@ def __init__(self, conn: ClientConnection): | |||
| self.stream = [] | |||
| self._stop = False | |||
| self.conn = conn | |||
| self.thread = Thread() | |||
| self.thread = None | |||
There was a problem hiding this comment.
Only create a thread for playback.
This PR is related to #11.
Cozmo uses packets including 744 samples of u-law encoded data to reproduce 22kHz audio recordings.
Changes in this PR:
Please check out the changes in conn. I am not familiar with network communications so I am not sure whether there are any patterns that can be used generally to resend packets (or frames).
Currently, only 16 bit samples have been tested, but it should also be possible to send 8 bit samples also.
The audio is limited to 22kHz. For 44 or 48kHz files, only one of every two samples is used.
It is possible to use files with 1 or 2 channels (although only the first one will be used).
Only .wav files are supported, but if necessary, the functionality to load other files could be included in the future.
In the future, it would be good to include a method to allow playing a bytes object from memory, this would make it possible to include some text-to-speech capabilities to the robot.