I was hoping this would fix #367. It didn't but I think the changes are worthwhile regardless. Changes:

1. Frame retrieval is no longer in a while True loop.

• It's a scheduler instead
• The behaviour of the scheduler is almost identical to a while True loop since the time_to_next_frame is zero around 95% of the time (meaning it acts exactly like a while true with no delay).
• However, for the 5% of times, and also if the user is requesting a lower resolution, a small delay will be placed in the thread which frees up time otherwise spent waiting on the USB camera device.
• We can see from 8330fa1acf5d2c8f04076d3e49a4b0193ca4cee2 that small delays are a good thing and can benefit other parts of the system.

2. Calling self.on_frame is done by a separate thread

• This prevents the camera loop from being locked up by the user's on_frame callback function
• This means the next USB camera call is not blocked and can start being retrieved ready for when it's needed.
• In the worst case, the thread running the user's callback function finishes just after the new frame is ready. This results in it being missed and the loop goes onto get another USB camera frame. This worst case is exactly the current behaviour of the SDK. - In the best case, the user processing is shorter than the time it takes to grab a new frame and as soon as it's ready it calls their callback function again - the time saved is always going to be greater or equal to the current blocking case.

For change 1, I'm not sure if it's worthwhile implementing over a while True loop, but change 2 is definitely an improvement.

The reason this PR doesn't fix #367 is the same reason why change 1 might not be worth implementing - they are essentially doing the same thing. This is because the self.__frame_handler.frame = self.__camera.get_frame() is the bottleneck (time is very close to 1/FPS)

Codecov Report

Merging #372 (afe953e) into master (854e835) will decrease coverage by 6.05%. The diff coverage is n/a.

:exclamation: Current head afe953e differs from pull request most recent head 29ab634. Consider uploading reports for the commit 29ab634 to get more accurate results

@@            Coverage Diff             @@
##           master     #372      +/-   ##
==========================================
- Coverage   58.56%   52.50%   -6.06%     
==========================================
  Files          72       80       +8     
  Lines        2756     3114     +358     
==========================================
+ Hits         1614     1635      +21     
- Misses       1142     1479     +337     

Flag	Coverage Δ
unittests	52.50% <ø> (-6.06%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pitop/pma/potentiometer.py	50.00% <0.00%> (-10.00%)	:arrow_down:
pitop/pma/servo_motor.py	34.93% <0.00%> (-8.82%)	:arrow_down:
pitop/pma/light_sensor.py	50.00% <0.00%> (-8.34%)	:arrow_down:
pitop/pma/sound_sensor.py	50.00% <0.00%> (-8.34%)	:arrow_down:
pitop/core/mixins/supports_miniscreen.py	46.66% <0.00%> (-6.28%)	:arrow_down:
pitop/core/mixins/recreatable.py	40.00% <0.00%> (-5.46%)	:arrow_down:
pitop/core/mixins/stateful.py	33.33% <0.00%> (-5.13%)	:arrow_down:
pitop/pma/imu.py	41.89% <0.00%> (-5.03%)	:arrow_down:
...camera/core/capture_actions/capture_action_base.py	56.25% <0.00%> (-4.87%)	:arrow_down:
pitop/pma/encoder_motor.py	67.74% <0.00%> (-4.49%)	:arrow_down:
... and 32 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c5df5d1...29ab634. Read the comment docs.

The on_frame_thread seems like a good idea. I tried something like this before but was missing the critical piece to check self.__on_frame_thread.is_alive().

👍

We'd need to talk another look at the capture actions in the frame_handler also, I think they use threads already but without that addition to allow it to drop frames. We could consider removing the frame handler functionality to be honest or otherwise combining it with on_frame. It does make it a bit easier to add multiple processing callbacks, but there are less complex alternatives.

There's a ticket here for removing the frame handler stuff: https://github.com/pi-top/pi-top-Python-SDK/issues/198 It does bloat the camera code massively

I think other threads can run whilst this is blocked waiting for the next camera frame, without the need for a sleep or scheduler delay. I'm not sure though and the scheduler seems like an reasonable implementation but I worry a bit about the accuracy and overhead of the timing calls.

I'm a pretty big fan of sched but yeah it would be good to look into the overhead. Accuracy seems pretty decent although I've never tested it rigorously.

@m-roberts I think we can close this PR but in your opinion is item 2 worth implementing into the SDK? If so we can create a ticket for it.

what about using an FPS regulator on the camera similar to what we do with the miniscreen? i.e. if you are calling frames too often, then it will sleep to ensure that the FPS is no higher than what we want. This means "dynamic sleeping" rather than hard-coding it to always run.

I'm not sure - would be good to have some data to look at - what are the performance stats before/after? I don't really have much of an opinion on this right now as I'm not close enough to it...

what about using an FPS regulator on the camera similar to what we do with the miniscreen? i.e. if you are calling frames too often, then it will sleep to ensure that the FPS is no higher than what we want. This means "dynamic sleeping" rather than hard-coding it to always run.

Right now we are getting frames as fast as possible from the camera since it always guarantees the user has the most recent frame to use when their code requests one. If they use the get_frame method then we are "sleeping" until the next frame is ready since we have an Event there that is set by the looping thread.

I think what you're talking about is a user-settable FPS, which we can do for sure - this would limit the amount of IO reads to the camera and therefore reduce the overhead. It would be somewhat of an advanced feature when heavy computer vision is involved though since they need to set it higher than their algorithm FPS to ensure it doesn't slow down the system even more waiting for frames to be ready

I'm not sure - would be good to have some data to look at - what are the performance stats before/after? I don't really have much of an opinion on this right now as I'm not close enough to it...

It's not directly a performance thing (dependant on the user's code), it's just ensuring that the user callback function is put into a thread instead of stopping our camera thread from running. As I describe in the PR, I don't think there is any downside but just interested if you seen any potential drawbacks

I don't really see an issue - other than the build failing ;) (which is my bad)