Skip to content
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

camera: add grab_live #526

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

camera: add grab_live #526

wants to merge 4 commits into from

Conversation

tfarago
Copy link
Contributor

@tfarago tfarago commented Oct 26, 2023

Implements ufo-kit/libuca#97

@tfarago
Copy link
Contributor Author

tfarago commented Oct 26, 2023

@gabs1234 if you pull this you will be able to use viewer(camera.stream()). Also with phantom, however the _grab_live_real will need to be properly implemented for that particular case I guess.

@MarcusZuber
Copy link
Member

We have to discuss things about the states first. Some cameras have different capabilities to do live_grab in 'recording' and 'readout' state. We somehow have to also deliver this to concert and check for the states.
I also would like to keep the stream() with the normal grab and would prefer a 'live_stream'.

@tfarago
Copy link
Contributor Author

tfarago commented Oct 26, 2023

We have to discuss things about the states first. Some cameras have different capabilities to do live_grab in 'recording' and 'readout' state. We somehow have to also deliver this to concert and check for the states. I also would like to keep the stream() with the normal grab and would prefer a 'live_stream'.

Definitely. This is a WIP which is there to get things going.

@tfarago tfarago marked this pull request as draft October 26, 2023 09:14
@MarcusZuber
Copy link
Member

Very rough first thoughts:
I think the most flexible solution would be to (somehow) allow to add to libuca the possibility to read which property can be read/written in which state and which function is allowed to be called in which state (so basically the full state machine). Then this could be connected in concert to the state checks (or an exception from libuca would be propagated) to concert.
And I would make separate stream() and live_stream() functions.

@tfarago
Copy link
Contributor Author

tfarago commented Oct 27, 2023

Fuuuuuh, that's gonna be tough, although of course correct, but I would save the FSM in libuca for another day. Especially after the decentralized stuff is merged, that needs to be done ASAP. stream_live was added.

@MarcusZuber
Copy link
Member

I have the impression even this make stuff chaotic and needs more thinking (since both streams switch to recording etc.).
I was also surprised, that grab() does no state check.

How about redoing the stream in this way?

class Camera:
   ....
   async def stream(self)
      await self.start_recording():
      async for f in self.yield_frame():
         yield f

   async def yield_frames()
      while await self.get_state() in ['recoding', 'readout', open for discussion]:
         yield self.grab()


   async def stream_live(self)
      await self.start_recording():
      async for f in self.yield_frame_live():
         yield f

   async def yield_frames_live()
      while await self.get_state() in ['recoding', 'readout', open for discussion]:
         yield self.grab_live()

I will describe the use-case of this later (but have to finish now for today).

@MarcusZuber
Copy link
Member

I was doodling a lot of state machines and came to the conclusion (from my pov) that splitting the state for live view and normal acquisitions should be the best. Also libuca can raise exceptions when a requested state change is not allowed. So the full logic would be hidden in libuca and it should also work with non-updated plugins.
I can make a preliminary commit later to show you what I mean.

#TODO: The cached states and concert.base.transition also need to be adjusted
@tfarago
Copy link
Contributor Author

tfarago commented Oct 31, 2023

I was doodling a lot of state machines and came to the conclusion (from my pov) that splitting the state for live view and normal acquisitions should be the best. Also libuca can raise exceptions when a requested state change is not allowed. So the full logic would be hidden in libuca and it should also work with non-updated plugins. I can make a preliminary commit later to show you what I mean.

Fuh, this is going to take some hard convincing... Two states? That just doesn't sound right... A camera is either recording or not and you may either grab live frames or not and you may either change parameters or not. There is nothing really special about the live frames, or am I wrong? Then, every plugin should say whether and which parameters may be changed during recording, here we can help in the base class and the plugins can e.g. specify a list of changeable parameters or something.

@MarcusZuber
Copy link
Member

My attempt to convince you: I think I have that allow live view in normal recording, and only one of both. With the two states (and especially the two start_recordings) I could catch catch this then in libuca.

@tfarago
Copy link
Contributor Author

tfarago commented Oct 31, 2023

I think I have that allow live view in normal recording

What do you mean exactly? Anyway, to me it feels like a workaround a little bit.

@MarcusZuber
Copy link
Member

My sentence just did not make sense at all...

I meant that I have detectors, that can run live_view and normal acquisitions at the same time, but I also have detectors where only live_view or the "normal" acquisition is possible at one time (and of course cameras that do not support both modes at all). And to reflect this I could (with the above implementation) just to throw an exception in the different start_recordings dependent if it is allowed in the current detector state on the uca-plugin-level or for a non-uca-camera in concert.

@tfarago
Copy link
Contributor Author

tfarago commented Oct 31, 2023

I see your points, let's discuss this in person later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants