Add the initial version of Wi-Fi Manager #528

davidfather · 2017-09-05T06:56:08Z

Wi-Fi Manager is introduced to provide easy APIs for applications to easily manipulate Wi-Fi features

sunghan-chang

There are too many violations of coding rule.

sunghan-chang · 2017-09-05T07:07:03Z

framework/Makefile

@@ -42,6 +42,11 @@ ifeq ($(CONFIG_NETUTILS_MQTT), y)
 include src$(DELIM)mqtt$(DELIM)Make.defs
 endif

+ifeq ($(CONFIG_WIFI_MANAGER), y)
+#CFLAGS += -I$(TOPDIR)$(DELIM)..$(DELIM)framework$(DELIM)src$(DELIM)wifi_manager


Let's remove unnecessary codes.

sunghan-chang · 2017-09-05T07:07:19Z

framework/include/wifi_manager/wifi_manager.h

@@ -0,0 +1,129 @@
+/****************************************************************************
+ *
+ * Copyright 2016 Samsung Electronics All Rights Reserved.


Year of copyright should be 2017.

sunghan-chang · 2017-09-05T07:08:30Z

framework/include/wifi_manager/wifi_manager.h

+	// SOFT AP mode status
+	CLIENT_CONNECTED,
+	CLIENT_DISCONNECTED
+} connect_status;


How about add _e postfix to know easily what this is an enum structure?

connect_status_e

sunghan-chang · 2017-09-05T07:09:24Z

framework/include/wifi_manager/wifi_manager.h

+    void (*sta_disconnected) (void);
+    void (*softap_sta_join) (void);
+    void (*softap_sta_leave) (void);
+} wifi_manager_cb;


same as previous.

wifi_manager_cb_s

sunghan-chang · 2017-09-05T07:10:06Z

framework/include/wifi_manager/wifi_manager.h

+typedef struct {
+	char ip4_address[18];
+	char ssid[32];
+    int rssi;


Here are violations of coding rule. Please use tab as an indention.

I did apply fomatter.sh

sunghan-chang · 2017-09-05T07:21:22Z

framework/src/wifi_manager/wifi_mutex.c

+
+wifi_utils_result wifi_mutex_create(wifi_mutex* mutex)
+{
+	return wifi_semaphore_create((wifi_semaphore *) mutex, 1);


Let' remove space after type casting.

(wifi_semaphore *)mutex

I did go through fomatter.sh

sunghan-chang · 2017-09-05T07:22:24Z

framework/src/wifi_manager/wifi_mutex.c

+#include "wifi_mutex.h"
+#include "wifi_semaphore.h"
+
+wifi_utils_result wifi_mutex_create(wifi_mutex* mutex)


(wifi_mutex *mutex)

I did go through fomatter.sh

sunghan-chang · 2017-09-05T07:24:06Z

framework/src/wifi_manager/wifi_mutex.h

+ * @return WIFI_UTILS_FAIL          :  fail
+ * @return WIFI_UTILS_INVALID_ARGS  :  input parameter invalid
+ */
+wifi_utils_result wifi_mutex_create( wifi_mutex* mutex );


xxx yyy(wifi_mutex *mutex);

Need same at belows

I did go through fomatter.sh

sunghan-chang · 2017-09-05T07:25:39Z

framework/src/wifi_manager/wifi_net.c

+#include "wifi_net.h"
+
+#define ptr2uint32(ptr) (uint32_t)ptr[3] |     \
+                ((uint32_t)ptr[2] << 8)  |     \


use tabs intead of spaces as an indentation

I did go through fomatter.sh

sunghan-chang · 2017-09-05T07:30:46Z

framework/include/wifi_manager/wifi_manager.h

+} wifi_manager_ap_config;
+
+
+wifi_manager_result wifi_manager_init(wifi_manager_cb *wmcb);


Even though this file shows public APIs, you didn't add comments for doxygen. But you added doxygen comments at local header file. Please check it.
And when you add doxygen comments at public APIs, since option should be there at end of comments.

* @since Tizen RT v1.1

Added api description

Wi-Fi Manager is introduced to provide easy APIs for applications to easily manipulate Wi-Fi features

sunghan-chang · 2017-09-06T05:04:41Z

framework/src/wifi_manager/Make.defs

@@ -0,0 +1,23 @@
+###########################################################################
+#
+# Copyright 2016 Samsung Electronics All Rights Reserved.


2016 -> 2017

sunghan-chang · 2017-09-06T05:17:24Z

os/net/Kconfig

@@ -133,6 +133,20 @@ endif #NET_SECURITY_TLS

 endmenu #Protocols

+menu "Wireless"
+
+config WIFI_MANAGER


This is not under network folder so that let's make a new Kconfig at wifi_manager and include it here.

wifi_manager/Kconfig is created

sunghan-chang · 2017-09-06T05:18:53Z

apps/system/utils/netcmd_dhcpd.c

@@ -148,7 +148,7 @@ int cmd_dhcpd(int argc, char *argv[])
 			}
 		}

-		if (dhcpd_start(argv[2]) != 0) {
+		if (dhcpd_start(argv[2], NULL) != 0) {


Is there any releation between WiFi Manager and DHCPD stubs?

Yes, wifi manager needs to be informed of join of a new station in softap mode.

sunghan-chang · 2017-09-06T05:19:36Z

external/dhcpd/dhcpd.c


-		ret = select(sockfd+1, &sockfd_set, NULL, NULL, &g_select_timeout);
-		if ((ret > 0) && FD_ISSET(sockfd, &sockfd_set)) {
+		ret = select(g_dhcpd_sockfd+1, &sockfd_set, NULL, NULL, &g_select_timeout);


Please add spaces around +.

g_dhcpd_sockfd + 1

I go through fomatter.

sunghan-chang · 2017-09-06T05:23:43Z

external/dhcpd/dhcpd.c

-			}
+	/* Create a socket to listen for requests from DHCP clients */
+	/* TODO : Need to add cancellation point */
+	if (g_dhcpd_sockfd < 0) {


You always put -1 to g_dhcpd_sockfd at line 1552. This if condition is redundant.

I think so. if statement is removed.

sunghan-chang · 2017-09-06T06:32:09Z

framework/src/wifi_manager/wifi_net.h

+ *
+ ****************************************************************************/
+
+#pragma once


#pragma once is not a standard so that some compiler can not understand this command.
Let's change this like belows.

#ifndef XXX #define ... #endif

sunghan-chang · 2017-09-06T06:33:56Z

framework/src/wifi_manager/wifi_semaphore.c

+		return WIFI_UTILS_INVALID_ARGS;
+	}
+
+	if (sem_destroy((sem_t *) semaphore)) {


(sem_t *) semaphore => (sem_t *)semaphore

formatter doesn't fix this problem. I mannually fix it.

sunghan-chang · 2017-09-06T06:37:07Z

framework/src/wifi_manager/wifi_utils.c

+		}
+	}
+
+	ret = WiFiNetworkJoin((uint8_t *) ap_connect_config->ssid, ap_connect_config->ssid_length, NULL, config);


remove space

sunghan-chang · 2017-09-06T06:39:21Z

framework/src/wifi_manager/wifi_utils.c

+		config = (slsi_security_config_t *)zalloc(sizeof(slsi_security_config_t));
+		if (!config) {
+			ndbg("Memory allocation failed!\n");
+			goto connect_ap_fail;


In this case, we don't need to check config so that let's change this to "return result;"

from the viewpoint of readability, this statement seems to be fine.

sunghan-chang · 2017-09-06T06:43:08Z

framework/src/wifi_manager/wifi_utils.h

+	WIFI_UTILS_AUTH_WPA2_PSK,				 /**<  WPA2_PSK mode                  */
+	WIFI_UTILS_AUTH_WPA_AND_WPA2_PSK,		 /**<  WPA_PSK and WPA_PSK mixed mode */
+	WIFI_UTILS_AUTH_UNKNOWN,				 /**<  unknown type                   */
+} wifi_utils_ap_auth_type;


how about _e

jeongchanKim · 2017-09-06T06:50:33Z

framework/include/wifi_manager/wifi_manager.h

+} wifi_manager_ap_config_s;
+
+/**
+* @brief Initialize Wi-Fi Manager including starting Wi-Fi interface.


Our public APIs should be in group.
If those APIs need new group, you should do "@defgroup " , and include all APIs to that group with "@InGroup"

You can refer to below
/**

@defgroup DHCP DHCP

@InGroup NETWORK

@{
*/

jeongchanKim · 2017-09-06T06:51:43Z

framework/include/wifi_manager/wifi_manager.h

+wifi_manager_result_e wifi_manager_init(wifi_manager_cb_s *wmcb);
+
+/**
+* @brief Deinitialize Wi-Fi Manager including stoping Wi-Fi interface.


check the indent for doxygen comment(space is needed before *)

jeongchanKim · 2017-09-06T06:53:16Z

framework/src/wifi_manager/wifi_mutex.h

+
+/**
+ @}
+	 */// end mutex group


remove unnecessary tab

jeongchanKim · 2017-09-06T06:53:48Z

framework/src/wifi_manager/wifi_net.h

+
+/**
+ @}
+	 */// end net work utility group


check the indent

jeongchanKim · 2017-09-06T06:54:18Z

framework/src/wifi_manager/wifi_semaphore.c

+		return WIFI_UTILS_INVALID_ARGS;
+	}
+
+	if (sem_post((sem_t *) semaphore)) {


remove space after type casting

jeongchanKim · 2017-09-06T06:56:05Z

framework/src/wifi_manager/wifi_semaphore.h

+ */
+
+/**
+ * @defgroup semaphore semaphore


Those APIs are related with wifi semaphore.
but if you define this group to "semaphore", then category name in doxygen is set to semaphore.
that makes some confusion to users. isn't it?

This is not public APIs. Anyway, change to Wi-Fi Manager group.

jeongchanKim · 2017-09-06T06:56:17Z

framework/src/wifi_manager/wifi_semaphore.h

+
+/**
+ @}
+	 */// end semaphore group


check the indent

jeongchanKim · 2017-09-06T10:40:34Z

framework/include/wifi_manager/wifi_manager.h

+ * @since Tizen RT v1.1
+ */
+wifi_manager_result_e wifi_manager_set_softap_config(wifi_manager_softap_config_s *config);
+


you opened the "Wi-Fi_Manager ingroup" brace, but not close.
please add below.
/**

@}
*/

you didn't close the }. please check this

jeongchanKim · 2017-09-06T10:43:23Z

framework/src/wifi_manager/wifi_semaphore.h

+ */
+
+/**
+ * @defgroup Wi-Fi_Manager Wi-Fi_Manager


you already made defgroup Wi-Fi_Manager in wifi_manager.h
if you want to add below APIs to that group, you can just do ingroup, not making again.

this file doesn't include public apis so that related comments are removed.

sunghan-chang · 2017-09-06T10:41:59Z

external/dhcpd/dhcpd.c

@@ -610,15 +622,15 @@ static inline bool dhcpd_parseoptions(void)
 			optlen = ptr[DHCPD_OPTION_LENGTH] + 2;
 			if (optlen >= 4 && optlen < remaining) {
 				memcpy(&tmp, &ptr[DHCPD_OPTION_DATA], 4);
-				g_state.ds_optreqip = (in_addr_t)ntohl(tmp);
+				g_state.ds_optreqip = (in_addr_t) ntohl(tmp);


Let's revert this line change.

sunghan-chang · 2017-09-06T10:42:08Z

external/dhcpd/dhcpd.c

 			}
 			break;

 		case DHCP_OPTION_LEASE_TIME:	/* IP address lease time */
 			optlen = ptr[DHCPD_OPTION_LENGTH] + 2;
 			if (optlen >= 4 && optlen < remaining) {
 				memcpy(&tmp, &ptr[DHCPD_OPTION_DATA], 4);
-				g_state.ds_optleasetime = (time_t)ntohl(tmp);
+				g_state.ds_optleasetime = (time_t) ntohl(tmp);


Let's revert this line change.

sunghan-chang · 2017-09-06T10:42:14Z

external/dhcpd/dhcpd.c

@@ -633,7 +645,7 @@ static inline bool dhcpd_parseoptions(void)
 			optlen = ptr[DHCPD_OPTION_LENGTH] + 2;
 			if (optlen >= 4 && optlen < remaining) {
 				memcpy(&tmp, &ptr[DHCPD_OPTION_DATA], 4);
-				g_state.ds_optserverip = (in_addr_t)ntohl(tmp);
+				g_state.ds_optserverip = (in_addr_t) ntohl(tmp);


Let's revert this line change.

sunghan-chang · 2017-09-06T10:42:23Z

external/dhcpd/dhcpd.c

@@ -942,38 +917,35 @@ static int dhcpd_sendpacket(int bbroadcast)
 	if (bbroadcast) {
 		ipaddr = INADDR_BROADCAST;
 	} else if (memcmp(g_state.ds_outpacket.ciaddr, g_anyipaddr, 4) != 0) {
-		dhcpd_arpupdate((uint16_t *)g_state.ds_outpacket.ciaddr, g_state.ds_outpacket.chaddr);
+		dhcpd_arpupdate((uint16_t *) g_state.ds_outpacket.ciaddr, g_state.ds_outpacket.chaddr);


Let's revert this line change.

sunghan-chang · 2017-09-06T10:42:29Z

external/dhcpd/dhcpd.c

 		memcpy(&ipaddr, g_state.ds_outpacket.ciaddr, 4);
 	} else if (g_state.ds_outpacket.flags & HTONS(BOOTP_BROADCAST)) {
 		ipaddr = INADDR_BROADCAST;
 	} else {
-		dhcpd_arpupdate((uint16_t *)g_state.ds_outpacket.yiaddr, g_state.ds_outpacket.chaddr);
+		dhcpd_arpupdate((uint16_t *) g_state.ds_outpacket.yiaddr, g_state.ds_outpacket.chaddr);


Let's revert this line change.

sunghan-chang · 2017-09-06T10:43:39Z

external/dhcpd/dhcpd.c

+		g_dhcp_sta_joined = dhcp_join_cb;
+	}
+
+	status = pthread_create(&g_tid, &attr, (pthread_startroutine_t) dhcpd_run, NULL);


Let's remove space after type casting.

(pthread_startroutine_t) dhcpd_run => (pthread_startroutine_t)dhcpd_run

sunghan-chang · 2017-09-06T10:47:05Z

framework/src/wifi_manager/wifi_manager.c

+	}
+
+	if (!g_wifi_softap_config) {
+		g_wifi_softap_config = (wifi_manager_softap_config_s *)malloc(sizeof(wifi_manager_softap_config_s));


I can't find codes to free g_wifi_softap_config. Only allocation is here. Is it intended?

Very good comment!
Actually this api is not required if softap configuration information is delivered when wifi_manager_set_mode() is called.

jeongchanKim · 2017-09-06T10:56:57Z

external/include/protocols/dhcpd.h

@@ -88,6 +88,8 @@ extern "C" {
 #define EXTERN extern
 #endif

+typedef void (*dhcp_sta_joined) (void);


remove the space between (*dhcp_sta_joined) and (void);

jeongchanKim · 2017-09-06T10:58:18Z

framework/src/wifi_manager/wifi_manager.c

+
+static wifi_utils_result_e stop_dhcp_server(void)
+{
+	struct in_addr in = {.s_addr = INADDR_NONE };


space is needed after '{'

jeongchanKim · 2017-09-06T11:00:15Z

framework/src/wifi_manager/wifi_mutex.h

+
+/**
+ @}
+	 */// end mutex group


remove unnecessary tab

this file doesn't include public apis so that related comments are removed.

jeongchanKim · 2017-09-06T11:00:31Z

framework/src/wifi_manager/wifi_mutex.h

+wifi_utils_result_e wifi_mutex_destroy(wifi_mutex *mutex);
+
+/**
+ @}


add '*' infront of @

this file doesn't include public apis so that related comments are removed.

davidfather requested review from pillip8282, sangwon03 and sunghan-chang September 5, 2017 06:56

sunghan-chang requested changes Sep 5, 2017

View reviewed changes

Add the initial version of Wi-Fi Manager

cd3eb9a

Wi-Fi Manager is introduced to provide easy APIs for applications to easily manipulate Wi-Fi features

davidfather force-pushed the wifi_manager branch from 322c82c to c3bbc4d Compare September 6, 2017 04:55

sunghan-chang reviewed Sep 6, 2017

View reviewed changes

jeongchanKim reviewed Sep 6, 2017

View reviewed changes

davidfather force-pushed the wifi_manager branch 2 times, most recently from 6044413 to deb69fb Compare September 6, 2017 08:52

jeongchanKim reviewed Sep 6, 2017

View reviewed changes

sunghan-chang reviewed Sep 6, 2017

View reviewed changes

jeongchanKim reviewed Sep 6, 2017

View reviewed changes

davidfather force-pushed the wifi_manager branch 2 times, most recently from 28b0101 to cd3eb9a Compare September 7, 2017 01:56

sunghan-chang approved these changes Sep 7, 2017

View reviewed changes

sunghan-chang merged commit db3685b into Samsung:master Sep 7, 2017

		} wifi_manager_ap_config;


		wifi_manager_result wifi_manager_init(wifi_manager_cb *wmcb);

Add the initial version of Wi-Fi Manager #528

Add the initial version of Wi-Fi Manager #528

Conversation

davidfather commented Sep 5, 2017

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

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

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

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