donghengqaz commented on a change in pull request #1893: URL: https://github.com/apache/incubator-nuttx/pull/1893#discussion_r494716868
########## File path: arch/xtensa/src/esp32/Make.defs ########## @@ -136,3 +136,36 @@ ifeq ($(CONFIG_ARCH_USE_MODULE_TEXT),y) CHIP_CSRCS += esp32_modtext.c CMN_ASRCS += xtensa_loadstore.S endif + +ifeq ($(CONFIG_ESP32_WIRELESS),y) +WIRELESS_DRV_UNPACK = esp-wireless-drivers-3rdparty +WIRELESS_DRV_ZIP = master.zip +WIRELESS_DRV_URL = https://github.com/espressif/esp-wireless-drivers-3rdparty/archive Review comment: > I only added comments to some simple things because I cannot really review the actual contents of this PR since I'm not familiar with WiFi code in NuttX. However, I must say that the adapter file has a **lot** going on, it is a really big file with lots of auxiliary functions. Also, I feel that is quite an obscure implementation, it is very difficult to understand what is this code doing in general, I only get little pieces by looking at the small comments for each function. > > It would be good to add some general documentation, either in the form of a README or simple as more detailed comments explaining what all this code is doing and what each part is doing. > I fear that besides the obvious fact that there's some closed code behind this, this code itself will be difficult to maintain/debug by someone else as is. > > That said, I really appreciate the support for ESP32 WiFi on NuttX. There are 2 main source files, one is `esp32_wlan.c` and another is `esp32_wifi_adapter.c`, we can think the `esp32_wlan.c` is alike with `esp32_emac.c`, and it realizes the Ethernet communication. `esp32_wifi_adapter.c` is mainly contains of `OS adapter` fucntions, like `task, semaphore, mutex, message queue`, even including `libc`, `log`, `file system` and so on. So it mainly have no specific logic. But when espressif developed the wifi driver, they don't fully consider porting this driver to other OS platforms, so there are some complex designing which I think should be taken into libraries. Because these logic functions are not related to porting OS adapter functions. So I have been trying to improve the wifi adapter layer to reduce the functions which we should add to support other OS. ########## File path: arch/xtensa/src/esp32/Make.defs ########## @@ -136,3 +136,36 @@ ifeq ($(CONFIG_ARCH_USE_MODULE_TEXT),y) CHIP_CSRCS += esp32_modtext.c CMN_ASRCS += xtensa_loadstore.S endif + +ifeq ($(CONFIG_ESP32_WIRELESS),y) +WIRELESS_DRV_UNPACK = esp-wireless-drivers-3rdparty +WIRELESS_DRV_ZIP = master.zip +WIRELESS_DRV_URL = https://github.com/espressif/esp-wireless-drivers-3rdparty/archive Review comment: Yes, you are right. Because the `3rdparty` is just created, and we will talk about create like branch `release/vX.X` to bind with NuttX specific branch, so now please let me use master now. ########## File path: arch/xtensa/src/esp32/Kconfig ########## @@ -531,4 +533,24 @@ config ESP32_ETH_PHY_ADDR endmenu # ESP32_EMAC +menu "WiFi configuration" + depends on ESP32_WIRELESS + +config ESP32_WIFI_SAVE_PARAM + bool "Save WiFi Parameters" + default n + help + Enable this option, WiFi adapter save WiFi parameters + into file system without generating or calculating some + connection parameters again. Review comment: Great, I will list the more important parameters for users, but some background parameters for wifi, I think they are not needed to be mentioned, because users don't use this directly and they event don't care about this. ########## File path: arch/xtensa/src/esp32/esp32_wifi_adapter.c ########## @@ -0,0 +1,4443 @@ +/**************************************************************************** + * arch/xtensa/src/esp32/esp32_wifi_adapter.c + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + ****************************************************************************/ + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include <stddef.h> +#include <stdlib.h> +#include <stdio.h> +#include <string.h> +#include <assert.h> +#include <pthread.h> +#include <mqueue.h> +#include <fcntl.h> +#include <unistd.h> +#include <clock/clock.h> +#include <sys/time.h> +#include <sys/types.h> +#include <sys/stat.h> +#include "nuttx/kmalloc.h" +#include "nuttx/spinlock.h" +#include <nuttx/irq.h> +#include <nuttx/semaphore.h> +#include <nuttx/kthread.h> +#include <nuttx/wdog.h> +#include <nuttx/wqueue.h> +#include <nuttx/sched.h> +#include <nuttx/signal.h> + +#include "xtensa.h" +#include "xtensa_attr.h" +#include "hardware/esp32_dport.h" +#include "hardware/esp32_emac.h" +#include "esp32_cpuint.h" +#include "esp32_wifi_adapter.h" + +#include "espidf_wifi.h" + +/**************************************************************************** + * Pre-processor Definitions + ****************************************************************************/ + +#define PHY_RF_MASK ((1 << PHY_BT_MODULE) | (1 << PHY_WIFI_MODULE)) + +#ifdef CONFIG_ESP32_WIFI_SAVE_PARAM +# define NVS_FS_PREFIX CONFIG_ESP32_WIFI_FS_MOUNTPT +# define NVS_DIR_BASE NVS_FS_PREFIX"/wifi." +# define NVS_FILE_MODE 0777 +#endif + +#define WIFI_CONNECT_TIMEOUT (10) Review comment: Of course. ########## File path: arch/xtensa/src/esp32/Make.defs ########## @@ -136,3 +136,36 @@ ifeq ($(CONFIG_ARCH_USE_MODULE_TEXT),y) CHIP_CSRCS += esp32_modtext.c CMN_ASRCS += xtensa_loadstore.S endif + +ifeq ($(CONFIG_ESP32_WIRELESS),y) +WIRELESS_DRV_UNPACK = esp-wireless-drivers-3rdparty +WIRELESS_DRV_ZIP = master.zip +WIRELESS_DRV_URL = https://github.com/espressif/esp-wireless-drivers-3rdparty/archive Review comment: > Before merging it will be better to point to a specific commit, even if that commit is the latest one. The point is that the build should be reproducible and using master means that every build could be different since master changed in between. If you later need to update the commit, a quick PR is all that is needed to change the hash. OK, it is meaningful. ########## File path: arch/xtensa/src/esp32/Make.defs ########## @@ -136,3 +136,36 @@ ifeq ($(CONFIG_ARCH_USE_MODULE_TEXT),y) CHIP_CSRCS += esp32_modtext.c CMN_ASRCS += xtensa_loadstore.S endif + +ifeq ($(CONFIG_ESP32_WIRELESS),y) +WIRELESS_DRV_UNPACK = esp-wireless-drivers-3rdparty +WIRELESS_DRV_ZIP = master.zip +WIRELESS_DRV_URL = https://github.com/espressif/esp-wireless-drivers-3rdparty/archive Review comment: > > `esp32_wifi_adapter.c` is mainly contains of `OS adapter` fucntions, like `task, semaphore, mutex, message queue`, even including `libc`, `log`, `file system` and so on. So it mainly have no specific logic. > > It would be better to split this file then into different files, each with a specific function. It will be easier to review and maintain. > > But please try to add some general comment of what the code is doing, how this adapting layer works, etc. Adding more comment to introduce the principle how the adapter works is meaningful. But I would not like to split `adapter` into multiply files, because they are adapter's private function, and mainly just adapter of OS APIs, not realize some wifi functions. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
