Esp insights library support #7566

sanketwadekar · 2022-12-09T09:42:20Z

Description of Change

This PR brings library support for ESP Insights in Arduino.

Tests scenarios

I have tested this library on esp32 dev module.

Related links

Depends on other PRs
espressif/esp32-arduino-lib-builder#100
#7567

sanketwadekar · 2022-12-09T09:43:59Z

Tagging @shahpiyushv and @vikramdattu

me-no-dev · 2022-12-10T01:57:41Z

lib-builder changes are merged in master (and pulled here)

me-no-dev · 2022-12-10T02:26:50Z

There are some errors: https://github.com/espressif/arduino-esp32/actions/runs/3662188925/jobs/6191099956#step:5:255

sanketwadekar · 2022-12-13T08:17:39Z

There are some errors: https://github.com/espressif/arduino-esp32/actions/runs/3662188925/jobs/6191099956#step:5:255

The errors are coming from esp-insights repo. Some changes are required in esp-insights for this pipeline to pass. MR for these changes has been raised.

me-no-dev · 2022-12-13T09:05:56Z

The errors are coming from esp-insights repo. Some changes are required in esp-insights for this pipeline to pass. MR for these changes has been raised.

Then this means that even the changes in lib-builder were too early to post. When will that MR get merged and changes available on github?

sanketwadekar · 2022-12-14T03:46:57Z

The MR should get merged in this week.

me-no-dev · 2022-12-21T00:36:26Z

@sanketwadekar after a couple of minor fixes, CI is passing. Onto review :)

me-no-dev · 2022-12-21T00:37:17Z

docs/source/api/insights.rst

@@ -0,0 +1,767 @@
+############


@pedrominatel PTAL

me-no-dev · 2022-12-21T00:42:10Z

libraries/Insights/examples/DiagnosticsSmokeTest/DiagnosticsSmokeTest.ino

+            if (s_reset_count > MAX_CRASHES) {
+                ESP_DIAG_EVENT(TAG, "[count][%d]", count);
+            } else {
+               log_e("[count][%d] [crash_count][%"PRIu32"] [excvaddr][0x0f] Crashing...", count, s_reset_count);


warnings are generated if there is no space between quoted text/defines.

log_e("[count][%d] [crash_count][%" PRIu32 "] [excvaddr][0x0f] Crashing...", count, s_reset_count);

me-no-dev · 2022-12-21T00:42:42Z

libraries/Insights/examples/DiagnosticsSmokeTest/DiagnosticsSmokeTest.ino

+            void *p = malloc(size);
+            if (p) {
+                memset(p, size, 'A' + (esp_random() % 26));
+                log_i("Allocated %"PRIu32" bytes", size);


Same note as above

libraries/Insights/examples/DiagnosticsSmokeTest/DiagnosticsSmokeTest.ino

me-no-dev · 2022-12-21T00:44:56Z

libraries/Insights/examples/MinimalDiagnostics/MinimalDiagnostics.ino

+    Serial.begin(115200);
+    WiFi.mode(WIFI_STA);
+    WiFi.begin(WIFI_SSID, WIFI_PASSPHRASE);
+    if(!WiFi.isConnected()) {


same note about how to wait for WiFi connect

me-no-dev · 2022-12-21T00:48:33Z

platform.txt

@@ -24,6 +24,9 @@ tools.esp_ota.cmd.windows="{runtime.platform.path}/tools/espota.exe" -r
 tools.gen_esp32part.cmd=python3 "{runtime.platform.path}/tools/gen_esp32part.py"
 tools.gen_esp32part.cmd.windows="{runtime.platform.path}/tools/gen_esp32part.exe"

+tools.gen_insights_pkg.cmd = python3 {runtime.platform.path}/tools/gen_insights_package.py
+tools.gen_insights_pkg.cmd.windows = {runtime.platform.path}/tools/gen_insights_package.exe


I suggest you quote and format the commands as the ones above in order to prevent possible path/etc issues.

I get errors if I add quotes

/var/folders/0l/c75rh0953fng_6ccdgnm48gr0000gn/T/arduino_build_640628: -c: line 0: unexpected EOF while looking for matching `"' /var/folders/0l/c75rh0953fng_6ccdgnm48gr0000gn/T/arduino_build_640628: -c: line 1: syntax error: unexpected end of file

Can you please help me out with this?

Also, I need some help to test this on Windows.
Thanks

@SuGlider please help with testing on Windows

me-no-dev · 2022-12-21T00:49:21Z

platform.txt

@@ -211,10 +214,15 @@ recipe.c.combine.pattern="{compiler.path}{compiler.c.elf.cmd}" "-Wl,--Map={build
 recipe.objcopy.partitions.bin.pattern={tools.gen_esp32part.cmd} -q "{build.path}/partitions.csv" "{build.path}/{build.project_name}.partitions.bin"

 ## Create bin
-recipe.objcopy.bin.pattern_args=--chip {build.mcu} elf2image --flash_mode "{build.flash_mode}" --flash_freq "{build.flash_freq}" --flash_size "{build.flash_size}" -o "{build.path}/{build.project_name}.bin" "{build.path}/{build.project_name}.elf"
+recipe.objcopy.bin.pattern_args=--chip {build.mcu} elf2image --flash_mode "{build.flash_mode}" --flash_freq "{build.flash_freq}" --flash_size "{build.flash_size}" --elf-sha256-offset 0xb0 -o "{build.path}/{build.project_name}.bin" "{build.path}/{build.project_name}.elf"


--elf-sha256-offset 0xb0 Is this change necessary? What does it do?

Hi @me-no-dev the sha256 of firmware is needed for debugging by ESP-Insights backend.

esptool.py from ESP-IDF inserts sha256 by default and is implicit. In case of Arduino however this change was not present. Maybe it was not found useful to add this at the point.

https://github.com/espressif/esp-idf/blob/master/components/esptool_py/project_include.cmake#L48

@ivankravets @valeros PTAL. This is needed for elf2bin part

I guess it will suffice to add a hot fix directly to the PlatformIO build script in this repo until a new version of the core with this feature is released.

@sanketwadekar Please add the following snippet somewhere at the bottom of https://github.com/espressif/arduino-esp32/blob/master/tools/platformio-build.py:

# # Adjust the `esptoolpy` command in the `ElfToBin` builder with firmware checksum offset # action = deepcopy(env["BUILDERS"]["ElfToBin"].action) action.cmd_list = env["BUILDERS"]["ElfToBin"].action.cmd_list.replace( "-o", "--elf-sha256-offset 0xb0 -o" ) env["BUILDERS"]["ElfToBin"].action = action

Also, an extra import should be added at the top of the same file:

from copy import deepcopy

Thanks!

me-no-dev · 2022-12-21T00:50:24Z

platform.txt

 recipe.objcopy.bin.pattern="{tools.esptool_py.path}/{tools.esptool_py.cmd}" {recipe.objcopy.bin.pattern_args}
 recipe.objcopy.bin.pattern.linux=python3 "{tools.esptool_py.path}/{tools.esptool_py.cmd}" {recipe.objcopy.bin.pattern_args}

+## Create Insights Firmware Package
+recipe.hooks.objcopy.postobjcopy.1.pattern_args={build.path} {build.project_name} {build.path}


wouldn't it make more sense to output the zip to the sketch folder instead?

me-no-dev · 2022-12-21T01:14:11Z

boards.txt

@@ -672,6 +672,9 @@ esp32.menu.PartitionScheme.fatflash.upload.maximum_size=2097152
 esp32.menu.PartitionScheme.app3M_fat9M_16MB=16M Flash (3MB APP/9.9MB FATFS)
 esp32.menu.PartitionScheme.app3M_fat9M_16MB.build.partitions=app3M_fat9M_16MB
 esp32.menu.PartitionScheme.app3M_fat9M_16MB.upload.maximum_size=3145728
+esp32.menu.PartitionScheme.insights=Insights
+esp32.menu.PartitionScheme.insights.build.partitions=insights
+esp32.menu.PartitionScheme.insights.upload.maximum_size=3145728


this should be added to the other dev boards as well

vikramdattu · 2022-12-21T06:40:31Z

libraries/Insights/src/Diagnostics.h

@@ -0,0 +1,28 @@
+// Copyright 2015-2022 Espressif Systems (Shanghai) PTE LTD


Let's change the license header to SPDX format and attribute year 2022 only.

/* * SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */

vikramdattu · 2022-12-21T06:40:59Z

libraries/Insights/src/Diagnostics.h

+};
+
+extern DiagnosticsClass Diagnostics;
+#endif


minor: new lines missing at multiple places

vikramdattu · 2022-12-21T06:41:39Z

libraries/Insights/src/Metrics.cpp

@@ -0,0 +1,139 @@
+#include "sdkconfig.h"


Please add license header to all files.

me-no-dev · 2022-12-21T13:14:14Z

tools/partitions/app3M_fat9M_16MB.csv

@@ -4,4 +4,5 @@ otadata,  data, ota,     0xe000,  0x2000,
 app0,     app,  ota_0,   0x10000, 0x300000,
 app1,     app,  ota_1,   0x310000,0x300000,
 ffat,     data, fat,     0x610000,0x9F0000,
-# to create/use ffat, see https://github.com/marcmerlin/esp32_fatfsimage
+coredump, data, coredump, , 64K,
+# to create/use ffat, see https://github.com/marcmerlin/esp32_fatfsimage


if you do not lower the size of the last partition, coredump will end up outside of the flash chip

Would leaving it blank cause it to automatically take up the rest of the space?

afraid not. You do need to subtract 64K from the size

me-no-dev · 2022-12-21T13:14:32Z

tools/partitions/insights.csv

+nvs,       data,  nvs,     0x9000,   0x5000,
+otadata,   data,  ota,     0xe000,   0x2000,
+ota_0,     app,   ota_0,   0x10000,  0x1E0000,
+coredump, data, coredump, , 64K,


we should not need this anymore?

Have you tested any Insights examples with other partitions? If it works well, then this can be removed.

I have not tried because look at my previous comment: "if you do not lower the size of the last partition, coredump will end up outside of the flash chip"

pedrominatel

Here are my comments and suggestions, otherwise LGTM.

docs/source/api/insights.rst

libraries/Insights/examples/DiagnosticsSmokeTest/README.md

docs/source/api/insights.rst

builder

urish · 2023-01-08T22:11:24Z

After upgrading Wokwi to arduino-esp32 2.0.6, users started getting the following errors in the serial monitor:

E (10) esp_core_dump_flash: No core dump partition found!
E (10) esp_core_dump_flash: No core dump partition found!

the code still runs successfully. I'm not sure if we should add a core dump partition to the default partition table that Wokwi generates?
The title of this pull request says "Draft:", so is it final or still work-in-progress?

me-no-dev · 2023-01-09T09:50:44Z

Hi @urish . I guess github does not remove the string "Draft:" even thou it added it. Anyway, this is already a part of 2.0.6, so please add a 64KB coredump partition to wokwi

urish · 2023-01-09T10:29:41Z

Thanks! Now getting:

E (10) esp_core_dump_flash: Failed to read core dump data size (258)!

Should we also initialize the partition somehow?

urish · 2023-01-09T10:46:11Z

Never mind, it turns out the partition offset was incorrect. Now it works well!

sanketwadekar mentioned this pull request Dec 9, 2022

log_printf: Restructured log_printf for wrapping #7567

Merged

sanketwadekar force-pushed the esp_insights branch from 1a9a9f4 to 8210dd3 Compare December 9, 2022 10:10

VojtechBartoska added Type: Feature request Feature request for Arduino ESP32 Area: Libraries Issue is related to Library support. labels Dec 9, 2022

sanketwadekar added 5 commits December 13, 2022 13:17

ESP Insights: Added library support

0116e1d

ESP Insights: Added Examples

87d179e

ESP Insights: Added custom partitions file

48a6b7b

ESP Insights: Added API documentation.

2456199

Added recipe and script to create Insights package

76977d1

sanketwadekar force-pushed the esp_insights branch from 6b88ede to 2456199 Compare December 13, 2022 08:04

me-no-dev and others added 3 commits December 21, 2022 00:24

Merge branch 'master' into esp_insights

87accc0

Fix postobjcopy script on UNIX

86e0600

Fix VariablesClass::deinitNetworkVariables not returning

10f7c9a

me-no-dev reviewed Dec 21, 2022

View reviewed changes

docs/source/api/insights.rst

@@ -0,0 +1,767 @@

############

Copy link

Member

me-no-dev Dec 21, 2022

Choose a reason for hiding this comment

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

@pedrominatel PTAL

me-no-dev reviewed Dec 21, 2022

View reviewed changes

libraries/Insights/examples/DiagnosticsSmokeTest/DiagnosticsSmokeTest.ino Show resolved Hide resolved

me-no-dev reviewed Dec 21, 2022

View reviewed changes

vikramdattu reviewed Dec 21, 2022

View reviewed changes

sanketwadekar force-pushed the esp_insights branch from 7e20e56 to 98ae75c Compare December 21, 2022 07:53

vikramdattu approved these changes Dec 21, 2022

View reviewed changes

me-no-dev and others added 6 commits December 21, 2022 14:09

Small script fixes

6dcea99

Merge branch 'master' into esp_insights

82620a0

Fix Insights package for Windows

8e7c562

Updated .exe of insights script

d273cbb

Added coredump partition to all schemes

edfd14f

Updated header files of Insights diagnostics

c501198

me-no-dev reviewed Dec 21, 2022

View reviewed changes

pedrominatel self-requested a review December 21, 2022 14:36

pedrominatel approved these changes Dec 21, 2022

View reviewed changes

me-no-dev marked this pull request as ready for review December 21, 2022 18:00

sanketwadekar and others added 9 commits December 22, 2022 10:12

Added example to insights doc

d9d344e

hotfix: Added elf-sha256-offset flag in elf2bin

595da64

builder

Merge branch 'master' into esp_insights

edc9b09

Update API to be more Arduino-like and partitions offsets

14d9c41

Revert adding insights partitions

ca0b512

Fix docs

07b985e

minor fixed of example and docs

a4d6a40

Update DiagnosticsSmokeTest.ino

fb38f84

Update Insights.cpp

b1a36ba

me-no-dev merged commit 161b167 into espressif:master Dec 22, 2022

mrengineer7777 mentioned this pull request Jan 25, 2023

Verify firmware does not work after update. #7755

Closed

1 task

VojtechBartoska changed the title ~~Draft: Esp insights library support~~ Esp insights library support Apr 27, 2023

Jason2866 mentioned this pull request Dec 30, 2023

OTA compatibility issue #9049

Closed

1 task

		@@ -0,0 +1,28 @@
		// Copyright 2015-2022 Espressif Systems (Shanghai) PTE LTD

Esp insights library support #7566

Esp insights library support #7566

Conversation

sanketwadekar commented Dec 9, 2022 • edited Loading

Description of Change

Tests scenarios

Related links

sanketwadekar commented Dec 9, 2022

me-no-dev commented Dec 10, 2022

me-no-dev commented Dec 10, 2022

sanketwadekar commented Dec 13, 2022

me-no-dev commented Dec 13, 2022

sanketwadekar commented Dec 14, 2022

me-no-dev commented Dec 21, 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

sanketwadekar Dec 21, 2022 • edited Loading

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

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

pedrominatel left a comment

Choose a reason for hiding this comment

urish commented Jan 8, 2023

me-no-dev commented Jan 9, 2023

urish commented Jan 9, 2023

urish commented Jan 9, 2023

sanketwadekar commented Dec 9, 2022 •

edited

Loading

sanketwadekar Dec 21, 2022 •

edited

Loading