Skip to content

Update camera example for support LED Intensity #6791

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

Update camera example for support LED Intensity #6791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
74bf57f
Update camera example for support LED Intensity
DmytroKorniienko May 23, 2022
f5dffd3
Revert "Update camera example for support LED Intensity"
DmytroKorniienko May 25, 2022
0aeed97
fix
DmytroKorniienko May 25, 2022
5fde24a
Merge branch 'master' into master
SuGlider May 26, 2022
55a27b5
Merge branch 'master' into master
SuGlider May 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions libraries/ESP32/examples/Camera/CameraWebServer/CameraWebServer.ino
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ const char* ssid = "**********";
const char* password = "**********";

void startCameraServer();
void ledc_setup();

void setup() {
Serial.begin(115200);
Expand Down Expand Up @@ -139,6 +140,8 @@ void setup() {
Serial.print("Camera Ready! Use 'http://");
Serial.print(WiFi.localIP());
Serial.println("' to connect");

ledc_setup(); // uncomment CONFIG_LED_ILLUMINATOR_ENABLED in app_httpd.cpp if you need this
}

void loop() {
Expand Down
22 changes: 22 additions & 0 deletions libraries/ESP32/examples/Camera/CameraWebServer/app_httpd.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,17 @@
#include "sdkconfig.h"
#include "camera_index.h"

//#define CONFIG_LED_ILLUMINATOR_ENABLED 1
#ifdef CONFIG_LED_ILLUMINATOR_ENABLED
#define FLASH_LED_GPIO GPIO_NUM_4
#define CONFIG_LED_MAX_INTENSITY 255
Copy link
Collaborator

Choose a reason for hiding this comment

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

CONFIG_LED_MAX_INTENSITY - defined but not used anywhere.

Copy link
Author

Choose a reason for hiding this comment

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

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

#define CONFIG_LED_LEDC_CHANNEL LEDC_CHANNEL_0
#endif

#ifdef CONFIG_LED_ILLUMINATOR_ENABLED
#include "esp32-hal-ledc.h"
#endif

#if defined(ARDUINO_ARCH_ESP32) && defined(CONFIG_ARDUHAL_ESP_LOG)
#include "esp32-hal-log.h"
#define TAG ""
Expand Down Expand Up @@ -1304,3 +1315,14 @@ void startCameraServer()
httpd_register_uri_handler(stream_httpd, &stream_uri);
}
}

void ledc_setup()
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 :)

{
#ifdef CONFIG_LED_ILLUMINATOR_ENABLED
// PWM
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");
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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? :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

#endif
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

yes.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.