Skip to content
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

More cleanup #31

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

More cleanup #31

wants to merge 3 commits into from

Conversation

brandon-groundlight
Copy link
Contributor

An attempt to make things more readable by separating concerns into separate files. This involves moving enums and associated functions out of the main deployable example file.

Additionally, this adds a RAII class to help manage getting frames from the camera in the main loop. While this doesn't put a formal lock on the frame, it does guarantee that at the end of each loop the frame is properly released preventing errors from successive calls to esp_camera_fb_get between loops

frame = esp_camera_fb_get();
// Testing how to get the latest image
esp_camera_fb_return(frame);
frame = esp_camera_fb_get();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell, there isn't a clear reason to get and release before getting the frame we'll actually use. If there is, we can incorporate that into the frameLock class

Copy link
Contributor

Choose a reason for hiding this comment

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

this may be to clear a buffered frame. test with an actual camera and an image that is changing to confirm ?

@@ -789,14 +716,13 @@ void loop () {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Immediately above is a return statement without a esp_camera_fb_return call. Using the frameLock class should change the behavior there as we now will implicitly call esp_camera_fb_return in the class destructor

@brandon-groundlight
Copy link
Contributor Author

There's enough code changes here that I'm a little nervous merging. Currently looking into Pio test infrastructure to see how we could better automatically review changes like this

Copy link
Contributor

@positavi positavi left a comment

Choose a reason for hiding this comment

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

LGTM if it tests well and you think its easier to manage/maintain. please double check there is no frame buffer issue removing the extra camera frame. it may also be legacy for one of the other supported camera modules.

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