Conversation
micolspitale93
requested review from
RMichaelSwan and
chrismbirmingham
and removed request for
chrismbirmingham
|
Great work so far! Everything you have done so far will be useful for future work. We appreciate the work that you have done getting a working example for integrating HARMONI with py_trees. However, for where the project is right now we have some additional requirements and recommendations for how we would like to see the integration happen:
In the future we will be looking at the possibility of a standalone python based HARMONI, but for now we want the py_trees to utilize the communication system we have already set up. The current revision of work should be saved as a separate branch so that we can use it later. |
There was a problem hiding this comment.
I see a lot of improvements have been made. I still need to test on my PC and review some of the newer non-leaf classes, but overall looks pretty good. One comment on commit history: use descriptive commits and/or squash commits together! I am not too picky on git history, but when I see multiple commits with the exact same comment, it does make it notably harder to understand what actually occurred.
| else: | ||
| behavior_data = data | ||
| viseme_set = [] | ||
| #FIXME decommenta le righe |
There was a problem hiding this comment.
| #FIXME decommenta le righe |
| if ".wav" in data: | ||
| data = self.file_path_to_audio_data(data) | ||
| duration = data["duration"] | ||
| duration = data["duration"]-0.5 |
There was a problem hiding this comment.
Leave out magic numbers if not obvious. One option: make a constant, e.g. AUDIO_DELAY or something (the reason for or meaning of "0.5" should be clear).
| #FIXME | ||
| #vorrei scrivere | ||
| #self.result_msg = tts_response["audio_data"] | ||
| #ma scrivo |
There was a problem hiding this comment.
Is this still needed? Also, English please :) this is not the only instance.
| INTERACTION = "Background_noInterazione" | ||
| VISUAL = "Background_visivo" | ||
| CARTA = "Carta" | ||
| RACCOLTA = "ConfirmRaccolta" |
There was a problem hiding this comment.
Forgive my lack of Italian, but could we get translations in comments at least? Ideally everything would be in English, but I can understand not reworking lower importance things. Much appreciated whatever you decide.
| that a new goal can be received. | ||
| """ | ||
| self.service_manager.pause() | ||
| self.service_manager.stop() |
There was a problem hiding this comment.
I believe this was working with pause, so not clear on why the change was made here. Stop might mean a service will require a more in-depth startup procedure to begin again
| @@ -0,0 +1,175 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
Repeated code is a warning sign/design smell. All the leaves in this pull request tend to have a similar structure which means a small architecture change (e.g. something with HarmoniServiceSever) would require identical modifications to all leaves. To reduce total amount of code, I would strongly recommend creating an abstract base class for all leaves.
6 participants
Rostests already implements for the following pytree leafs:
rostest harmoni_face face_pytree.testrostest harmoni_bot lex_pytree.testrostest harmoni_tts polly_pytree.testrostest harmoni_speaker speaker_pytree.testrostest harmoni_web web_pytree.testThe remaining leaf's tests (harmoni_microphone, harmoni_camera, and harmoni_stt) are still work in progress.
If you want to test a already created Tree, you can run the following:
rostest harmoni_pattern pytree.testIt will execute a try composed by a sequence of:
Still not handling the different HARMONI API in the testing (WORK IN PROGRESS)