-
Notifications
You must be signed in to change notification settings - Fork 13
wi-fi: support STA mode #129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: julianuziemblo/RTOS-216
Are you sure you want to change the base?
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 introduces comprehensive support for Wi-Fi Station (client) mode, significantly enhancing the Wi-Fi manager's capabilities. To accommodate both STA and Access Point (AP) modes more effectively, the existing 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces support for Wi-Fi Station (STA) mode and refactors the existing Wi-Fi manager. The single /dev/wifi device is split into three distinct devices: /dev/wifi/ctrl for overall control, /dev/wifi/sta for station mode, and /dev/wifi/ap for access point mode. This is a significant and well-structured change that improves modularity. My review has identified a few issues, including a critical bug in the usage of the Wi-Fi scanning API that could lead to undefined behavior, a high-severity buffer over-read in a string trimming function, and incorrect command parsing. I have also included some medium-severity suggestions to improve code clarity and maintainability.
9daa85e to
784ddcb
Compare
|
/gemini review |
784ddcb to
83a2483
Compare
f3bc2e2 to
9d95cb0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces support for Wi-Fi Station (STA) mode and refactors the existing Wi-Fi manager to handle multiple interfaces: STA, AP, and a control device. The code is well-structured, separating concerns for each mode. However, I have identified several critical issues related to error handling and resource management. These include potential resource leaks during initialization, incorrect state management on failure, and unhandled error return codes from key functions. Addressing these points will improve the robustness and stability of the Wi-Fi manager.
83a2483 to
31c06f8
Compare
9d95cb0 to
6c57a6d
Compare
a05dec7 to
60be5a8
Compare
6c57a6d to
9eaa89e
Compare
JIRA: RTOS-1166
JIRA: RTOS-1166
cmd-line utilities may pass extra space or newline as opposed to NULL byte at the end of the string, making them hold wrong values. JIRA: RTOS-1166
This change required some reorganising of the code. 3 new devices were added in /dev/wifi/: - /dev/wifi/ctrl: control (Wi-Fi on/off) - /dev/wifi/ap: AP mode ctrl (set credentials, start/stop) - /dev/wifi/sta: STA mode ctrl (set credsentials, connect/disconnect) Migration for AP: - turn on Wi-Fi by writing "on" to /dev/wifi/ctrl - set credentials, timeout and start/stop like before, writing to /dev/wifi/ap instead of /dev/wifi Using STA: - turn on Wi-Fi by writing "on" to /dev/wifi/ctrl - set credentials and timeout the same as in AP mode, writing them to /dev/wifi/sta - write "connect"/"disconnect" to /dev/wifi/sta to manage connection to an access point JIRA: RTOS-1166
60be5a8 to
064df9b
Compare
| if (timeout < 0) { | ||
| errno = 0; | ||
| timeout = strtol(buf, &endp, 0); | ||
| if (errno != 0 || endp == buf || timeout == LONG_MAX) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
timeout == LONG_MAX should be already handled by errno != 0 (ERANGE).
| return -1; | ||
| } | ||
|
|
||
| ssid = trim(ssid, &len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move it before SSID size check.
| return -1; | ||
| } | ||
|
|
||
| key = trim(key, &len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move it before PSK size checks.
|
|
||
| static int wifi_dev_write(const char *data, size_t size) | ||
| { | ||
| data = trim(data, &size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that strncmp is not aware of trimmed size. For example if data is timeout it will call wifi_ap_set_idle_timeout(data + 8, 7 - 8).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it will as before strncmp there is size >= 8 check which will not be passed, hence not calling wifi_ap_set_idle_timeout
| struct { | ||
| bool busy; | ||
| char buf[128]; | ||
| char buf[16]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use move it to stack since it's very small.

Description
This PR adds STA mode to Wi-Fi manager. For easier Wi-Fi management, the
/dev/wifidevice had to be split into three devices:/dev/wifi/ctrl: Wi-Fi state management: on/off. Putting Wi-Fi "on" initializes the WHD interfaces: STA as primary and AP as secondary. It also creates the latter 2 devices for managing STA and AP modes./dev/wifi/sta: STA mode management: set connection timeout, set SSID and key of the network you want to connect to, connect/disconnect./dev/wifi/ap: AP mode management: set idle timeout, set SSID and key of the access point, start/stop (interface unchanged)Motivation and Context
STA mode (client mode) needed on some platforms (especially imxrt).
Types of changes
/dev/wifidevice split into three devices (described in "Description" section above)How Has This Been Tested?
armv7m7-imxrt117x-evk, NILEE.Checklist:
Special treatment