-
Notifications
You must be signed in to change notification settings - Fork 17
[Camera] Adds support for camera app yaml test cases through camera-controller #213
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v2.13-develop
Are you sure you want to change the base?
[Camera] Adds support for camera app yaml test cases through camera-controller #213
Conversation
Signed-off-by: Sathvik K Gatti <[email protected]> Signed-off-by: Suyambulingam Rathinasamy Muthupandi <[email protected]> Signed-off-by: Charles Kim <[email protected]>
Signed-off-by: Sathvik K Gatti <[email protected]> Signed-off-by: Suyambulingam Rathinasamy Muthupandi <[email protected]> Signed-off-by: Charles Kim <[email protected]>
@antonio-amjr: Thank you for your guidance. Could you please review this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @chulspro,
I left some suggestions below. Let me know if those make sense.
if yaml_step.command in [ | ||
"UserPrompt", | ||
"PromptWithResponse", | ||
"VerifyVideoStream", | ||
]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think someone mentioned before, but anyway I think that would be good to store this array in a variable to use whenever necessary.
# if any step has a UserPrompt, PromptWithResponse or VerifyVideoStream command, | ||
# categorize as semi-automated | ||
if any( | ||
s.command in ["UserPrompt", "PromptWithResponse", "VerifyVideoStream"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as before:
I think someone mentioned before, but anyway I think that would be good to store this array in a variable to use whenever necessary.
try: | ||
await websocket.accept() | ||
logger.info(f'Websocket connected: "{websocket}".') | ||
except RuntimeError as e: | ||
logger.info(f'Failed to connect with error: "{e}".') | ||
raise e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it ok to replace for something similar to the websocket_endpoint()
?
Like:
try: | |
await websocket.accept() | |
logger.info(f'Websocket connected: "{websocket}".') | |
except RuntimeError as e: | |
logger.info(f'Failed to connect with error: "{e}".') | |
raise e | |
socket_connection_manager = SocketConnectionManager() | |
await socket_connection_manager.connect(websocket) |
sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM, 0) | ||
sock.bind((UDP_SOCKET_INTERFACE, UDP_SOCKET_PORT)) | ||
logger.info("UDP socket bound successfully") | ||
loop = asyncio.get_event_loop() | ||
while True: | ||
data, _ = await loop.run_in_executor(None, sock.recvfrom, 65536) | ||
# send data to ws | ||
await websocket.send_bytes(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to use SocketConnectionManager()
to handle those operations with a new method?
Problem
Change overview
Testing