Update camera example for support LED Intensity #6791

DmytroKorniienko · 2022-05-23T11:35:37Z

Closes #6790

CLAassistant · 2022-05-23T11:35:45Z

All committers have signed the CLA.

libraries/ESP32/examples/Camera/CameraWebServer/CameraWebServer.ino

me-no-dev · 2022-05-25T08:14:38Z

My 2 cents:

BrownOut should not be disabled here at all. Especially since you are using WiFi. BrownOut issues should be resolved in hardware. It is after all a hardware issue.
You have modified way more than just disabling BO and adding ledc init. Revert all other modifications in order to continue the discussion. Changing the example structure it outside of the scope of the issue.
It is hard to grasp what is the actual problem by reading all comments. Is it that the LED powers on full brightness and that causes the BO? If that is the case, then digitalWrite(LED, level_to_turn_off) in the beginning of setup() should be sufficient to turn off the LED

This reverts commit 74bf57f.

DmytroKorniienko · 2022-05-25T11:53:54Z

My 2 cents:

BrownOut should not be disabled here at all. Especially since you are using WiFi. BrownOut issues should be resolved in hardware. It is after all a hardware issue.

You have modified way more than just disabling BO and adding ledc init. Revert all other modifications in order to continue the discussion. Changing the example structure it outside of the scope of the issue.

It is hard to grasp what is the actual problem by reading all comments. Is it that the LED powers on full brightness and that causes the BO? If that is the case, then digitalWrite(LED, level_to_turn_off) in the beginning of setup() should be sufficient to turn off the LED

Changes reverted and new commit prepared
BO not related with a LED but anyway I deleted this code part

SuGlider · 2022-05-26T13:55:33Z

libraries/ESP32/examples/Camera/CameraWebServer/app_httpd.cpp

+//#define CONFIG_LED_ILLUMINATOR_ENABLED 1
+#ifdef CONFIG_LED_ILLUMINATOR_ENABLED
+#define FLASH_LED_GPIO GPIO_NUM_4
+#define CONFIG_LED_MAX_INTENSITY 255


CONFIG_LED_MAX_INTENSITY - defined but not used anywhere.

CONFIG_LED_MAX_INTENSITY - defined but not used anywhere.

It used for UI led intensity limit

#ifdef CONFIG_LED_ILLUMINATOR_ENABLED void enable_led(bool en) { // Turn LED On or Off int duty = en ? led_duty : 0; if (en && isStreaming && (led_duty > CONFIG_LED_MAX_INTENSITY)) { duty = CONFIG_LED_MAX_INTENSITY; } ledc_set_duty(CONFIG_LED_LEDC_SPEED_MODE, CONFIG_LED_LEDC_CHANNEL, duty); ledc_update_duty(CONFIG_LED_LEDC_SPEED_MODE, CONFIG_LED_LEDC_CHANNEL); ESP_LOGI(TAG, "Set LED intensity to %d", duty); } #endif

SuGlider · 2022-05-26T13:56:40Z

libraries/ESP32/examples/Camera/CameraWebServer/app_httpd.cpp

+#else
+  ESP_LOGI(TAG, "Flash led illumination is not configured");
+#endif
+}


I think that all this code should go to CameraWebServer.ino instead of here.

I think that all this code should go to CameraWebServer.ino instead of here.

Please take a look - CONFIG_LED_ILLUMINATOR_ENABLED is defined in a app_httpd.cpp file and MAY be undifined on compile time for CameraWebServer.ino since compile order may be various. So we have an issue in this case. This can be resolved by moving defines in the header file (as I suggest in the fist commit) but:

Changing the example structure it outside of the scope of the issue.

So what is your desision for this case? Move all defines in the one CameraWebServer.ino or separate header or something else?

cc: @me-no-dev

Actually ... CONFIG_LED_ILLUMINATOR_ENABLED is defined nowhere.
I also searched it in sdkconfig and there is nothing there as well.

This is a sort of loose example, maybe coming from IDF.

I changed my mind... just leave it there. I see other code lines in app_httpd.cpp that test CONFIG_LED_ILLUMINATOR_ENABLED.

SuGlider · 2022-05-26T13:57:47Z

libraries/ESP32/examples/Camera/CameraWebServer/app_httpd.cpp

+  ledcSetup(LEDC_CHANNEL_0, 4000, LEDC_TIMER_8_BIT);
+  ledcAttachPin(FLASH_LED_GPIO, LEDC_CHANNEL_0);
+#else
+  ESP_LOGI(TAG, "Flash led illumination is not configured");


Please use Arduino function log_i(...) instead of IDF ESP_LOGI()

Please use Arduino function log_i(...) instead of IDF ESP_LOGI()

There are more then one line with ESP_LOGI (I think example was ported from IDF), so all must be changed or only this one or maybe leave as is? :)

I see... it is used all over in app_httpd.cpp.
If possible, it should be changed everywhere.

Let's keep it simple for now. Keep it using ESP_LOGI()

SuGlider · 2022-05-26T13:59:06Z

libraries/ESP32/examples/Camera/CameraWebServer/app_httpd.cpp

@@ -1304,3 +1315,14 @@ void startCameraServer()
        httpd_register_uri_handler(stream_httpd, &stream_uri);
    }
 }
+
+void ledc_setup()


ledc_setup() could have as parameters all the information declared in the #define, making it more generic and clear.

ledc_setup() could have as parameters all the information declared in the #define, making it more generic and clear.

This is possible of course, but requre a lot of changes and still requre a defines since some boards can have different GPIO pins for led or not have a led at all. I can prepare such changes if you ready to review it, but first of all please look to the comment above where discussing about example structure. Can I append new header file for defines or not? I will move forvard depending of answer.

Yes, ... maybe this example should be refactored for good.
Let's keep it simple for now. Keep it as you proposed.

This example is a copy of another example we have for ESP-IDF and turned into Arduino sketch. Structure allows changes if they make sense :)

SuGlider

Those are my suggestions for this PR.

SuGlider · 2022-05-27T20:28:03Z

This example seems to come from an IDF example/structure.
It has a bad structure for an Arduino example and uses CONFIG_XXX with ESP_LOGX() etc.

I found some other Arduino and IDF implementations that are based on this same example:
https://github.com/easytarget/esp32-cam-webserver
https://github.com/bkeevil/esp32-cam (also with the webrowser and an Iluminator component)

I think the whole code may need a refactoring and a review.
Not sure if @DmytroKorniienko wants to do that.

Given it only modifies a tiny piece in the example, I'm also not sure what is best to do with it as well...
@me-no-dev - I would like your opinion again! It seems that the very first code was a good Arduino code, but it got changed along the time and now it seems it need a deep review.

DmytroKorniienko · 2022-05-29T00:14:54Z

I think the whole code may need a refactoring and a review.
Not sure if @DmytroKorniienko wants to do that.

Unfortunately I'm not ready to spend a lot of time with this example. Reason is quite simple - this is not so interesting for me and I better continue work with my own project: https://github.com/DmytroKorniienko/esp32cam_EmbUI which has intesity support too and based on Asyncwebser. Feel free to close this PR and start new work with code refactoring if you want. No offense on my part :). I'm willing to make partial corrections, but not a complete rewrite of the example, because I don't think byte array-packed resources (index.html) are a good idea and prefer using a filesystem.

rodyeo · 2022-07-27T13:35:48Z

The ESP32-CAM Flight Light LED Illuminator solution which everyone is awaiting is now resolved by the blog author and programmer Bye Renzo

https://www.mischianti.org/forums/topic/esp32-cam-upgrade-camerawebserver-with-flash-on-esp32-framework-2-0-4/

Thank you ;)

VojtechBartoska · 2022-08-10T13:02:18Z

@PilnyTomas Please research shared resources and evaluate if the refactoring is needed and how. We can discuss later what are the options.

Related HW is available.

PilnyTomas · 2022-08-24T09:23:52Z

Have you tried compiling it for all the possible boards? Changes in PR should not break stuff for other users.
For example, S3 results in an error:
app_httpd.cpp:92:40: error: 'LEDC_HIGH_SPEED_MODE' was not declared in this scope

VojtechBartoska · 2022-09-16T12:52:06Z

any updates on this @DmytroKorniienko? Thanks

VojtechBartoska · 2022-09-21T13:05:33Z

Due to no answer, @PilnyTomas will take over and finish this PR.

DmytroKorniienko · 2022-09-22T01:41:01Z

Due to no answer, @PilnyTomas will take over and finish this PR.

Hi, I have some other tasks not related with opensource, so can't monitor PR with a so long lifecycle))). But I can take a look if this still necessary. I have ESP32-CAM and ESP32,ESP32-C3,ESP32-S2,ESP32-S3 without attached camera - so can only test ability to compile/link without errors. Let me know if this what you need, otherwise - feel free to do all needed work by yourself.

BTW Why CI hardware tests is skipped? I thought this easy can point to an errors and exclude routine checking...

DmytroKorniienko · 2022-09-23T11:26:34Z

@PilnyTomas

Have you tried compiling it for all the possible boards? Changes in PR should not break stuff for other users. For example, S3 results in an error: app_httpd.cpp:92:40: error: 'LEDC_HIGH_SPEED_MODE' was not declared in this scope

You can use LOW SPEED mode, then code compiled without errors at S3:
#define CONFIG_LED_ILLUMINATOR_ENABLED 1
#define CONFIG_LED_LEDC_LOW_SPEED_MODE

Moreover CONFIG_LED_ILLUMINATOR_ENABLED is DISABLED by default, so changes can't break something in an exising code. If you have any suggestions - please explain.

CC: @VojtechBartoska

PilnyTomas · 2022-09-23T11:59:19Z

Hi @DmytroKorniienko, the code should detect which board is used with #if CONFIG_IDF_TARGET_ESP32S3 and decide whether to enable some functionality or not. The aim is that the user will be able to compile for any SoC or board without any change in code.

DmytroKorniienko · 2022-09-24T22:47:10Z

Hi @DmytroKorniienko, the code should detect which board is used with #if CONFIG_IDF_TARGET_ESP32S3 and decide whether to enable some functionality or not. The aim is that the user will be able to compile for any SoC or board without any change in code.

Hi @PilnyTomas, as I said - CONFIG_LED_ILLUMINATOR_ENABLED is disabled by default. Are you think there is a good idea to enable it??? Some boards can be without leds at all, some have led on a other pin then ESP32_CAM module and at my opinion - impossible make universal variant for all cases. But if you want and going to do improvements - go ahead.

BTW: Instead of analyzing CONFIG_IDF_TARGET_ESP32S3 better check SOC_LEDC_SUPPORT_HS_MODE, look at hal/ledc_types.h:

typedef enum {
#if SOC_LEDC_SUPPORT_HS_MODE
    LEDC_HIGH_SPEED_MODE = 0, /*!< LEDC high speed speed_mode */
#endif
    LEDC_LOW_SPEED_MODE,      /*!< LEDC low speed speed_mode */
    LEDC_SPEED_MODE_MAX,      /*!< LEDC speed limit */
} ledc_mode_t;

P-R-O-C-H-Y · 2022-11-29T14:15:57Z

@DmytroKorniienko Thanks for your time, but closing this PR as I have created new PR #7533 with these changes.

Update camera example for support LED Intensity

74bf57f

SuGlider reviewed May 24, 2022

View reviewed changes

libraries/ESP32/examples/Camera/CameraWebServer/CameraWebServer.ino Outdated Show resolved Hide resolved

DmytroKorniienko added 2 commits May 25, 2022 14:40

Revert "Update camera example for support LED Intensity"

f5dffd3

This reverts commit 74bf57f.

fix

0aeed97

DmytroKorniienko requested a review from SuGlider May 25, 2022 14:21

SuGlider reviewed May 26, 2022

View reviewed changes

SuGlider requested changes May 26, 2022

View reviewed changes

SuGlider added 2 commits May 26, 2022 11:01

Merge branch 'master' into master

5fde24a

Merge branch 'master' into master

55a27b5

VojtechBartoska added Type: Example Issue is related to specific example. Status: Needs investigation We need to do some research before taking next steps on this issue labels Aug 10, 2022

VojtechBartoska assigned PilnyTomas Aug 10, 2022

VojtechBartoska added this to the 2.1.0 milestone Aug 24, 2022

VojtechBartoska unassigned PilnyTomas Nov 16, 2022

VojtechBartoska requested a review from P-R-O-C-H-Y November 16, 2022 13:41

VojtechBartoska assigned P-R-O-C-H-Y Nov 16, 2022

P-R-O-C-H-Y closed this Nov 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update camera example for support LED Intensity #6791

Update camera example for support LED Intensity #6791

DmytroKorniienko commented May 23, 2022

CLAassistant commented May 23, 2022 •

edited

Loading

me-no-dev commented May 25, 2022

DmytroKorniienko commented May 25, 2022

SuGlider May 26, 2022

DmytroKorniienko May 27, 2022

SuGlider May 26, 2022

me-no-dev May 27, 2022

DmytroKorniienko May 27, 2022

SuGlider May 27, 2022

SuGlider May 26, 2022

DmytroKorniienko May 27, 2022

SuGlider May 27, 2022

SuGlider May 27, 2022

SuGlider May 26, 2022

DmytroKorniienko May 27, 2022

SuGlider May 27, 2022

me-no-dev May 27, 2022

SuGlider left a comment

SuGlider commented May 27, 2022 •

edited

Loading

DmytroKorniienko commented May 29, 2022

rodyeo commented Jul 27, 2022

VojtechBartoska commented Aug 10, 2022

PilnyTomas commented Aug 24, 2022 •

edited

Loading

VojtechBartoska commented Sep 16, 2022

VojtechBartoska commented Sep 21, 2022

DmytroKorniienko commented Sep 22, 2022

DmytroKorniienko commented Sep 23, 2022

PilnyTomas commented Sep 23, 2022

DmytroKorniienko commented Sep 24, 2022

P-R-O-C-H-Y commented Nov 29, 2022

Update camera example for support LED Intensity #6791

Update camera example for support LED Intensity #6791

Conversation

DmytroKorniienko commented May 23, 2022

CLAassistant commented May 23, 2022 • edited Loading

me-no-dev commented May 25, 2022

DmytroKorniienko commented May 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuGlider left a comment

Choose a reason for hiding this comment

SuGlider commented May 27, 2022 • edited Loading

DmytroKorniienko commented May 29, 2022

rodyeo commented Jul 27, 2022

VojtechBartoska commented Aug 10, 2022

PilnyTomas commented Aug 24, 2022 • edited Loading

VojtechBartoska commented Sep 16, 2022

VojtechBartoska commented Sep 21, 2022

DmytroKorniienko commented Sep 22, 2022

DmytroKorniienko commented Sep 23, 2022

PilnyTomas commented Sep 23, 2022

DmytroKorniienko commented Sep 24, 2022

P-R-O-C-H-Y commented Nov 29, 2022

CLAassistant commented May 23, 2022 •

edited

Loading

SuGlider commented May 27, 2022 •

edited

Loading

PilnyTomas commented Aug 24, 2022 •

edited

Loading