Add CSI Framework and CSI driver #6399

sahil1dotgupta · 2024-09-16T07:15:17Z

Add CSI Framework

framework/serc/csifw: CSIFW source code
framework/include/csifw: CSIFW headers
apps/examples/csifw_test: Sample csifw app.

Add WIFI CSI Driver

os/drivers/wifi_csi: Add driver source code
os/board: Enable driver register at bootup
build/configs: Enable driver flags

vibhor-m · 2024-09-16T11:15:50Z

build/configs/rtl8730e/loadable_ext_psram/defconfig

@@ -1650,6 +1669,9 @@ CONFIG_EXAMPLE_LCD_FPS_TEST=5000
 #
 # CONFIG_EXAMPLES_TAHI is not set
 # CONFIG_EXAMPLES_WIFIMANAGER_TEST is not set
+# CONFIG_WIFIMANAGER_TEST_TRIAL=5


'# CONFIG_WIFIMANAGER_TEST_TRIAL is not set
'# CONFIG_EXAMPLES_WIFIMANAGER_AP_LIST_ITEMS_COUNT is not set

vibhor-m · 2024-09-16T11:16:07Z

build/configs/rtl8730e/loadable_ext_psram/defconfig

@@ -1274,6 +1289,9 @@ CONFIG_DEBUG_ERROR=y
 # Subsystem Debug Options
 #
 # CONFIG_DEBUG_AUDIO is not set
+CONFIG_DEBUG_WIFICSI=y
+CONFIG_DEBUG_WIFICSI_ERROR=y
+CONFIG_DEBUG_WIFICSI_INFO=y


Default is Error

sunghan-chang

There are so many coding rules violations.
UDP server support to send CSI data to UDP client is not proper in commit rules.

framework/src/csifw/CSILogsDumper.c

sunghan-chang · 2024-09-19T11:27:34Z

framework/src/csifw/CSILogsDumper.c

+{
+	print_log("\n RAW DATA %d\n\n", len);
+	unsigned long long *buff_tmp = (u64 *)buf;
+	int buff_len = (len/8) + 1;


sunghan-chang · 2024-09-19T11:27:47Z

framework/src/csifw/CSILogsDumper.c

+
+void set_event_listener(csiDataDumpListener listener)
+{
+    gListener = listener;


coding rule violation.
tab for indentation
same as other lines

sunghan-chang · 2024-09-19T11:28:51Z

framework/src/csifw/CSILogsDumper.c

+    }
+}
+
+CSIFW_RES csi_logs_dumper_init()


should add void

sunghan-chang · 2024-09-19T11:29:17Z

tools/csi_datadump/csi_datadump_udp_client.c

+    fclose(file);
+    close(sockfd);
+    return 0;
+}


need to check the warning

sunghan-chang · 2024-09-19T11:31:28Z

framework/src/csifw/CSILogsDumper.c

+        return CSIFW_ERROR;
+    }
+
+    if (pthread_create(&gThreadId, NULL, dataTransmitter, NULL) != 0) {


When you create a pthread, you should call pthread_join or pthread_detach. If you don't, it will have a memory leak.

sunghan-chang · 2024-09-19T11:33:40Z

build/configs/rtl8730e/loadable_ext_psram/defconfig

@@ -1031,6 +1031,12 @@ CONFIG_LWNL80211=y
 # CONFIG_DEBUG_LWNL80211_VENDOR_DRV_ERROR is not set
 CONFIG_NET_LOOPBACK_INTERFACE=y

+# WIFI CSI Support


DO NOT modify defconfig manually.
After doing through menuconfig, and then copy changes into the defconfig.

sunghan-chang · 2024-09-19T11:34:07Z

os/Kconfig.debug

@@ -45,6 +45,38 @@ config DEBUG_VERBOSE

 comment "Subsystem Debug Options"

+config DEBUG_WIFICSI


Let's add it by Alphabet order to find it easily.

sunghan-chang · 2024-09-19T11:35:18Z

os/board/rtl8730e/src/rtl8730e_rtk_csi.c

+	int ret;
+
+	csivdbg("minor %d\n", minor);
+	printf("minor %d\n", minor);


duplicated, this line should be removed.

sunghan-chang · 2024-09-19T11:37:10Z

os/drivers/Kconfig

+
+if !WIFICSI_DEV_ROOT
+
+config WIFICSI_DEV_PATH


What is difference between WIFICSI_CUSTOM_DEV_PATH and this?

Obsolete, it has been removed now.

sunghan-chang · 2024-09-19T11:38:14Z

os/drivers/wifi_csi/Make.defs

+ifeq ($(CONFIG_WIFI_CSI_RTL8730E),y)
+CSRCS += rtk_wifi_csi.c
+endif


The driver should be common. This should be in Make.defs of RTL8730E, not here.

sunghan-chang · 2024-09-19T11:40:53Z

os/include/tinyara/wifi_csi/wifi_csi_common.h

+ * language governing permissions and limitations under the License.
+ *
+ ****************************************************************************/
+#ifndef __INCLUDE_TINYARA_WIFI_CSI_COMMON_H


Are you able to distinguish wifi_csi.h and wifi_csi_common.h in usage? How about renaming them to know what it is from the name?

sunghan-chang

Many basic things are missing like license, coding rules, commit rules.

sunghan-chang · 2024-09-19T11:42:09Z

apps/examples/csifw_test/Kconfig

+# see kconfig-language at https://www.kernel.org/doc/Documentation/kbuild/kconfig-language.txt
+#
+
+config EXAMPLES_CSIFW_TEST


This is not a framework. How about splitting a commit for app from framework commit?

sunghan-chang · 2024-09-19T11:43:02Z

apps/examples/csifw_test/csifw_test.c

+static int print_log(const char *format, ...)
+{
+
+	va_list args;
+	va_start(args, format);
+
+	vprintf(format, args);
+	va_end(args);
+	return 1;
+}


Could you let me know why you use this function instead of printf? Any advantage???

Changed to printf.

sunghan-chang · 2024-09-19T11:46:08Z

framework/src/csifw/Make.defs

+
+ifeq ($(CONFIG_CSIFW), y)
+
+CFLAGS += -I${TOPDIR}/os/include


should be unnecessary. Let's remove this.

Taejun-Kwon · 2024-09-24T11:45:30Z

os/drivers/wifi_csi/wifi_csi.c

+	if (upper->crefs > 1) {
+		upper->crefs--;
+	} else {
+		FAR struct wifi_csi_lowerhalf_s *lower = upper->dev;


unnecessary line

Right, corrected.

Taejun-Kwon · 2024-09-24T11:50:01Z

os/drivers/wifi_csi/wifi_csi.c

+			ret = lower->ops->getcsidata(lower, buf_arg->buffer, buf_arg->buflen);
+		} else {
+			csidbg("ERROR: Unsupported operation: getcsidata\n");
+			ret = ENOTSUP;


I think ENOSYS is better option.
If driver has not implemented function, then ENOSYS
But driver dosn't support by given configuration or something, then ENOTSUP.

Ohh right! thanks.

Taejun-Kwon · 2024-09-24T11:51:44Z

os/drivers/wifi_csi/wifi_csi.c

+ *   call via its callback pointer to our upper-half driver.
+ *
+ ****************************************************************************/
+static inline void wifi_csi_data_ready(FAR struct wifi_csi_upperhalf_s *upper, char *buff, uint32_t csi_data_len)


Type need to be changed int, and it should return some of errno if upper->usermq is null

wifi_csi_data_ready is a callback, so where would I check the return type, all callers are themselves void callbacks. (cannot return anything as no one is there to check ret value)

wifi driver sends data ready callback --> void lower_driver_data_readyCB() --> void wifi_csi_callback() --> void wifi_csi_data_ready()

Taejun-Kwon · 2024-09-24T11:54:16Z

framework/include/csifw/csifw_api.h

+ * 
+ * @param[in] raw_callback Callback function for receiving raw CSI data. Pass NULL if raw csi data not required.
+ * @param[in] parsed_callback Callback function for receiving parsed CSI data. Pass NULL if parsed csi data not required.
+ * @param[in] interval Data collection interval in microseconds.


please add description for void *ptr

Taejun-Kwon · 2024-09-24T11:55:59Z

framework/src/csifw/CSIService.c

+{	
+	if (g_service_state == CSI_COLLECT_STATE_STARTED) {
+		CSIFW_LOGE(" Already running");
+		return CSIFW_ERROR;


you should provide some kind of CSIFW_ERROR_WRONG_STATE or something

State is an internally maintained f/w property, so I followed the practice to not inform the app about f/w state.
In current case, the app developer will receive error and message that "CSIFW is already started".

But if still the change is needed, please tell, I'll correct it.

Taejun-Kwon · 2024-09-24T11:56:50Z

framework/src/csifw/CSIService.c

+	}
+	csi_packet_receiver_start_collect(&g_csifw_config);
+	ping_generator_start();
+	g_service_state = CSI_COLLECT_STATE_STARTED;


so it working based on state, then some kind of pthread_mutex_lock is required. to prevent change state in multiple thread environment

Taejun-Kwon · 2024-09-24T11:58:23Z

framework/src/csifw/CSIService.c

+CSIFW_RES csi_service_stop(CSIFW_REASON reason)
+{
+	if (g_service_state != CSI_COLLECT_STATE_STARTED) {
+		CSIFW_LOGE("Already stopped");


CSIFW_LOGE("stop error state : %d\n", g_service_state);
because it can be one of 4 states.

Taejun-Kwon · 2024-09-24T12:11:25Z

apps/examples/csifw_test/csifw_test.c

+	sleep(5);
+
+	PRNT_LN3;
+	csi_action_param_t config = {0};


I think most of app developer can't understand what this config means exactly.
so please refer to below.

some_csi_init_in_framework() { /* Initialize Base value Here */ g_config.group_num = CSI_GROUP_NUM_1; g_config.mode = CSI_MODE_NORMAL; g_config.accuracy = CSI_ACCU_1BYTE; g_config.ch_opt = CSI_CH_NON_LEGACY; g_config.trig_period = 200; g_config.data_rate = CSI_DATA_RATE_HIGH...; //0x80 is impossible to understand what it means. } csi_set_group_num(int num) { if (state_is_wrong) { return CSIFW_ERROR_STATE; } g_config.group_num = num; }

A better way is that structure in driver need to be avoided, so it should work without csi_service_set_config based on basic setting value.

Actually for the application to get data, they must configure what specific kind of csi data is required.

with some_csi_init_in_framework() do you wish to provide option to use default config for an app-developer who does not have info about config? And then provide apis (like csi_set_group_num() )for every config component, so that they can only change the config they are known to?

Taejun-Kwon · 2024-09-24T12:15:46Z

framework/src/csifw/CSILogsDumper.c

+	printf("\n RAW DATA %d\n\n", len);
+	unsigned long long *buff_tmp = (u64 *)buf;
+	int buff_len = (len / 8) + 1;
+	for (int i = 0; i < buff_len; i++) 


according to our coding rule, add bracket even if it is single line of code.

It is fixed

sunghan-chang

There are still many coding rules violations.

sunghan-chang · 2024-10-02T06:48:14Z

apps/examples/csifw_test/Kconfig_ENTRY

+config ENTRY_CSIFW_TEST
+	bool "CSI Framework example"
+	depends on EXAMPLES_CSIFW_TEST


To use this, you should add config USER_ENTRYPOINT in the Kconfig. Please refer other example.

sunghan-chang · 2024-10-02T06:49:14Z

apps/examples/csifw_test/csifw_test.c

+	return 0;
+}
+
+void csi_manager_csifw_test_init(void)


Could you let me know who executes this?

sunghan-chang · 2024-10-02T06:50:16Z

apps/examples/csifw_test/csifw_test.c

+	while (1) {
+		sleep(2);
+	}
+	// sleep(15);


Do you need to keep commented codes?

sunghan-chang · 2024-10-02T06:51:07Z

apps/examples/csifw_test/csifw_test.c

+	PRNT_LN1;
+	PRNT_LN2;


Could you let me know purpose of them, PRNT_LN1, LN2 and LN3?

sunghan-chang · 2024-10-02T06:51:59Z

apps/examples/csifw_test/csifw_test.c

+
+	#if LOG_PARSED_DATA
+	printf("[APP] Displaying parsed data, csi_data_len: %d\n\n", csi_data_len);
+	for(int i = 0; i < csi_data_len; i += 2) {


sunghan-chang · 2024-10-02T09:38:35Z

framework/src/csifw/CSIParser.c

+		CSIFW_LOGI("PRINTING PARSED DATA"); //csi_sequence, timestamp
+		printf("\n csi_buff_len (with header): %d, only csi len: 112",csi_buff_len);


When do you use CSIFW_LOGx or printf?

sunghan-chang · 2024-10-02T09:44:57Z

framework/src/csifw/CSIParser.c

+		CSIFW_LOGI("%d,%d,", csi_seq_num, timestamp); //csi_sequence, timestamp
+
+		printf("0x%x%x,", csi_buff[1], csi_buff[0]); //csi_signature
+		printf("0x%x,", csi_buff[2]); //hdr_len
+		CSIFW_LOGI("%x:%x:%x:%x:%x:%x,", csi_buff[8],csi_buff[7],csi_buff[6],csi_buff[5],csi_buff[4], csi_buff[3]); //mac_addr
+		CSIFW_LOGI("%x:%x:%x:%x:%x:%x,", csi_buff[14],csi_buff[13],csi_buff[12],csi_buff[11],csi_buff[10], csi_buff[9]); //trig_mac_addr
+		CSIFW_LOGI("%d,", ((int)csi_buff[31]/2 - 110)); //rssi
+
+		// CSIFW_LOGI("%s,",argv[1]); //real
+		CSIFW_LOGI("%d,", (int)csi_buff[19]); //channel, infered


I don't think we are ok to print all raw data defaultly. I don't mean this highlighted lines only. There are many data printing in the framework.

sunghan-chang · 2024-10-02T09:46:27Z

framework/src/csifw/CSIService.c

+CSIFW_RES csi_manager_init(void)
+{
+	CSIFW_LOGI("[CSIFW] initialization function is called\n");
+	return CSIFW_OK;
+}


This function looks unnecessary. Could you check it?

sunghan-chang · 2024-10-02T09:49:23Z

framework/src/csifw/CSIService.c

+static void csi_data_dump_listener(EVENT event) {
+	CSIFW_LOGD("csi_data_dump_listener called\n");
+	gCSIDataDumpEvent = event;
+	return;


unnecessary

sunghan-chang · 2024-10-02T09:52:50Z

os/board/rtl8730e/src/component/os/tizenrt/rtk_wifi_csi.c

+	if (!dev) {
+		csidbg("ERROR: dev null\n");
+		return;
+	}
+	if (!priv) {
+		csidbg("ERROR: priv null\n");
+		return;
+	}


Is there any case which dev is NOT null and priv is null? I mean that checking priv is necessary?

sahil1dotgupta · 2024-10-10T04:49:51Z

Hello all, thankyou for the suggestions, the pending comments will be resolved soon along with sync-up with internal developments.

framework/src/csifw: CSIFW source code framework/include/csifw: CSIFW headers os/drivers/wifi_csi: Add driver source code os/board: Enable driver register at bootup os/include/tinyara/wifi_csi/wifi_csi_struct.h: Define commmon wifi csi kernel constants and structures.

sahil1dotgupta marked this pull request as ready for review September 16, 2024 09:59

vibhor-m reviewed Sep 16, 2024

View reviewed changes

manish1-garg force-pushed the addcsifw branch from 6db8ae7 to 7052d42 Compare September 17, 2024 07:43

sahil1dotgupta mentioned this pull request Sep 17, 2024

Changes in csifw_test app and data-format in saved file sahil1dotgupta/TizenRT#2

Closed

sahil1dotgupta force-pushed the addcsifw branch from 7052d42 to 67c2816 Compare September 17, 2024 11:48

manish1-garg force-pushed the addcsifw branch from 67c2816 to 7052d42 Compare September 17, 2024 12:22

sunghan-chang requested changes Sep 19, 2024

View reviewed changes

sunghan-chang reviewed Sep 19, 2024

View reviewed changes

sunghan-chang requested changes Sep 19, 2024

View reviewed changes

sahil1dotgupta force-pushed the addcsifw branch 6 times, most recently from a6cb430 to ebeda22 Compare September 24, 2024 10:38

manish1-garg force-pushed the addcsifw branch from ebeda22 to f012e25 Compare September 24, 2024 10:58

sahil1dotgupta force-pushed the addcsifw branch 4 times, most recently from c48276c to b87ebb7 Compare September 24, 2024 11:42

Taejun-Kwon reviewed Sep 24, 2024

View reviewed changes

manish1-garg force-pushed the addcsifw branch from b87ebb7 to b58eb64 Compare September 25, 2024 12:43

sahil1dotgupta force-pushed the addcsifw branch from b58eb64 to ec9f44c Compare September 25, 2024 14:01

manish1-garg force-pushed the addcsifw branch from ec9f44c to 1d7c6a2 Compare September 25, 2024 14:26

sahil1dotgupta force-pushed the addcsifw branch 2 times, most recently from 102406d to 6d1c3b4 Compare September 30, 2024 06:50

sunghan-chang reviewed Oct 2, 2024

View reviewed changes

sahil1dotgupta force-pushed the addcsifw branch 2 times, most recently from 4f0b886 to 469d706 Compare November 27, 2024 11:37

		@@ -45,6 +45,38 @@ config DEBUG_VERBOSE

		comment "Subsystem Debug Options"

		config DEBUG_WIFICSI

		CSIFW_LOGI("PRINTING PARSED DATA"); //csi_sequence, timestamp
		printf("\n csi_buff_len (with header): %d, only csi len: 112",csi_buff_len);


		ifeq ($(CONFIG_CSIFW), y)

		CFLAGS += -I${TOPDIR}/os/include

Add CSI Framework and CSI driver #6399

Are you sure you want to change the base?

Add CSI Framework and CSI driver #6399

Conversation

sahil1dotgupta commented Sep 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunghan-chang 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

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

sunghan-chang 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahil1dotgupta Sep 25, 2024 • 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

sahil1dotgupta Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunghan-chang 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

sahil1dotgupta commented Oct 10, 2024 • edited Loading

sahil1dotgupta Sep 25, 2024 •

edited

Loading

sahil1dotgupta Sep 25, 2024 •

edited

Loading

sahil1dotgupta commented Oct 10, 2024 •

edited

Loading