-
Notifications
You must be signed in to change notification settings - Fork 8
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
Allow OAV to Redis forwarder to use different OAV streams #808
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #808 +/- ##
=======================================
Coverage 95.02% 95.03%
=======================================
Files 117 117
Lines 4765 4771 +6
=======================================
+ Hits 4528 4534 +6
Misses 237 237 ☔ View full report in Codecov by Sentry. |
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.
Looks okay, but can you confirm the streams are the right way round? The 'full screen' one looks smaller than 'roi' for me
Source.FULL_SCREEN.value: epics_signal_r( | ||
str, f"{prefix}MJPG:MJPG_URL_RBV" | ||
), | ||
Source.ROI.value: epics_signal_r(str, f"{prefix}XTAL:MJPG_URL_RBV"), |
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.
Looking at the stream links, the XTL is a larger image than the MPJG - are these definitely the right way round?
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.
Good spot! Thanks!
@@ -95,7 +110,8 @@ async def _get_frame_and_put_to_redis(self, response: ClientResponse): | |||
async def _open_connection_and_do_function( | |||
self, function_to_do: Callable[[ClientResponse, str | None], Awaitable] | |||
): | |||
stream_url = await self.stream_url.get_value() | |||
source = await self.selected_source.get_value() |
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.
Could: use asyncio.gather
for consecutive reads
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 don't think I can here because the read of stream_url
needs the value from the read of source
. asyncio.gather
will try and read them in parallel
Refactor the tests that are testing logic of one channel to be clearer that's what they're doing.
This reverts commit c6c6eca.
Needed for DiamondLightSource/mx-bluesky#511
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}