Skip to content

[enhancement] Add string information for error #3186

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

Merged
merged 7 commits into from
Jun 29, 2022
Merged

[enhancement] Add string information for error #3186

merged 7 commits into from
Jun 29, 2022

Conversation

a1012112796
Copy link
Contributor

@a1012112796 a1012112796 commented Nov 7, 2019

拉取/合并请求描述:(PR description)

[

  1. add rt_strerror();
  2. chang %03d to %s in list_thread errorno
    test on stm32f427-robomaster-a with mdk5
    result:
    图片

]

以下的内容不应该在提交PR时的message修改,修改下述message,PR会被直接关闭。请在提交PR后,浏览器查看PR并对以下检查项逐项check,没问题后逐条在页面上打钩。
The following content must not be changed in submitted PR message. Otherwise, the PR will be closed immediately. After submitted PR, please use web browser to visit PR, and check items one by one, and ticked them if no problem.

当前拉取/合并请求的状态 Intent for your PR

必须选择一项 Choose one (Mandatory):

  • 本拉取/合并请求是一个草稿版本 This PR is for a code-review and is intended to get feedback
  • 本拉取/合并请求是一个成熟版本 This PR is mature, and ready to be integrated into the repo

代码质量 Code Quality:

我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:

  • 已经仔细查看过代码改动的对比 Already check the difference between PR and old code
  • 代码风格正确,包括缩进空格,命名及其他风格 Style guide is adhered to, including spacing, naming and other style
  • 没有垃圾代码,代码尽量精简,不包含#if 0代码,不包含已经被注释了的代码 All redundant code is removed and cleaned up
  • 所有变更均有原因及合理的,并且不会影响到其他软件组件代码或BSP All modifications are justified and not affect other components or BSP
  • 对难懂代码均提供对应的注释 I've commented appropriately where code is tricky
  • 本拉取/合并请求代码是高质量的 Code in this PR is of high quality

Copy link
Contributor

@rtthread-bot rtthread-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AutoTest Result

Rate: 80.95%
Passed: 17/21

点击查看详情

Copy link
Contributor

@rtthread-bot rtthread-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AutoTest Result

Rate: 80.95%
Passed: 17/21

点击查看详情

Copy link
Member

@BernardXiong BernardXiong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please clean your PR.

src/kservice.c Outdated
ret = (error > RT_EINVAL)?
rt_errno_verbose[RT_EINVAL+1]:
rt_errno_verbose[error];
}else if(mode == RT_STRERROR_SHORT)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

请保持RTT的代码风格,每个if/else都在一行上,if与(间的空格需要留出。

src/kservice.c Outdated
@@ -909,9 +972,14 @@ rt_int32_t rt_vsnprintf(char *buf,
++ str;
}
continue;

case 'M':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不建议加入m/M,这个意义并不那么大,反而引入新的格式。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

您好,我觉得引入%m是有意义的,首先可以简化输出错误语句的写法,只需要在rt_kprintf加入一个参数而不是调用一个函数。其次,可以像%s一样用+-号等实现输出格式控制,并且%m是标准c规定的参数格式,也不算新的格式。所以我强烈建议加入%m,谢谢!

src/kservice.c Outdated
@@ -114,6 +114,69 @@ int *_rt_errno(void)
}
RTM_EXPORT(_rt_errno);

const char rt_errno_verbose[][24] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

请加入static修饰,仅限于这个文件使用。

src/kservice.c Outdated
@@ -114,6 +114,69 @@ int *_rt_errno(void)
}
RTM_EXPORT(_rt_errno);

const char rt_errno_verbose[][24] =
{
"no error",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

首字母请大写

src/kservice.c Outdated
"Unkown error"
};

const char rt_errno_short[][7] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

short模式意义相对比较小,请移除吧

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

您好,经过仔细分析,我觉得short模式比long模式各有意义,我的理由如下:首先对开发者而言,标准错误只是一个符号,用一个简单的,利于理解的缩写来表示,就已经足够了。输出一句短语固然显得更加友好,但实际意义并不大,其二,short模式占据的存储空间较小,并且在输出时可以方便的放入表格等输出方式中,例如我在list_thread中所做的修改。所以我建议删去long模式保留short模式作为唯一输出格式,并对部分缩写做了扩展。谢谢!

Copy link
Contributor

@rtthread-bot rtthread-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AutoTest Result

Rate: 100.00%
Passed: 21/21

点击查看详情

@a1012112796
Copy link
Contributor Author

还有一点不知是否必要修改,就是经过修改后rt_strerror函数的逻辑行数只有三行,请问是否可以添加rt_inline参数修饰?谢谢

Copy link
Contributor

@rtthread-bot rtthread-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AutoTest Result

Rate: 100.00%
Passed: 21/21

点击查看详情

Copy link
Contributor

@rtthread-bot rtthread-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AutoTest Result

Rate: 100.00%
Passed: 21/21

点击查看详情

@a1012112796
Copy link
Contributor Author

@BernardXiong 您好,请问方便给个原因吗,谢谢

@mysterywolf
Copy link
Member

mysterywolf commented Jun 20, 2022

@a1012112796 请采用merge方式合并这个pr https://github.com/a1012112796/rt-thread/pulls

@mysterywolf
Copy link
Member

@a1012112796 请修复一下CI的报错

Co-authored-by: Man, Jianting (Meco) <920369182@qq.com>
@mysterywolf mysterywolf requested a review from Guozhanxin June 23, 2022 00:32
@mysterywolf mysterywolf added the +1 Agree +1 label Jun 23, 2022
@mysterywolf
Copy link
Member

mysterywolf commented Jun 23, 2022

@Guozhanxin @BernardXiong 请再考虑一下这份PR,这份PR没有问题。 论坛中也有提到过相关的问题,最后一行错误码除了非常了解rtt的人之外,并不了解其含义。https://club.rt-thread.org/ask/question/f4810b30e049e655.html

我已经再次review和测试了,没有问题:
image

Signed-off-by: a1012112796 <1012112796@qq.com>
@Guozhanxin Guozhanxin added the +2 Agree +2 label Jun 27, 2022
@BernardXiong BernardXiong merged commit 697bf13 into RT-Thread:master Jun 29, 2022
@a1012112796 a1012112796 deleted the writing_zzc_addstrerror branch June 29, 2022 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
+1 Agree +1 +2 Agree +2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants