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

AVT Camera #142

Closed
wants to merge 11 commits into from
Closed

AVT Camera #142

wants to merge 11 commits into from

Conversation

i-jey
Copy link

@i-jey i-jey commented Feb 14, 2023

Work in progress
Issue: #141

Skeleton for now, actively working on this.

A couple questions - should we retain the format of the wrapper used in ulc-malaria-scope (here)

  • i.e circumvent the intended context manager format?
  • Should this class instead use a multithreaded producer/consumer structure - something like the example provided by VimbaPython here?
  • What API should it conform with to match the rest of coPylot?

@i-jey i-jey self-assigned this Feb 14, 2023
@i-jey i-jey added the feature request New feature or request label Feb 14, 2023
@i-jey i-jey mentioned this pull request Feb 14, 2023
Copy link
Contributor

@AhmetCanSolak AhmetCanSolak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we retain the format of the wrapper used in ulc-malaria-scope (here)

  • i.e circumvent the intended context manager format?

It seems wise to use the context manager in our class as it takes care of init/de-init steps, but we don't need to require our API users to create context managers with our adaptors. So something like the wrapper implemented in ulc-malaria-scope is a good starting point for us IMHO. Also this will make our adaptor a drop-in replacement in ulc-malaria-scope repo.

  • Should this class instead use a multithreaded producer/consumer structure - something like the example provided by VimbaPython here?

I checked the Vimba example, we can implement something similar when we have multiple camera use-case. Currently, we don't seem to need it, we can move with single camera assumption for now.

  • What API should it conform with to match the rest of coPylot?

There is no specified in cameras submodule yet, and we can discuss on this PR and merge something that might be subject to updates in the future.

@AhmetCanSolak
Copy link
Contributor

Btw, if you don't mind @i-jey, I like to move some of the functions from https://github.com/czbiohub/ulc-malaria-scope/blob/develop/ulc_mm_package/hardware/real/camera.py to this PR and submit to PR branch. Is it okay for you @i-jey ?

@i-jey
Copy link
Author

i-jey commented Feb 15, 2023

Btw, if you don't mind @i-jey, I like to move some of the functions from https://github.com/czbiohub/ulc-malaria-scope/blob/develop/ulc_mm_package/hardware/real/camera.py to this PR and submit to PR branch. Is it okay for you @i-jey ?

@AhmetCanSolak That sounds good! I was going to do that earlier but figured I should hold off until I heard your thoughts on how we should structure this class :)

@i-jey i-jey closed this Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants