wi-fi: Add CYW4373E USB WiFi modem support #118
Conversation
Summary of ChangesHello @julianuziemblo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the hardware support by integrating the CYW4373E USB WiFi modem. It introduces a new USB HAL driver and a message buffer protocol for the Wireless Host Driver, alongside refactoring the existing bus communication architecture. These changes aim to improve modularity, efficiency, and adaptability to different WiFi interfaces and firmware capabilities. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Summary of ChangesHello @julianuziemblo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the hardware compatibility of the WiFi stack by integrating support for the CYW4373E USB WiFi modem. It involves a substantial architectural overhaul of the Wireless Host Driver (WHD) to introduce a flexible protocol layer capable of handling various bus types, including the newly added USB. This change ensures that the system can seamlessly communicate with WiFi modules over different interfaces, improving adaptability and future-proofing the driver. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for the CYW4373E USB WiFi modem, which involves adding a new USB HAL implementation and refactoring the WHD layer for multiple bus protocols. The changes are extensive and well-structured. My review has identified a few critical issues in the new cyhal_usb.c and cyabs_rtos.c files, including a logic error in a queue reset function, incorrect loop conditions, and potential infinite loops in error handling. Addressing these points will improve the robustness and correctness of the new USB WiFi support.
Summary of ChangesHello @julianuziemblo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the hardware compatibility of the WiFi driver by adding full support for the CYW4373E USB WiFi modem. The changes involve creating a dedicated USB HAL driver, seamlessly integrating it into the existing Wireless Host Driver (WHD) architecture, and introducing a flexible protocol abstraction layer. This refactoring allows for easier adoption of different communication protocols (like message buffers) and enhances the overall robustness and maintainability of the WiFi stack. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces support for the CYW4373E USB WiFi modem. The changes are extensive, including a new USB Hardware Abstraction Layer (HAL), a new message buffer protocol, and modifications across the codebase to integrate the new USB interface type. While the overall approach seems sound, I've identified a few critical issues in the new USB HAL implementation that could lead to system lock-ups. There are also some bugs and documentation inconsistencies in the new RTOS queue abstraction that should be addressed. Additionally, I've pointed out the use of a magic number that should be replaced with a named constant for better code clarity and maintainability.
There was a problem hiding this comment.
Code Review
This pull request adds support for the CYW4373E USB WiFi modem. The changes are extensive, touching many files to introduce the USB interface and adapt the existing code to be bus-agnostic. New HAL files for USB are added, and existing HAL and WHD files are refactored.
I've found a few issues, including a critical bug in the new queue implementation, and several high-severity issues in the new USB HAL implementation related to memory management, potential infinite loops, and uninitialized variables. There are also some medium-severity issues regarding busy-wait loops. Please see my detailed comments.
julianuziemblo
force-pushed
the
julianuziemblo/RTOS-216
branch
from
a526315 to
a1076ba
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for the CYW4373E USB WiFi modem. This is a significant feature addition, introducing a new USB interface type alongside the existing SDIO, SPI, and M2M interfaces. The changes include a new USB HAL implementation (cyhal_usb.c), updates to Makefiles, and modifications to existing HAL and BSP files to accommodate the new interface. Additionally, a lock-free SPSC queue implementation has been added to cyabs_rtos.c.
My review focuses on the new code. I've found a few issues in the new queue implementation in cyabs_rtos.c and in the new USB HAL in cyhal_usb.c. Specifically, the queue item counting and space calculation logic is incorrect, and the USB bulk send function does not handle short writes, which could lead to data loss. I've provided suggestions to fix these issues. The rest of the changes for integrating the USB support look good and are well-structured.
julianuziemblo
force-pushed
the
julianuziemblo/RTOS-216
branch
from
a1076ba to
47e0e75
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for the CYW4373E USB WiFi modem, which involves significant changes across the HAL, WHD, and LwIP port layers. The changes include adding a new USB HAL implementation, refactoring the bus protocol layer to support USB, and updating various files to accommodate the new hardware and protocol. Overall, the changes are well-structured, but I've identified a few issues related to potential bugs, code correctness, and maintainability that should be addressed.
There was a problem hiding this comment.
Code Review
This pull request introduces support for the CYW4373E USB WiFi modem, which involves substantial changes across the Wi-Fi stack. Key additions include a new USB HAL implementation, and a significant refactoring of the Wi-Fi Host Driver (WHD) to abstract the bus protocol layer, enabling support for SDIO, SPI, and USB interfaces, as well as different data protocols like CDC/BDC and the new MSGBUF. The changes are extensive but generally well-structured, moving towards a more modular design. I have identified a critical bug that could lead to a crash, along with a couple of high-severity issues related to portability and potential performance problems that should be addressed.
julianuziemblo
force-pushed
the
julianuziemblo/RTOS-216
branch
3 times, most recently
from
8103402 to
2191b19
Compare
|
was the new version of WHD driver tested on DTR platform? |
julianuziemblo
force-pushed
the
julianuziemblo/RTOS-216
branch
3 times, most recently
from
30835af to
30f6d97
Compare
Tested, works, even with current firmware, CLM and NVRAM. |
julianuziemblo
force-pushed
the
julianuziemblo/RTOS-216
branch
5 times, most recently
from
fc245ba to
a0c9292
Compare
julianuziemblo
force-pushed
the
julianuziemblo/RTOS-216
branch
from
a0c9292 to
f3bc2e2
Compare
Imported from Infineon's AIROC-Wi-Fi-Bluetooth-STM32 repository v1.7.1.5645 New WHD version is 3.3.0.24096 JIRA: RTOS-216
JIRA: RTOS-216
JIRA: RTOS-216
If it's not defined there, the fallback option is SDIO interface JIRA: RTOS-216
JIRA: RTOS-216
JIRA: RTOS-216
JIRA: RTOS-216
JIRA: RTOS-216
julianuziemblo
force-pushed
the
julianuziemblo/RTOS-216
branch
2 times, most recently
from
9d95cb0 to
6c57a6d
Compare
JIRA: RTOS-216
Bus failure is usually indicative of device disconnect, so we can just exit when this happens. JIRA: RTOS-216
JIRA: RTOS-216
Some targets may not support the way periferal access is done in other targets (at least for now). JIRA: RTOS-216
JIRA: RTOS-216
JIRA: RTOS-216
julianuziemblo
force-pushed
the
julianuziemblo/RTOS-216
branch
from
6c57a6d to
9eaa89e
Compare
clang-format-diffandcodespellchecked locally to be passing because of the PR's sizeDescription
Motivation and Context
Types of changes
How Has This Been Tested?
armv7m7-imxrt117x-evk,armv7a7-imx6ull-dataroChecklist:
Special treatment