Camera: try to make discovery more robust#13100
Conversation
|
This does improve camera discovery for me but I want to walk through it once more and verify what is going on. |
b21f7c6 to
32ad1ea
Compare
|
I tested this again and went through it once again. I believe it's in a better state overall but if you have concerns, let me know. |
32ad1ea to
2d4026e
Compare
|
I don't know if this would make your code more complicated with all the timers and whatnot but there is a method for the "try this and if that doesn't work do this instead" pattern: Not sure if that would simplify or complicate your code? |
2d4026e to
a19d02e
Compare
|
@DonLakeFlyer yes that would probably make sense to use. Does this command do internal retries or not? If it does, it wouldn't work because these need to happen outside where we can try both new and legacy commands. |
|
Only these commands will retry: |
|
So this would retry the MAV_CMD_REQUEST_MESSAGE command and that means we can't use it. Unless we add an argument that let's us specify whether to retry. |
|
How come you don't want to retry the request message? |
|
I am retrying it with the logic around it but I'm retrying like with the legacy command interleaved, e.g.:
5 times each with exponential back off for slow cameras. |
|
OK, understood on retries. @julianoes I poked at this a bit and the crashes in unit tests are real. They are crashing in the camera retry logic. Run with --unittest and a debugger. |
|
Ok, I will look into that. Thanks! |
This is an attempt to fix some of the corner cases trying to discover cameras, namely: - Implement missing retries for CAMERA_INFORMATION, us old specific commands and REQUEST_MESSAGE. - Try to simplify CameraControl::_initWhenReady a bit. - Fix _requestStreamInfo() and _requestStreamStatus() using wrong retry variable. - Add a few missing timers.
The logic was spread across 3 functions, we can actually remove a bit of this duplication.
This makes us retry to discover a camera if it disappears after not having been connected previously (for whatever reason).
In case a camera takes a while to properly respond, it's useful to wait a while with retries while also not spamming the component with commands the whole time. Therefore, retry with exponential backoff should do the job.
- Fix missing retry counter reset in handleCaptureStatus() - Fix missing timer stop in handleCaptureStatus() - Consolidate inconsistent counter usage for REQUEST_CAMERA_CAPTURE_STATUS - Standardize all retry limits to 6 attempts (was mixed 5/6) - Standardize all retry timeouts to 1000ms (was mixed 500ms/1000ms) - Standardize video stream info retry factor from 5 to 6 (_expectedCount * 6) - Standardize _initWhenReady timing to consistent 500ms spacing - Add consistent debug logging across all request functions - Verify all success handlers properly stop timers and reset counters - Ensure all requests use proper alternating message patterns
This fixes the unittests segfaulting.
a19d02e to
57adaf1
Compare
|
I've added a commit to fix the unit tests crashing on destruction. |
|
You should bring this across to Stable as well |
|
@DonLakeFlyer I tried but it's a bit tricky with quite a few conflicts, mostly due to the changes in #11178. Edit: oh, you mean Stable_V5.0, not V4.4! |
|
@DonLakeFlyer backport: #13342 |
This is an attempt to fix some of the corner cases trying to discover cameras, namely:
I haven't been able to properly test each and every retry mechanism used but overall this consistently initializes correctly for my case when it previously does not.