-
Notifications
You must be signed in to change notification settings - Fork 0
Add hello_ei sample #21
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.
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
base: main
Are you sure you want to change the base?
Conversation
Add a new sample application demonstrating Edge Impulse machine learning integration on nRF54L15DK. The sample is based on edge_impulse/wrapper sample from NCS. However, it does not use ei_wrapper library from NCS, but uses a very simplified local wrapper for EI library. Its purpose is to separate C application code from C++ EI library. The management of input data is handled by the application, not by the wrapper anymore. Features: - Modular build system with separate CMakeLists for app and Edge Impulse library. - C/C++ wrapper interface (ei_wrapper) providing C-compatible API for ML operations. - Support for multiple inference runs. - Configurable sample buffer. - 2 test data sets (sine and traingle wave) for inference validation. Technical details: - Uses ExternalProject to download, patch and build Edge Impulse SDK. - Full Newlib C library for C++ standard library support (cmath, cfloat, etc.). Tested on: nRF54L15DK Signed-off-by: Bartosz Meus <[email protected]>
Signed-off-by: Bartosz Meus <[email protected]>
Signed-off-by: Bartosz Meus <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently this is taken as-is from wrapper sample
| CONFIG_MAIN_STACK_SIZE=2048 | ||
|
|
||
| # Use URL to fetch Edge Impulse library from the Internet or local path | ||
| CONFIG_EDGE_IMPULSE_URI="edge_impulse/nrf_accel_sim-v35.zip" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EI lib should be updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should try to simplify this CMake and the template. For example we are no longer downloading zip
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that can be simplified for sure, or at least should be documented in a better way.
I'm not sure about download functionality though. I mean, we probably would like to keep SDK deployed as a zip (?). Then we also need to extract this archive and ExternalProject_Add() gives us both things (extract & download) "for free". The zip provided with sample can be a local file, but with a possibility to retrieve it from somewhere else if a user wants it.
I prepared a following commit that documents this file a little bit more, extracts URI handling to separate cmake file.
| target_include_directories(edge_impulse INTERFACE | ||
| ${EDGE_IMPULSE_SOURCE_DIR} | ||
| ${EDGE_IMPULSE_SOURCE_DIR}/tflite-model | ||
| ${EDGE_IMPULSE_SOURCE_DIR}/model-parameters | ||
| ${EDGE_IMPULSE_SOURCE_DIR}/edge-impulse-sdk/ | ||
| ${EDGE_IMPULSE_SOURCE_DIR}/edge-impulse-sdk/third_party/ruy | ||
| ${EDGE_IMPULSE_SOURCE_DIR}/edge-impulse-sdk/third_party/gemmlowp | ||
| ${EDGE_IMPULSE_SOURCE_DIR}/edge-impulse-sdk/third_party/flatbuffers/include | ||
| ${EDGE_IMPULSE_SOURCE_DIR}/edge-impulse-sdk/third_party | ||
| ${EDGE_IMPULSE_SOURCE_DIR}/edge-impulse-sdk/tensorflow | ||
| ${EDGE_IMPULSE_SOURCE_DIR}/edge-impulse-sdk/dsp | ||
| ${EDGE_IMPULSE_SOURCE_DIR}/edge-impulse-sdk/classifier | ||
| ${EDGE_IMPULSE_SOURCE_DIR}/edge-impulse-sdk/anomaly | ||
| ${EDGE_IMPULSE_SOURCE_DIR}/edge-impulse-sdk/CMSIS/NN/Include | ||
| ${EDGE_IMPULSE_SOURCE_DIR}/edge-impulse-sdk/CMSIS/DSP/PrivateInclude | ||
| ${EDGE_IMPULSE_SOURCE_DIR}/edge-impulse-sdk/CMSIS/DSP/Include | ||
| ${EDGE_IMPULSE_SOURCE_DIR}/edge-impulse-sdk/CMSIS/Core/Include | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this involve internal structure of EI SDK, I think this should be in template file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not possible. Template file is used for replacing main cmake file in EI SDK. It's used for compiling a static lib and it already keeps track of include directories. These include directories here are needed for the interface library that is used as a "glue" between libedge_impulse.a and the application. This way app knows where to look for headers.
However, the zephyr interface library doesn't need all the paths - just ${EDGE_IMPULSE_SOURCE_DIR} would be enough if the app refers to SDK EI using full paths (e.g. "edge-impulse-sdk/classifier/ei_classifier_types.h").
samples/hello_ei/src/ei_wrapper.cpp
Outdated
| #if !CONFIG_ZTEST | ||
| /* Fixes warnings about redefinition of Zephyr ROUND_UP macro. */ | ||
| #ifdef ROUND_UP | ||
| #undef ROUND_UP | ||
| #endif | ||
| #endif /* !CONFIG_ZTEST */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrapper in no longer tested with ZTEST
samples/hello_ei/CMakeLists.txt
Outdated
| project(hello_ei) | ||
|
|
||
| # Add Edge Impulse library as a separate module | ||
| add_subdirectory(edge_impulse) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: move it next to link library
samples/hello_ei/src/main.c
Outdated
| #include <zephyr/kernel.h> | ||
| #include <zephyr/logging/log.h> | ||
|
|
||
| #include "ei_classifier_types.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use more complete path of <edge-impulse-sdk/classifier/ei_classifier_types.h>. We already do this for model metadata. Note the angle brackets as EI SDK is build a library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, we can switch to using complete path, then we can reduce number of include directories in cmake files.
I'm not sure about angle bracket though. If we try to simplify cmake system for this sample (compile SDK into app target), this won't be external library anymore. Even if we leave the current approach (SDK as static lib linked against app), the lib still will be local to the sample application, not a system-wide one, so I guess it's acceptable to leave quotes instead of angle brackets.
samples/hello_ei/src/ei_wrapper.cpp
Outdated
| features_signal.get_data = get_data_cbk; | ||
| features_signal.total_length = window_size; | ||
|
|
||
| EI_IMPULSE_ERROR err = run_classifier(&features_signal, ei_result, DEBUG_MODE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In file edge-impulse-sdk/classifier/ei_run_classifier_c.h there is already extern "C" function ei_run_classifier. Maybe we can get rid of this wrapper entirely.
Check also https://github.com/edgeimpulse/example-standalone-inferencing-c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point, I managed to get rid off it.
| # This targets remove the `edge_impulse_project-download` stamp file, which | ||
| # causes the Edge impulse library to be fetched on each build invocation. | ||
| # Note: This also results in the `ALL` target to always be considered out-of-date. | ||
| if(CONFIG_EDGE_IMPULSE_DOWNLOAD_ALWAYS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we either remove it or add to Kconfig and actually do download?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I restored it for now
| @@ -0,0 +1,81 @@ | |||
| # | |||
| # Copyright (c) 2020 Nordic Semiconductor ASA | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would either put year 2026 or move whole commit history of that file and leave old date.
IMO second option is not worth effort. You can mention authors of previous implementation in commit message.
Same for other files.
samples/hello_ei/src/ei_wrapper.cpp
Outdated
|
|
||
| #include "ei_wrapper.h" | ||
|
|
||
| #define DEBUG_MODE IS_ENABLED(CONFIG_EI_WRAPPER_DEBUG_MODE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove this define and use IS_ENABLED() as we use it in only one place.
samples/hello_ei/src/main.c
Outdated
|
|
||
| print_inference_result(&inference_result, delta); | ||
|
|
||
| // Keep track of how many inferences have been performed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are supposed to use /* */ comment style for multi line comments.
Also in other places
samples/hello_ei/src/main.c
Outdated
| LOG_MODULE_REGISTER(hello_ei); | ||
|
|
||
| #define INPUT_WINDOW_SIZE EI_CLASSIFIER_DSP_INPUT_FRAME_SIZE | ||
| #define LABEL_COUNT EI_CLASSIFIER_LABEL_COUNT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You define it here and later you still use EI_CLASSIFIER_LABEL_COUNT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for EI_CLASSIFIER_DSP_INPUT_FRAME_SIZE
Used in print_model_info(void)
| CONFIG_UART_CONSOLE=y | ||
| CONFIG_RTT_CONSOLE=n | ||
|
|
||
| # Enable Edge Impulse dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it isn't more neat to have it under Kconfig dependencies/selects.
This would require however config EDGE_IMPULSE so if you think otherwise leave it as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get an idea, but I'll postpone this decision until the way of handling SDK (standalone lib vs in-app) is clarified.
| INTERFACE -Wno-unused | ||
| ) | ||
|
|
||
| if(CONFIG_EI_WRAPPER) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also needs fixing/removing
samples/hello_ei/sample.yaml
Outdated
| harness_config: | ||
| ordered: true | ||
| regex: | ||
| - "Classification results" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regex needs to be updated
samples/hello_ei/sample.yaml
Outdated
| - "Anomaly: (-[0-9]+[.][0-9]+)|(0[.]0[0-9]+)" | ||
| type: multi_line | ||
| platform_allow: | ||
| - nrf52dk/nrf52832 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we support all SoCs? I guess we can drop nrf52 and maybe some other platforms? This probably should be clarified on meeting with PMs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree, for now it was just copied from the wrapper sample. I'll add this to the question list being prepared.
- Edge Impulse C linkage used intead of C++ wrapper. - Fixed comment for format, updated copyright notices. - Removed nRF52 targets. - Chanaged twister regex (still to be improved). - Restored EDGE_IMPULSE_DOWNLOAD_ALWAYS option. Signed-off-by: Bartosz Meus <[email protected]>
|
WIP, I'm still working on part of the issues raised in PR |
…modes Restructure Edge Impulse library integration to support two build configurations: 1. Compile into app (CONFIG_EDGE_IMPULSE_COMPILE_INTO_APP=y): - Uses FetchContent to compile EI sources directly into application - Faster incremental builds and better optimization (LTO) - Warning suppression applied per-file to EI sources only 2. Standalone library (CONFIG_EDGE_IMPULSE_COMPILE_INTO_APP=n): - Uses ExternalProject to build EI as separate static library - Better build caching and reusability across multiple apps - Isolated build context with independent compiler flags Changes: - Split monolithic CMakeLists.txt into modular files: * ei_sdk_utils.cmake: URI processing and API key handling * ei_compile_in_app.cmake: FetchContent-based integration * ei_standalone_lib.cmake: ExternalProject-based integration - Add Kconfig option CONFIG_EDGE_IMPULSE_COMPILE_INTO_APP - Simplify main CMakeLists.txt to conditional include logic - Update prj.conf and main.c with minor improvements - Reduce number of include directories Signed-off-by: Bartosz Meus <[email protected]>
|
The latest commit introduces dual compilation mode - SDK compiled into app vs SDK kept as a static lib. It doesn't mean I push this as an ultimate solution, it's kept for purpose of comparison of 2 options. Personally, I like the simplicity of a version with SDK compiled into application (without patching, additional interface lib target, exported lib target etc.), but I wonder if there might be some use case or significant benefit of keeping also option with isolated SDK binary. |
This pull request proposes a new "Hello Edge Impulse" sample application for Zephyr, demonstrating how to integrate and use the Edge Impulse machine learning library on Nordic Semiconductor devices. The sample includes CMake build integration to fetch and build the Edge Impulse library, configuration options, sample input data, and a main application that runs inferences on example datasets.
The samples replaces
wrappersample from nrf repository in a way that it demonstrates running inference with Edge Impulse. However, it is a simplified version, without extensive abstraction layer (ei_wrapperlibrary) for interaction with Edge Impulse. Instead, C linkage of EI library is utilized, so no need to introduce any wrapper layer. This means that memory management or time measurements in the new sample are handled by application itself, not external component (library).Another difference between new sample and
wrappersamples is existence of 2 input signals compiled-in (sine wave and triangle wave). Both signals are extended with 8 additional elements at the end, to allow multiple inferences.Jira: NCSDK-36946