prusa3d / Prusa-Firmware-ESP32-Cam

Firmware for ESP32 Cam modules to be used in Prusa Connect
GNU General Public License v3.0
97 stars 10 forks source link

Fix CapturePhoto Method Issues #25

Closed Anheledir closed 1 month ago

Anheledir commented 1 month ago

Description

  1. Removed Redundant Frame Buffer Handling:

    • The initial call to esp_camera_fb_get() followed immediately by esp_camera_fb_return(FrameBuffer) captures and immediately releases a frame buffer, which seems redundant.
  2. Added Error Handling for Semaphore Take Failure:

    • Added a check to handle the case where xSemaphoreTake fails, logging an error and returning appropriately.
  3. Released Semaphore on Error:

    • Ensured the semaphore is released if the initial call to esp_camera_fb_get() fails, preventing possible deadlocks.
  4. Prevented Potential Infinite Loop:

    • Introduced a maximum attempt limit (5) for capturing a valid photo to avoid an infinite loop if the frame buffer length is consistently below the threshold. This happened for one of my boards and caused the flash led being damaged due to being on for a long period.

Changes

johnyHV commented 1 month ago
  1. step this is not the redundancy. this sequence was for obtaining a training photo. There used to be a problem with the fact that the current photo was one frame behind. maybe it is not currently needed. I have to check it.

Everything else looks good. I need verify the functionality of the changes and then I can confirm them

johnyHV commented 1 month ago

@Anheledir I tried it, without this sequence of code:

/* get training photo */
FrameBuffer = esp_camera_fb_get();
esp_camera_fb_return(FrameBuffer);

But I still have a problem with the current photo being one frame behind. So it's neccesary keep this code sequence. I already had to partially modify function for the get photo. The release of the photo is not performed in this function.

I suggest the following procedure: I will confirm your pull request, then I will add the sequence for the training photo and remove the release of the photo. You added good protection against looping and improved the semaphore. We need to keep that.

It's okay for you ?

Anheledir commented 1 month ago

Sure, sounds good for me. I oversaw that issue you observed, would recommend to add also a concern to the code to explain why this seemingly unnecessary snippet is necessary!

Glad you approve the other changes, I really like your whole effort with this project! 👍🏻

johnyHV commented 1 month ago

This is good point. I will add the reason for using this code.

Thanks, I'm glad you like this project. and thanks for improving the code