-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat - sync audio file using wifi AP instead of STA #4129
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
Conversation
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 refactors the Wi-Fi functionality from Station (STA) mode to Access Point (AP) mode, allowing clients to connect directly to the device. While this is a significant feature change, the implementation introduces several critical issues. There is a fundamental design flaw where the device acts as a TCP client instead of a server, attempting to connect to a hardcoded IP. Additionally, a race condition has been introduced by removing atomic protection for a shared variable. Multiple stack and buffer size reductions across the firmware also pose a high risk of system instability and data loss without supporting analysis. These critical issues must be addressed before merging.
I am having trouble creating individual review comments. Click here to see my feedback.
omi/firmware/omi/omi.conf (323)
The CONFIG_MAIN_STACK_SIZE has been reduced from 8192 to 4096 bytes. This significantly increases the risk of a stack overflow, which can lead to unpredictable crashes and is notoriously difficult to debug. This change should be reverted unless stack usage analysis has been performed to prove that 4096 bytes is sufficient for the main thread under all conditions. The addition of stack monitoring tools (CONFIG_THREAD_STACK_INFO) is good, but such a drastic reduction is dangerous without concrete data confirming it's safe.
CONFIG_MAIN_STACK_SIZE=8192
omi/firmware/omi/src/lib/core/codec.c (49)
The stack size for the codec thread has been drastically reduced from 32000 to 19000 bytes. Audio processing, especially with codecs like Opus, can be stack-intensive. This reduction introduces a significant risk of stack overflow, which would cause a critical system failure. Please revert this change unless stack profiling data can prove this smaller size is safe under worst-case conditions.
K_THREAD_STACK_DEFINE(codec_stack, 32000);omi/firmware/omi/src/wifi.c (37)
The global variable wifi_ready_status has been changed from an atomic_t to a bool, but it is accessed from multiple threads without synchronization. It is written in wifi_ready_cb (a callback likely executed in a system thread) and read in start_wifi_thread. This creates a race condition that can lead to unpredictable behavior.
To fix this, wifi_ready_status should be restored to an atomic_t to ensure thread-safe access. All reads and writes must use the atomic API:
- The declaration should be
static atomic_t wifi_ready_status; - Writes (e.g., in
wifi_ready_cb) should useatomic_set(&wifi_ready_status, wifi_ready); - Reads (e.g., in
start_wifi_thread) should useif (!atomic_get(&wifi_ready_status)) { ... }
static atomic_t wifi_ready_status;omi/firmware/omi/src/wifi.c (55-56)
The device is configured as a Wi-Fi Access Point, but the TCP communication logic implements a client that attempts to connect to a hardcoded client IP address (192.168.1.5). This is a fundamental design flaw.
In an AP role, the device should act as a TCP server, listening for and accepting incoming connections from clients that connect to its network. The current implementation is brittle and will fail if the client uses a different IP address or if multiple clients are present. The tcp_client_start function should be redesigned to implement a TCP server using bind(), listen(), and accept() instead of connect().
omi/firmware/omi/src/wifi.c (588-590)
The stack size for start_wifi_thread has been halved from 8192 to 4096 bytes. The networking stack, particularly in AP mode managing connections, can have significant and variable stack requirements. This reduction poses a high risk of stack overflow, leading to system instability. It is strongly recommended to revert this change unless detailed stack usage analysis confirms that 4096 bytes provides a safe margin.
K_THREAD_DEFINE(start_wifi_thread_id, 8192,
start_wifi_thread, NULL, NULL, NULL,
6, 0, -1);omi/firmware/omi/src/lib/core/button.c (420)
The call to wifi_turn_off() is commented out. The turnoff_all() function is responsible for gracefully shutting down all peripherals before powering off the system. By not calling wifi_turn_off(), the WiFi module may be left in an inconsistent state, potentially causing issues on the next boot. This call should be restored.
wifi_turn_off();omi/firmware/omi/src/lib/core/storage.c (289-324)
The logic for handling Wi-Fi setup has been changed to only accept an SSID. This is consistent with the move to AP mode. However, the corresponding server implementation in wifi.c now attempts to connect to a hardcoded client IP. The device should be acting as a server, not a client. This part of the logic in storage.c is now disconnected from a proper server implementation.
omi/firmware/omi/src/mic.c (34)
The number of DMIC buffer blocks (BLOCK_COUNT) has been halved from 8 to 4. This reduces the buffer capacity for incoming audio data. If the audio processing thread is delayed for any reason (e.g., due to other high-priority tasks), this smaller buffer is more likely to overflow, leading to lost audio samples and degraded recording quality. This change should be reverted unless performance testing confirms that 4 blocks are sufficient to prevent data loss under all operating conditions.
#define BLOCK_COUNT 8omi/firmware/omi/src/sd_card.c (19)
The size of the SD card request queue (SD_REQ_QUEUE_MSGS) has been reduced from 30 to 25. If the system is under heavy load and generates SD card requests (e.g., writing audio data) faster than the SD card can process them, a smaller queue increases the likelihood of the message queue filling up. This would cause k_msgq_put to fail, leading to dropped requests and potential data loss. Consider reverting this change to maintain a larger buffer for requests.
#define SD_REQ_QUEUE_MSGS 30 // Number of messages in the SD request queue
1. Configure WiFi & TCP Server (WIFI_SETUP)Command: 0x01 Payload format:
2. Enable WiFi (WIFI_START)Command: 0x02 3. Disable WiFi (WIFI_SHUTDOWN)Command: 0x03 Wi-Fi Error Codes
|
|
@TuEmb is this one ready to be merged? |
| int16_t codec_input_samples[CODEC_PACKAGE_SAMPLES]; | ||
| uint8_t codec_output_bytes[CODEC_OUTPUT_MAX_BYTES]; | ||
| K_THREAD_STACK_DEFINE(codec_stack, 32000); | ||
| K_THREAD_STACK_DEFINE(codec_stack, 19000); |
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.
| */ | ||
| #define MAX_BLOCK_SIZE BLOCK_SIZE(MAX_SAMPLE_RATE, 2) | ||
| #define BLOCK_COUNT 8 | ||
| #define BLOCK_COUNT 4 |
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.
mic buffer works well with ~0.4s buffer data
|
@beastoin It's ready to be merged |
|
lgtm @TuEmb |


This pull request introduces wifi sync feature using AP mode (removed STA mode). The device will create an access point, the phone app can connect and get audio file faster and easier. The AP mode needs a lot of RAM usage, I optimised some other tasks to reserved RAM for heap pool.
32000bytes -->19000bytes (this thread is using ~18000 bytes)