[component][net][at] at_client增加deInit接口 #10598
Conversation
📌 Code Review Assignment🏷️ Tag: componentsReviewers: Maihuanyi Changed Files (Click to expand)
🏷️ Tag: components_net_atReviewers: Ryan-CW-Code lizhen9880 Changed Files (Click to expand)
📊 Current Review Status (Last Updated: 2025-08-11 20:20 CST)
📝 Review Instructions
|
|
上面实现中，at_client_rx_ind使用了 更新: 已修改为轮询方式 |
|
CI错误都是at_device软件包的报错，合并此PR后会对at_device提PR |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a deInit interface to the at_client component, enabling proper cleanup and deinitialization of AT clients. The implementation refactors the client management from a static array to a dynamic linked list structure and modernizes synchronization mechanisms.
- Replaces static client array with dynamic linked list for better memory management
- Migrates from semaphore-based to event-based synchronization for improved efficiency
- Adds
at_client_deInit()function to properly clean up AT client resources
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| components/net/at/src/at_client.c | Implements deInit functionality, converts to linked list management, and updates synchronization mechanisms |
| components/net/at/include/at.h | Updates client struct to use events/mutexes and declares deInit function |
components/net/at/src/at_client.c
Outdated
|
|
||
| if (client == RT_NULL) | ||
| { | ||
| LOG_E("input AT client object is NULL, please create or get AT Client object!"); | ||
| return -RT_ERROR; | ||
| } | ||
|
|
||
| resp = at_create_resp(64, 0, rt_tick_from_millisecond(300)); | ||
| resp = at_create_resp(64, 0, rt_tick_from_millisecond(1000)); |
There was a problem hiding this comment.
The timeout value was changed from 300ms to 1000ms without explanation. This magic number should be documented or defined as a constant to explain the reasoning for this specific timeout value.
| resp = at_create_resp(64, 0, rt_tick_from_millisecond(1000)); | |
| resp = at_create_resp(64, 0, rt_tick_from_millisecond(AT_CLIENT_CONN_RESP_TIMEOUT_MS)); |
Copilot uses AI. Check for mistakes.
components/net/at/src/at_client.c
Outdated
| (void (*)(void *parameter))client_parser, | ||
| client, | ||
| 1024 + 512, | ||
| 9, |
There was a problem hiding this comment.
The thread priority was changed from a calculated value to a hardcoded '9'. This magic number should be defined as a constant or the calculation should be restored with proper documentation explaining why this specific priority is chosen.
| 9, | |
| AT_CLIENT_THREAD_PRIORITY, |
Copilot uses AI. Check for mistakes.
| { | ||
| is_full = RT_TRUE; | ||
| rt_event_send(&client->event, at_client_deInit_over_event); | ||
| rt_thread_delete(rt_thread_self()); |
There was a problem hiding this comment.
Calling rt_thread_delete() on the current thread (rt_thread_self()) is generally unsafe and can lead to undefined behavior. Consider using rt_thread_exit() or restructuring the code to allow the thread to terminate naturally.
| rt_thread_delete(rt_thread_self()); | |
| rt_thread_exit(); |
Copilot uses AI. Check for mistakes.
components/net/at/src/at_client.c
Outdated
|
|
||
| return &at_client_table[0]; | ||
| rt_base_t level = rt_hw_interrupt_disable(); | ||
| client = rt_slist_first_entry(&g_at_client_list, struct at_client, list); |
There was a problem hiding this comment.
This code will crash if the list is empty because rt_slist_first_entry() doesn't check for an empty list. Add a check using rt_slist_isempty() before calling rt_slist_first_entry().
| client = rt_slist_first_entry(&g_at_client_list, struct at_client, list); | |
| if (!rt_slist_isempty(&g_at_client_list)) | |
| { | |
| client = rt_slist_first_entry(&g_at_client_list, struct at_client, list); | |
| } |
Copilot uses AI. Check for mistakes.
components/net/at/src/at_client.c
Outdated
| break; | ||
| } | ||
| } | ||
| rt_hw_interrupt_enable(level); | ||
|
|
||
| return RT_NULL; | ||
| return client; |
There was a problem hiding this comment.
The loop may exit with client pointing to the last entry even if no match was found. The function should set client to RT_NULL when no match is found, or return RT_NULL explicitly in the else case.
Copilot uses AI. Check for mistakes.
| return -RT_EFULL; | ||
| } | ||
|
|
||
| // Since the buffer state is uncertain, we proactively clear it; the overhead is negligible. |
There was a problem hiding this comment.
[nitpick] The comment uses '//' style which is inconsistent with the existing C-style '/* */' comments used throughout the file. Consider using consistent comment style.
| // Since the buffer state is uncertain, we proactively clear it; the overhead is negligible. | |
| /* Since the buffer state is uncertain, we proactively clear it; the overhead is negligible. */ |
Copilot uses AI. Check for mistakes.
| } | ||
| else | ||
| { | ||
| /* log_d("unrecognized line: %.*s", client->recv_line_len, client->recv_line_buf);*/ | ||
| // rt_kprintf("unrecognized line: %.*s", client->recv_line_len, client->recv_line_buf); |
There was a problem hiding this comment.
This commented-out debug code should be removed or replaced with a proper logging mechanism if debugging is needed.
| // rt_kprintf("unrecognized line: %.*s", client->recv_line_len, client->recv_line_buf); | |
| LOG_D("unrecognized line: %.*s", client->recv_line_len, client->recv_line_buf); |
Copilot uses AI. Check for mistakes.
|
@Rbb666 |
我研究下 |
拉取/合并请求描述：(PR description)
[
为什么提交这份PR (why to submit this PR)
你的解决方案是什么 (what is your solution)
请提供验证的bsp和config (provide the config and bsp)
V2版本的串口，已在at_socket没有使用deinit测试3天，ppp拨号中每隔3分钟deinit一次测试4天。
V1版本的串口，在at_socket测试一个小时。
已知待解决：at_device软件包中直接访问at_client的lock变量需要修改一下，合并后就准备给at_device提PR
]
当前拉取/合并请求的状态 Intent for your PR
必须选择一项 Choose one (Mandatory):
代码质量 Code Quality：
我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:
#if 0代码，不包含已经被注释了的代码 All redundant code is removed and cleaned up