Gestures in the CB. #36
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: pepperplus/pepperplus-cb#36
Reference in New Issue
Block a user
Delete Branch "feat/cb2ri-gestures"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
ref: N25B-334
To verify:
Yeah, to actually test this you need to be at pepper, and launch both the UI and RI (with the RI on the correct network and having the correct qi-url). Without going to pepper you could test:
Fetching the gestures gives an empty list (we're not connected).
Do gesture
doeswork, at least it gives a print statement in the RI. Note that an error could occur when trying to send it to pepper, due to not having a qi session:)Check that the tests make sense
New:
With the new threads applied, the endpoints are changed. This means that all the testing should be done on a branch of the UI where these changes are also reflected (which they now are in the gestures branch).
After the threads everything also works.
assigned to @9828273
requested review from @8464960
added 8 commits
dev1c9b722b- Merge branch 'dev' into feat/cb2ri-gesturesCompare with previous version
added 1 commit
2e472ea2- chore: remove wrong test pathsCompare with previous version
marked the checklist item Pipeline (tests) pass as completed
FastAPI uses the types in the function signature for model checking:
If the given command is not a valid
SpeechCommand, FastAPI should reject the request before this function even gets called.For now it works because a
GestureCommandis a sort of subset ofSpeechCommand, but ifSpeechCommandchanges, this may no longer be the case.I suggest making two endpoints, for example
/command/speechand/command/gesture.The "get" in
/get_...is superflous because it's an HTTP GET endpoint. We should probably define standards for URL naming conventions, but I'd say a conventional name would be "/command/gesture/tags".Can be done with a query parameter, e.g. "/command/gesture/tags?count=<number>"
You could use
req_socket.recv_json()to merge this with the next step.This
ri_command_addressis not actually something that we can know beforehand, we only know it from the negotiation. I've removed the setting in the feat/environment-variables branch, so it'll have to be removed here as well when that's merged.The suggested change will break unit tests because there is no longer a default, which the tests assume.
You may reject this :P
Please move this address to the config file
marked the checklist item Do gesture
doeswork, at least it gives a print statement in the RI. Note that an error could occur when trying to send it to pepper, due to not having a qi session:) as completedmarked the checklist item Fetching the gestures gives an empty list (we're not connected). as completed
marked the checklist item Style checks pass as completed
marked the checklist item Documentation is up to date as completed
marked the checklist item Tests are up to date (new code is covered) as completed
marked the checklist item Check that the tests make sense as completed
Small detail: this should be "It receives gesture commands"
I tested it both with and without Pepper and it works great!
If you resolve the current threads (and as Twirre said, move the adresses to the config file), you have my approval.
left review comments
changed this line in version 5 of the diff
added 1 commit
b2d01475- Apply 1 suggestion(s) to 1 file(s)Compare with previous version
changed this file in version 6 of the diff
changed this line in version 6 of the diff
changed this line in version 6 of the diff
changed this line in version 6 of the diff
changed this line in version 6 of the diff
added 2 commits
daf31ac6- fix: change the address to the config, update some logic, seperate the api...71d86f5f- Merge branch 'feat/cb2ri-gestures' of...Compare with previous version
added 1 commit
f15a5189- fix: testsCompare with previous version
changed the description
changed the description
changed the description
This validation is redundant: FastAPI already does this for you.
You already have tests covering this
test_receive_speech_command_invalid_payloadandtest_receive_gesture_command_invalid_payload. Remove this validation and you'll see they still pass.mentioned in merge request !38
changed this line in version 8 of the diff
added 1 commit
db5504db- chore: remove redundant checkCompare with previous version
marked the checklist item After the threads everything also works. as completed
mentioned in commit
4ab6b2a0e6I approve!