-
Notifications
You must be signed in to change notification settings - Fork 0
Gesture recognition #20
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 gesture_recognition application as it is in: https://github.com/Neuton-tinyML/neuton-nordic-thingy53-ble-remotecontrol on commit: b2063f2ae42fa8aead6b3c7df477b3cda7caac29 Application was devloped by Raman Rusak. JIRA: NCSDK-36990 Signed-off-by: Jan Zyczkowski <[email protected]>
Fix application for NCS 3.2.0. Remove dupplicated USB stack. Leave only USB_DEVICE_STACK_NEXT which is enabled for thingy53 by deafult. JIRA: NCSDK-36990 Signed-off-by: Jan Zyczkowski [email protected]
JIRA: NCSDK-36990 Signed-off-by: Jan Zyczkowski [email protected]
Use EdgeAI API and library from repository instead of using a copy from application location. Remove copy of API and library from application location. JIRA: NCSDK-36990 Signed-off-by: Jan Zyczkowski [email protected]
|
I still have several issues which needs to be discussed:
|
| @@ -0,0 +1,24 @@ | |||
| Standard: Cpp11 | |||
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.
please use .clang-format from root of this repository
https://github.com/nrfconnect/sdk-edge-ai/blob/main/.clang-format
|
|
||
|  | ||
|
|
||
| ## Setup Software Environment <div id='setup-sw-env'/> |
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 that since this application are now part of the add-on, the sections Setup Software Environment and Setup Firmware Project should be reworked to match the add-on documentation.
| CONFIG_BT_SMP=y | ||
| CONFIG_BT_L2CAP_TX_BUF_COUNT=5 | ||
| CONFIG_BT_PERIPHERAL=y | ||
| CONFIG_BT_DEVICE_NAME="Neuton NRF RemoteControl" |
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.
change CONFIG_BT_DEVICE_NAME something like "nRF Edge AI Remote Control", or please consult with @wbober for proper name
| @@ -0,0 +1,16 @@ | |||
| sample: | |||
| description: Hello World sample, the simplest Zephyr | |||
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 match the provided application description :) I believe now it is template for hello world
| @@ -0,0 +1,39 @@ | |||
| /* 2023-06-09T11:22:25Z */ | |||
|
|
|||
| /* ---------------------------------------------------------------------- | |||
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.
please change this copyright according add-on license and file tag format
My point of view:
|
Thanks for your input. You are right about issue that different hardware might not work correctly with model trained on data acquired from thingy53. Does it mean that we should provide for all other DKs simulated data? As an alternative I see would be having a different model for each hardware shape (I guess there is no much difference between DKs) I'am trying to make app working on nrf54h20 with shield. I guess that when I will mange to do that we will see how big the issue is and we might then decide how to proceed. |
| #include <zephyr/bluetooth/uuid.h> | ||
| #include <zephyr/settings/settings.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.
This style of sectioning is not used in nrf
|
|
||
| static void input_ccc_changed(const struct bt_gatt_attr* attr, uint16_t value) | ||
| { | ||
| printk("Input CCCD %s\n", value == BT_GATT_CCC_NOTIFY ? "enabled" : "disabled"); |
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.
Use LOG subsys
| #define KEY_MEDIA_PREV_TRACK ( 1 << 4 ) | ||
| #define KEY_MEDIA_NEXT_TRACK ( 1 << 5 ) | ||
|
|
||
| #if CONFIG_SAMPLE_BT_USE_AUTHENTICATION |
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 CONFIG is not present in this sample
| #define KEY_ARROW_LEFT (0x50) | ||
| #define KEY_ARROW_RIGHT (0x4F) | ||
| #define KEY_F5 (0x3E) | ||
| #define KEY_ESP (0x29) |
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.
Is this typo? Should it be ESC?
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.
Do we need BSP? We target only Nordic chips
| [CLASS_LABEL_ROTATION_LEFT] = "ROTATION LEFT" | ||
| }; | ||
|
|
||
| static const uint8_t LABELS_CNT = sizeof(LABEL_VS_NAME) / sizeof(LABEL_VS_NAME[0]); |
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.
ARRAY_SIZE, same below
|
|
||
| ////////////////////////////////////////////////////////////////////////////// | ||
|
|
||
| static const char* get_name_by_target_(uint8_t predicted_target) |
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 this functions can be placed where they are declared, no need to split declaration from definition.
| @@ -0,0 +1,22 @@ | |||
| # | |||
| # Copyright (c) 2024 Nordic Semiconductor | |||
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.
correct year in every file
| cmake_minimum_required(VERSION 3.20.0) | ||
|
|
||
| find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE}) | ||
| project(nrf_edgeai_thingy53) |
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.
gesture_recognition
| @@ -0,0 +1,17 @@ | |||
| // To get started, press Ctrl+Space to bring up the completion menu and view the available nodes. | |||
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 comment are not needed, file should be in boards dir
applications: gesture_recognition: Add gesture_recognition application
Add gesture_recognition application as it is in:
https://github.com/Neuton-tinyML/neuton-nordic-thingy53-ble-remotecontrol
on commit: b2063f2ae42fa8aead6b3c7df477b3cda7caac29
Application was devloped by Raman Rusak.