Skip to content
This repository has been archived by the owner on Sep 4, 2024. It is now read-only.

fix and test software triggering #34

Merged
merged 15 commits into from
Aug 8, 2023

Conversation

nclack
Copy link
Member

@nclack nclack commented Aug 1, 2023

closes #32

  • added tests. Moved integration and driver api tests to separate folders.
  • fixed a deadlock involving software triggering.
    • The mutex protecting the rendering buffer is no longer held while waiting for a trigger.
  • Depends on check for mapped reader acquire-video-runtime#74 that resolves a silent failure that I ran into during testing.

@nclack nclack changed the title 32 add test for software trigger fix and test software triggering Aug 4, 2023
@nclack nclack marked this pull request as ready for review August 4, 2023 23:36
nclack added a commit to acquire-project/acquire-video-runtime that referenced this pull request Aug 7, 2023
Fixes an unchecked failure in `acquire_map_read()` caused when a caller
fails to call `acquire_unmap_read()` at the right time.

For example:

```c
acquire_map_read(runtime,0,&beg,&end);
// When end>beg
acquire_map_read(runtime,0,beg,end); // Should return AcquireStatus_Error
```

whereas


```c
acquire_map_read(runtime,0,&beg,&end);
// When end>beg
acquire_unmap_read(runtime,0,&beg,&end);
acquire_map_read(runtime,0,beg,end); // Should return AcquireStatus_Ok
```

acquire-project/acquire-driver-common#34 depends on this
…acquire-driver-common into 32-add-test-for-software-trigger
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.

I don't have enough knowledge/context to approve or request changes here, but hopefully my comments and questions are useful.

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.

A few things I'd like to see changed, but no real showstoppers. Could merge as is.

tests/integration/acquire-video-runtime Outdated Show resolved Hide resolved
tests/integration/abort-while-waiting-for-trigger.cpp Outdated Show resolved Hide resolved
tests/integration/configure-triggering.cpp Outdated Show resolved Hide resolved
tests/integration/CMakeLists.txt Outdated Show resolved Hide resolved
tests/integration/list-digitial-lines.cpp Outdated Show resolved Hide resolved
tests/integration/list-digitial-lines.cpp Outdated Show resolved Hide resolved
Comment on lines +167 to +175
while (end == beg && is_running(runtime) &&
clock_toc_ms(&t0) < test_timeout_ms) {
// delay so loop isn't busy polling
clock_sleep_ms(0, 10);
OK(acquire_map_read(runtime, 0, &beg, &end));
}

if (end == beg && !is_running(runtime))
break;
Copy link
Member Author

Choose a reason for hiding this comment

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

@andy-sweet I think the test is not flaky now.

I kept the sleep in the loop just so it's not burning CPU. I had to add the additional is_running check. Otherwise, there was a window where the acquisition would stop but it would make it to line 167 and hit the test timeout (and fail).

@nclack nclack merged commit 3f59301 into acquire-project:main Aug 8, 2023
4 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add test for software trigger
3 participants