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

Fix setting ROI #86

Merged
merged 9 commits into from
Aug 15, 2023
Merged

Conversation

nclack
Copy link
Member

@nclack nclack commented Aug 14, 2023

closes #85

adds test, updates changelog

Changes the ROI setting procedure so it's more robust:

  1. Set offset to 0, so size can have full range of sensor.
  2. Set the size
  3. Set the offset

Sometimes the offset has to be divisible by 4. When setting a parameter that is not appropriately divisible, the call just fails with an invalid parameter which is not very helpful.

Fortunately, when running in "progressive" mode (the default for light-sheet), there are fewer restrictions. I'm not exactly sure how to query the restrictions, so the approach for this PR is to make sure we're setting the mode early.

@nclack nclack requested a review from aliddell August 14, 2023 21:19
@nclack nclack marked this pull request as draft August 14, 2023 21:25
@nclack nclack requested a review from andy-sweet August 14, 2023 23:06
@nclack nclack marked this pull request as ready for review August 14, 2023 23:07
src/dcam.getset.c Outdated Show resolved Hide resolved
src/dcam.getset.c Outdated Show resolved Hide resolved
@nclack nclack changed the title set shape before offset to avoid offset oob error Fix setting ROI Aug 14, 2023
Copy link

@andy-sweet andy-sweet left a comment

Choose a reason for hiding this comment

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

Looks good to me, other than the CHANGELOG comment.

CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@aliddell aliddell left a comment

Choose a reason for hiding this comment

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

Lgtm

@aliddell aliddell merged commit 6c0af4a into acquire-project:main Aug 15, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

setting an roi is broken
3 participants