Skip to content

Add AcquisitionEvent.setTimeoutMs - #141

Merged
marktsuchida merged 1 commit into
micro-manager:mainfrom
bls337:event-timeout-setter
Nov 4, 2026
Merged

marktsuchida merged 1 commit into
micro-manager:mainfrom
bls337:event-timeout-setter

Conversation

@bls337

@bls337 bls337 commented Oct 20, 2026 •

Copy link
Copy Markdown
  • Add AcquisitionEvent.setTimeoutMs. The timeout could be set from JSON but not from Java. The default is still null, meaning no time limit.

@bls337 bls337 closed this Oct 22, 2026
@bls337 bls337 reopened this Oct 22, 2026
@bls337

bls337 commented Oct 22, 2026

Copy link
Copy Markdown
Author

@nicost This is my first PR to AcqEngJ, please take a look. Please keep an eye on this repo, I will be making a number of PRs to fix issues.

@bls337

bls337 commented Nov 4, 2026

Copy link
Copy Markdown
Author

@marktsuchida if you have a chance, please take a look

@marktsuchida

Copy link
Copy Markdown
Member

Looks good to me but I think propagating the timeout in the merge constructor doesn't actually do much, because the timeout is read from each sub-event of a sequence:

if (event.getSequence().get(i).getTimeoutMs() != null) {

So the effect is only visible if you call getTimeoutMs() on the merged event itself, and might be slightly misleading if the sub-events have differing timeouts. Maybe that's still a good thing (for consistency with the other two fields you mentioned) — just wanted to be sure it's intentional.

@bls337
bls337 force-pushed the event-timeout-setter branch from 2c95ed3 to ea09ae8 Compare November 4, 2026 20:35
@bls337

bls337 commented Nov 4, 2026

Copy link
Copy Markdown
Author

@marktsuchida Thanks. I was trying to be consistent but upon further inspection it was unnecessary.

Unlike the minimum start time and exposure, nothing in AcqEngJ reads the timeout off the merged event, so I dropped the line and the test.

The new commit's CI needs approving again.

@marktsuchida

Copy link
Copy Markdown
Member

@bls337 Thanks!

@marktsuchida
marktsuchida merged commit 9f46e53 into micro-manager:main Nov 4, 2026
1 check 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.

2 participants