Quirc esp-eye example (IEC-51) #240

FBEZ · 2023-09-19T14:23:49Z

Checklist

Component contains License
Component contains README.md
Component contains idf_component.yml file with url field defined
Component was added to upload job
Component was added to build job
Optional: Component contains unit tests
CI passing

Change description

It is an example for the quirc component using an esp-eye

CLAassistant · 2023-09-19T14:23:59Z

All committers have signed the CLA.

igrr

Hey @FBEZ, thanks for your PR!

I've left a few comments, please ping me if you have any questions or when I should review again.

igrr · 2023-09-19T14:40:45Z

quirc/examples/esp-eye-qrcode/main/include/app_peripherals.h

+
+#include "esp_camera.h"
+
+#define CONFIG_CAMERA_MODULE_ESP_EYE 1


I would recommend bringing board definitions using the board support package. This way to support a new board, work needs to be done only on BSP side, and the example doesn't have to be updated.

Example: https://github.com/espressif/qrcode-demo/blob/188cc80ca4e3654f597f00cdcb2193fad1709010/main/idf_component.yml#L4

(It's okay if that means that fewer boards are supported. After all, it's just an example.)

Thank you Ivan for your kind and detailed feedback.

Using the bsp approach would be the best choice for sure. Right now though I don't see the bsp for the esp-eye (meaning the older one). I could setup a very basic bsp for this board starting form the esp32-s3-eye and update this example accordingly.

I will receive and Esp32-s3-eye the next week to test this example on it anyway.

Ah yes, we don't seem to have a BSP for the original ESP-EYE.

I think at this point it's probably safe to recommend ESP32-S3-EYE for new applications, so I wouldn't mind keeping this example supported on that board only. Later when we add ESP-EYE BSP, we can update the example to support it as well.

Ok, then it will take a few days to receive the hardware and make the changes.
If it's better, I can close this PR here and reopen when everything it's in place. Otherwise I'll leave it here and update it in a few days.

You can keep this PR, just make the changes and push to the same branch when you are ready. Later once everything is ready you can clean up the commit history.

quirc/examples/esp-eye-qrcode/main/app_main.c

igrr · 2023-09-19T14:42:04Z

quirc/examples/esp-eye-qrcode/main/app_main.c

+
+static const char *TAG = "APP_CODE_SCANNER";
+
+static void decode_task()


FreeRTOS task function definition should look like this:

Suggested change

static void decode_task()

static void decode_task(void* arg)

igrr · 2023-09-19T14:42:24Z

quirc/examples/esp-eye-qrcode/main/app_main.c

+    }
+
+    quirc_destroy(q);
+}


vTaskDelete(NULL); missing here?

This part of the code isn't supposed to be reached. I'm not sure what the best approach would be here.
Do you suggest the vTaskDelete or to remove the quirc_destroy completely?

Depends on the error handling strategy — see the other comment about this.

If you decide to handle errors by terminating the task, probably having the cleanup path with quirc_destroy would make sense. You could use an out: goto label and use macros like ESP_GOTO_ON_ERROR to handle errors and perform cleanup.

If you decide to handle errors by aborting (which is not an unreasonable strategy for an example project) then quirc_destroy can be removed since it's unreachable.

Ok then since as you say it's an example, I think it's best to keep it the simplest possible. I removed the quirc_destroy.

igrr · 2023-09-19T14:43:49Z

quirc/examples/esp-eye-qrcode/main/app_main.c

+    // Initializing the quirc handle
+    struct quirc *q = quirc_new();
+    if (!q) {
+        ESP_LOGE(TAG,"Failed to allocate memory\n");


Newline at the end of format strings is not necessary for ESP_LOG, it's added automatically.

Also, missing space after comma. (Have you installed the pre-commit hooks using pre-commit install? They should automatically format the source files.)

igrr · 2023-09-19T14:45:19Z

quirc/examples/esp-eye-qrcode/main/app_main.c

+    struct quirc *q = quirc_new();
+    if (!q) {
+        ESP_LOGE(TAG,"Failed to allocate memory\n");
+        exit(1);


Error handling is a bit inconsistent. In the check above you are doing vTaskDelete(), here you are using exit. And in the check just below you only log the error code and continue.

Let's try to make this more consistent. You can decide the error handling strategy for this function — return or abort — and use the same approach in the whole function.

igrr · 2023-09-19T14:47:40Z

quirc/examples/esp-eye-qrcode/.devcontainer/Dockerfile

@@ -0,0 +1,48 @@
+FROM espressif/idf


No problem with devcontainers in general, but we usually don't add them directly to the example projects. Perhaps we can add one at project level, though. Still, I would prefer if this is done in a separate PR.

FBEZ · 2023-10-18T15:19:31Z

Hi there,
I've ported the example to the esp32-s3-eye through bsp. The image taken is also shown on the display to ease the pointing of the qr code.

FBEZ added 4 commits September 19, 2023 15:53

first commit

302913f

Changed output string, updated readme

a7e61d9

Changed output string, updated readme

e4616b8

Changed output string, updated readme

70ee9c4

github-actions bot changed the title ~~Quirc esp-eye example~~ Quirc esp-eye example (IEC-51) Sep 19, 2023

igrr requested changes Sep 19, 2023

View reviewed changes

FBEZ added 2 commits September 20, 2023 09:31

removed .devcontainers

018a689

removed unreachable code

88a3f31

FBEZ mentioned this pull request Sep 29, 2023

examples position for esp-bsp (BSP-385) espressif/esp-bsp#230

Open

FBEZ added 8 commits October 16, 2023 17:08

moved to esp32-s3 eye - decode failed ecc failure

f3e9132

added example for esp32-s3

e2f8508

removed esp-eye example

eff35ad

cleaned code, updated readme

4f787d0

Updated readme

d9f2fb6

removed sdconfig.defaults

d0043a2

added config

9b3ae06

updated readme

0cbc4af

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quirc esp-eye example (IEC-51) #240

Quirc esp-eye example (IEC-51) #240

FBEZ commented Sep 19, 2023

CLAassistant commented Sep 19, 2023 •

edited

igrr left a comment

igrr Sep 19, 2023

FBEZ Sep 20, 2023

igrr Sep 20, 2023

FBEZ Sep 20, 2023

igrr Sep 20, 2023

igrr Sep 19, 2023

igrr Sep 19, 2023

FBEZ Sep 20, 2023

igrr Sep 20, 2023

FBEZ Sep 20, 2023

igrr Sep 19, 2023

igrr Sep 19, 2023

igrr Sep 19, 2023

FBEZ commented Oct 18, 2023


		#include "esp_camera.h"

		#define CONFIG_CAMERA_MODULE_ESP_EYE 1


		static const char *TAG = "APP_CODE_SCANNER";

		static void decode_task()

Quirc esp-eye example (IEC-51) #240

Are you sure you want to change the base?

Quirc esp-eye example (IEC-51) #240

Conversation

FBEZ commented Sep 19, 2023

Checklist

Change description

CLAassistant commented Sep 19, 2023 • edited

igrr left a 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

Choose a reason for hiding this comment

FBEZ commented Oct 18, 2023

CLAassistant commented Sep 19, 2023 •

edited