-
Notifications
You must be signed in to change notification settings - Fork 16
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
Introduce recv_blocking()
method on PayloadReceiver
#153
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I tested this locally, and run well.
Nit: Could you update async-std
dependency to 1.12.0
?
@bschwind Could you review and merge the PR if it's ok? I totally agree to update the version after the PR is merged. |
Done. Was the intent to make sure that But perhaps we don't have to worry too much - and I also plan to be touching this in another PR. |
Oops, it's true. Sorry for the additional work. |
Strange, when I run
This is on commit d7caba6 Edit: Ah, I ran a |
With a real camera attached, I get 3 test failures, but these also happened in The camera is an IDS U3-3881LE-C-HQ Rev 1.2
|
Additionally, with that same camera attached, I get an error on the
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's double check the stream
example and why it's failing before merging.
As for the cameleon/cameleon/src/camera.rs Lines 350 to 372 in 4341ea0
At line 362, So this bug would be resolved by calling the |
@bschwind I've run this branch on `dev-lite-build0-portal` and surprisingly it runs fine (expand for logs)
Interesting difference between this and your logs from Probably the first capture always fails for you? Could be also something Mac-specific. The previous stream example silently ignored such errors, but the example as of d7caba6 was rather defensive and panicked on them straight away. @Y-Nak note that it is the very first capture (receive) that fails for Brian - I've originally read his logs wrong, but he pointed that out. Anyways, I've changed the panic to logging the warning in the latest commit - please try with that. If that helps I'll also change the code in READMEs I guess. |
Yeah, I realized it. I mean, if Brian ran the camera.start_streeming() // <- main
... // <- main
camera.stop_streaming() // <- main: here overflow occurred.
camera.start_streaming() // <- the pr: the buffer state is still overflowed if the camera is not unplugged.
payload_rx.
recv_blocking().
expect("should receive a payload"); // <- the pr; panic here affected by the overflow that occurred in the first run. I guess the problem will be solved if you unplug the camera every time before running the example. |
But this is just speculation; I'm unsure if it's true.
I think we can exclude the possibility. I basically run tests on Linux, Mac, and Windows. But I didn't find the difference between OS except for
This is nice for finding some kind of bugs. But also losing some packets sometimes happens without a bug, not so often, though. For example, if many devices are streaming simultaneously on the same USB host controller, then some packets might be lost. |
Interesting - I'm receiving approximately random results on each run. Sometimes all captures fail, sometimes just a few fail, and a lot in between. I don't see any particular patterns. Re-plugging the camera also doesn't resolve the issue. The current Here are two example runs from this branch:
|
Allows callers to wait for a frame without using futures. Also bump async-std dependency to 1.12 (unfortunately still doesn't ensure recent-enough `async-channel` - but I'll fix that in another PR). Also update the stream example and READMEs with the new method, making it print any receive errors (but not panic on them) and doing exactly 10 tries (instead of waiting for 10 success payloads).
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! We can dig into the stream errors separately as it's nothing introduced by this PR.
👍 |
Allows callers to wait for a frame without using futures. Turned out async_channel::Receiver already had a
recv_blocking()
method, so this is rather straightforward.In addition to the API changes, update READMEs and example to use the new method. The version in current
main
does a busy loop (consumes 100% of its CPU thread).Best viewed with whitespace change ignored.
This is yet to be tested with a real camera (:pray: :pray: @bschwind).
test_all.sh
is passed.Addfix #{ISSUE_NUMBER}
if the corresponding issue exists.## Changelog
section. If the change is for only internal use, please writeNone
to the section.Changelog
recv_blocking()
method was introduced onPayloadReceiver
. It allows callers to wait for a frame without using futures.