Re: [PR] xtensa/esp32: use common Espressif wireless source [nuttx]
jerpelea merged PR #15853: URL: https://github.com/apache/nuttx/pull/15853 -- 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. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] xtensa/esp32: use common Espressif wireless source [nuttx]
tmedicci commented on PR #15853: URL: https://github.com/apache/nuttx/pull/15853#issuecomment-2681656097 Hi! Is there anything else on this PR? Can it be merged? (This PR is available for review for 8 days. Previous [PR](https://github.com/apache/nuttx/pull/15749#issuecomment-2648980512) - with both ESP32 and ESP32-S3 - was submitted on Feb 3rd and reviewed there) -- 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. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] xtensa/esp32: use common Espressif wireless source [nuttx]
fdcavalcanti commented on PR #15853: URL: https://github.com/apache/nuttx/pull/15853#issuecomment-2681652366 Rebased and added a sign off. Can we go forward? -- 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. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] xtensa/esp32: use common Espressif wireless source [nuttx]
fdcavalcanti commented on PR #15853: URL: https://github.com/apache/nuttx/pull/15853#issuecomment-2677185775 > Thanks @tmedicci for the details !! Sorry for the noise :-) We are trying to work out the real world hardware automation and the problem we saw is security - untrusted code from PR lunached on local / home / company device and network. This is a separate subject but thus my questions - would it be possible to download everything before build along with the code, then launch offline jails where execution takes place :-) It is possible to do offline build, yes. You can download the external dependencies and set an environment variable that contains the path to those downloaded dependencies. Then, when building, NuttX will look for the local path. Of course, with time it must be updated as more boards and bugfixes are added. We can discuss this somewhere else and I can give some more detailed instructions. -- 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. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] xtensa/esp32: use common Espressif wireless source [nuttx]
cederom commented on PR #15853: URL: https://github.com/apache/nuttx/pull/15853#issuecomment-2676495183 Thanks @tmedicci for the details !! Sorry for the noise :-) We are trying to work out the real world hardware automation and the problem we saw is security - untrusted code from PR lunached on local / home / company device and network. This is a separate subject but thus my questions - would it be possible to download everything before build along with the code, then launch offline jails where execution takes place :-) -- 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. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] xtensa/esp32: use common Espressif wireless source [nuttx]
tmedicci commented on PR #15853: URL: https://github.com/apache/nuttx/pull/15853#issuecomment-2676313004 Hi Tomek, let me answer about these points: > * From what I understand also from other threads and updates Espressif decided to replace "legacy" driver approach with external "HAL" approach? If so this seems a breaking change and should be first discussed on the mailing list with the community. I know this may be easier to code / maintain for you but what is the impact for long term compatibility? This isn't true. We are only merging drivers that implement the same peripherals for the same arch in a single common driver. This PR is only removing duplicated code. It's easier for us - Espresfif - and for anyone on NuttX to maintain. This isn't a breaking change. The interfaces are the same, but built from a common source at `arch/xtensa/src/common/espressif/`. > * If external HAL approach is used, what are external packages dependencies required, and can code build fully offline? If no offline build is possible then what packages are required and how to setup them for offline build? This is important for our automated build and runtime testing we are working on. Please update documentation in a free moment where required :-) Offline build isn't possible at the moment. There are static libraries and header files that are downloaded from the internet. The same is true for other chips from other vendors (look for `EXTRA_LIBS`). NuttX relies on external packages for both the `nuttx` (low-level drivers, mostly) and the `nuttx-apps` repositories: NuttX was built this way. We can keep a copy of these files in a container and check the version, but I don't think offline would be possible soon. One final explanation because the name "HAL" causes a lot of confusion. We used to download the Wi-Fi library from this [esp-wireless-drivers-3rdparty](https://github.com/espressif/esp-wireless-drivers-3rdparty) repository up to 2 or 3 years ago. Espressif didn't use this library anymore on ESP-IDF because most of the Wi-Fi-related driver had been opened (the supplicant layer, for instance). What we've done so far in NuttX - especially for the Wi-Fi driver - is to provide a smaller library and build the other required files along with NuttX. These files and the smaller libraries are provided by [`esp-hal-3rdparty`](https://github.com/espressif/esp-hal-3rdparty). Particularly - talking as PMC member, not Espressif's employee - I prefer to have smaller closed libraries and have control over what is being built and linked with the final firmware. The name "HAL" doesn't reflect the real purpose of it. Please, note that this PR doesn't change anything about external libraries: it's jus t merging and removing duplicated files. -- 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. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] xtensa/esp32: use common Espressif wireless source [nuttx]
fdcavalcanti commented on PR #15853: URL: https://github.com/apache/nuttx/pull/15853#issuecomment-2675181503 Any more updates remaining? Can we move on and merge? -- 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. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] xtensa/esp32: use common Espressif wireless source [nuttx]
fdcavalcanti commented on code in PR #15853: URL: https://github.com/apache/nuttx/pull/15853#discussion_r1959599259 ## arch/xtensa/src/common/espressif/Kconfig: ## @@ -720,6 +724,13 @@ config ESPRESSIF_WIFI_LISTEN_INTERVAL For example, if beacon interval is 100 ms and listen interval is 3, the interval for station to listen to beacon is 300 ms. +config ESPRESSIF_WIFI_WLAN_BUFFER_OPTIMIZATION + bool "Enable optimization of WLAN driver buffer" + default n + depends on ARCH_CHIP_ESP32 + ---help--- + Enable optimization of WLAN memory Review Comment: It's actually disabled by default. Those are used for dynamic allocation of the buffers used on the WiFi. Seems to be a legacy thing from ESP32. In fact, this will be **probably** removed soon as I'm testing an upgrade on `esp_wlan.c` to use IOBs. -- 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. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] xtensa/esp32: use common Espressif wireless source [nuttx]
acassis commented on code in PR #15853: URL: https://github.com/apache/nuttx/pull/15853#discussion_r1958685591 ## arch/xtensa/src/common/espressif/Kconfig: ## @@ -720,6 +724,13 @@ config ESPRESSIF_WIFI_LISTEN_INTERVAL For example, if beacon interval is 100 ms and listen interval is 3, the interval for station to listen to beacon is 300 ms. +config ESPRESSIF_WIFI_WLAN_BUFFER_OPTIMIZATION + bool "Enable optimization of WLAN driver buffer" + default n + depends on ARCH_CHIP_ESP32 + ---help--- + Enable optimization of WLAN memory Review Comment: Please give some hints about that are these optimization. Why are they important? And why did they are enable by default? -- 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. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] xtensa/esp32: use common Espressif wireless source [nuttx]
nuttxpr commented on PR #15853: URL: https://github.com/apache/nuttx/pull/15853#issuecomment-2663851137 [**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to meet the NuttX requirements, although some minor improvements could be made. Here's a breakdown: **Strengths:** * **Clear Summary:** The summary explains the "why," "what," and "how" of the changes. The reference to the previous PR (#15816) is helpful for context. * **Impact Assessment:** The impact section addresses all the required points. The description of the user impact (KConfig changes) is particularly important. * **Testing Information:** The testing section provides information on the host and targets used for testing. Mentioning specific defconfigs tested is good. **Areas for Improvement:** * **Missing Issue References:** While the previous PR is mentioned, it's beneficial to link a related NuttX or NuttX Apps issue if one exists. Even if this PR is purely a refactoring following from a previous issue, referencing that issue is helpful for traceability. * **Testing Logs:** The PR claims testing logs are included, but the provided template sections are empty. Actual log snippets demonstrating functionality before and after the change should be included. This provides concrete evidence of successful testing. If logs are too extensive, consider providing a link to an external log file or CI run. * **Hardware Impact Clarification:** The impact on hardware states "Affects only ESP32S3 boards." However, the summary mentions modifying ESP32. This discrepancy should be clarified. Does it affect *both* ESP32 and ESP32S3 or just ESP32S3? If ESP32S3 was already transitioned in the previous PR, that needs to be stated more explicitly, as the current wording implies this PR is the first change for the ESP32S3. * **Build Impact Nuance:** While the practical build impact is minimal, the movement of build instructions _is_ a change, even if minor. Acknowledging this explicitly and mentioning the specific files affected adds clarity. **Recommendation:** The PR is close to meeting the requirements. Adding the missing issue references, including actual testing logs (or links to them), and clarifying the hardware and build impact will strengthen the PR and make it easier for reviewers to assess. -- 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. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] xtensa/esp32: use common Espressif wireless source [nuttx]
fdcavalcanti opened a new pull request, #15853: URL: https://github.com/apache/nuttx/pull/15853 This PR is the second part of the WiFi common layer port for Espressif Xtensa devices. ESP32S3 has been recently merged here #15816. ## Summary This PR modifies ESP32 to use WiFi source code from the common layer, which is already used by ESP32S2 and ESP32S3. KConfig prefixes such as `ESP32_*` are replaced with `ESPRESSIF_*` (`CONFIG_ESP32_WIFI` -> `CONFIG_ESPRESSIF_WIFI`). This is applied through arch/ and also board/. BLE symbols are altered as well, since they share some common wireless implementations. However, source code for BLE has not been merged into the common layer. ## Impact - Impact on User: KConfig changes will cause issues to users who have custom defconfigs, as they should now update the wireless symbols. - Impact on Build: Not in practice, but some build instructions are removed from `arch/xtensa/src/esp32` and merged into `arch/xtensa/src/common/espressif`. - Impact on Hardware: Affects only ESP32S3 boards. - Impact on Documentation: No. - Impact on Security, Compatibility: Only compatibility issue is the KConfig symbols mentioned above. ## Testing Internal CI testing + `sta_softap`, `wifi`, `ble`, `blewifi` defconfigs (among others) have been tested and are performing as expected. -- 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. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
