-
Notifications
You must be signed in to change notification settings - Fork 5.2k
bsp:k230:add support for temperature sensor driver #10609
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
📌 Code Review Assignment🏷️ Tag: bsp_k230Reviewers: unicornx Changed Files (Click to expand)
📊 Current Review Status (Last Updated: 2025-08-19 16:45 CST)
📝 Review Instructions
|
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.
第一次 review。
请不要把 vendor 的代码直接拿过来,写得不合适的地方要把把关,能改进的要改进。
bsp/k230/board/Kconfig
Outdated
@@ -4,6 +4,11 @@ menu "Drivers Configuration" | |||
bool "Enable ADC" | |||
select RT_USING_ADC | |||
default n | |||
|
|||
config BSP_USING_TS | |||
bool "Enable TS" |
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.
建议 menu 上不要用 TS 这个缩写,不是啥专有名词,不好理解,直接用全称吧。
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.
这里sdk上的全程是tsensor,用这个吗,还是temperature_sensor
bsp/k230/drivers/utest/test_ts.c
Outdated
ts_dev = (rt_device_t)rt_device_find(TS_DEV_NAME); | ||
if (ts_dev == RT_NULL) | ||
{ | ||
uassert_false(1); |
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.
可以对 ts_dev 直接 uassert
bsp/k230/drivers/utest/test_ts.c
Outdated
|
||
if(rt_device_open(ts_dev, RT_DEVICE_OFLAG_RDWR) != RT_EOK) | ||
{ | ||
LOG_E("open %s device failed!\n", TS_DEV_NAME); |
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.
用 uassert
bsp/k230/drivers/utest/test_ts.c
Outdated
rt_uint32_t cnt; | ||
double temp = 0; | ||
ts_open(); | ||
for(cnt=0; cnt<5; cnt++) |
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.
代码不要挤在一起,关键字,变量名之间用空格隔开
例子:
for (cnt = 0; cnt < 5; cnt++)
bsp/k230/drivers/utest/test_ts.c
Outdated
reval = rt_device_read(ts_dev, 0, &temp, sizeof(double)); | ||
if(reval <= 0) | ||
{ | ||
uassert_false(1); |
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.
直接对 reval 进行 uassert
} | ||
|
||
static void tsensor_stop(void) { | ||
if (ts_base_addr == RT_NULL) return; // Ensure base address is set |
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.
如果要检查错误,那就应该第一时间返回错误,但这个函数返回了 void!
} | ||
|
||
static int tsensor_read_data(uint16_t *data, uint32_t timeout_ms) { | ||
if (ts_base_addr == RT_NULL || data == RT_NULL) return -1; // Ensure base address is set |
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.
不要返回立即数,用恰当的错误返回,不要用立即数。
全文检查。不再赘述
|
||
static rt_err_t ts_deivce_open(rt_device_t dev, rt_uint16_t oflag) | ||
{ | ||
tsensor_init(); |
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.
初始化如果失败了怎么办,仍然返回ok?
{ | ||
if(sizeof(double) != size) { | ||
LOG_E("%s invalid buffer size %u\n", __func__, size); | ||
return 0; |
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.
返回值 0 一般代表成功,即 RT_EOK。 可是这里是要返回失败!
全文检查不再赘述
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.
测试用例没有覆盖 ts_device_control, 建议增加
收到,我再继续改一下 |
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.
第二次 review,只有一个小问题,其他 LGTM。
|
||
config BSP_USING_TS | ||
bool "Enable TS" | ||
select RT_USING_TS |
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.
“Enable Temperature Sensor”
写全称吧,给人看的,不要简写。
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.
收到
@Rbb666 please review and merge, thanks. |
@DannyRay019 CI Check 有错误,请用 format 工具检查一下。 |
Added a temperature sensor driver and a test file test_ts.c. The test uses temperature sensor to measure the chip temperature, to check if the driver works correctly. Signed-off-by: XU HU 1337858472@qq.com
@Rbb666 检查完毕,请 review & merge,谢谢 |
Added a temperature sensor driver and a test file test_ts.c.
The test uses temperature sensor to measure the chip temperature,
to check if the driver works correctly.
Signed-off-by: XU HU 1337858472@qq.com