Skip to content
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

[doxygen][lwp] add doxygen comment for lwp_dbg.h #9924

Closed
wants to merge 1 commit into from

Conversation

1078249029
Copy link
Contributor

@1078249029 1078249029 commented Jan 16, 2025

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

[

为什么提交这份PR (why to submit this PR)

对debug这方面不熟,各位多捉虫

你的解决方案是什么 (what is your solution)

请提供验证的bsp和config (provide the config and bsp)

  • BSP:
  • .config:
  • action:

]

当前拉取/合并请求的状态 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 styles
  • 没有垃圾代码,代码尽量精简,不包含#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
  • 已经使用formatting 等源码格式化工具确保格式符合RT-Thread代码规范 This PR complies with RT-Thread code specification
  • 如果是新增bsp, 已经添加ci检查到.github/workflows/bsp_buildings.yml 详细请参考链接BSP自查

@github-actions github-actions bot added RT-Smart RT-Thread Smart related PR or issues component: lwp Component labels Jan 16, 2025
@supperthomas supperthomas reopened this Jan 17, 2025
@supperthomas supperthomas reopened this Jan 17, 2025
@unicornx
Copy link
Contributor

@unicornx unicornx self-requested a review January 17, 2025 02:38
*
* SPDX-License-Identifier: Apache-2.0
*
* Change Logs:
* Date Author Notes
* 2023-07-11 RT-Thread first version
* 2025-01-16 wumingzi add doxyzen comment
Copy link
Contributor

Choose a reason for hiding this comment

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

不需要,采用 git 后,写好你的 commit message,加上你的 signed-off 即可。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

commit message我改一下,但是change log也应该保留?有的人开发可能不使用git,况且为了代码风格一致也应该保留吧

Copy link
Contributor

@unicornx unicornx Jan 17, 2025

Choose a reason for hiding this comment

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

commit message我改一下,但是change log也应该保留?有的人开发可能不使用git,况且为了代码风格一致也应该保留吧

change log 随便你吧。

有的人开发可能不使用git

??? 这样的人如果也能给 RTT 提 PR,那么我服了

*
* @param dbg_ops
* @return void
* @ingroup lwp
Copy link
Contributor

Choose a reason for hiding this comment

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

"@InGroup" 是不是可以在开头只写一次,采用 block 方式对整个文件起效果?你研究一下呢?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

\ingroup ( []*)

If the \ingroup command is placed in a comment block of a compound entity (like class, file or namespace), then it will be added to the group(s) identified by the (s). In case of members (like variable, functions, typedefs and enums) the member will be added only to one group (to avoid ambiguous linking targets in case a member is not documented in the context of its class, namespace or file, but only visible as part of a group).

我的理解是ingroup在函数注释中可加可不加,考虑到代码风格还是不加比较好?

Copy link
Contributor

@unicornx unicornx Jan 17, 2025

Choose a reason for hiding this comment

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

@ngroup" 是不是可以在开头只写一次

我解释一下我想说的意思:

一种写法,也就是你目前的写法:

/**
 * @brief Get lwp debug state
 *
 * @return int   lwp debug state
 * @ingroup lwp
 */
int dbg_thread_in_debug(void);
/**
 * @brief Register dbg_ops to struct dbg_ops_t
 *
 * @param dbg_ops
 * @return void
 * @ingroup lwp
 */
void dbg_register(struct dbg_ops_t *dbg_ops);
......

我说的在开头只写一次的意思是,改成下面这样:

/**
 * @ingroup lwp
 * @{
 */

/**
 * @brief Get lwp debug state
 *
 * @return int   lwp debug state
 */
int dbg_thread_in_debug(void);
/**
 * @brief Register dbg_ops to struct dbg_ops_t
 *
 * @param dbg_ops
 * @return void
 */
void dbg_register(struct dbg_ops_t *dbg_ops);

......
/*! @}*/

@unicornx
Copy link
Contributor

unicornx commented Jan 17, 2025

为什么提交这份PR (why to submit this PR)

#9263

#9263 这个 issue 包含了很多内容,不是这个 pr 能够完全解决的,所以不要在 PR 中关联 #9263 。对于 #9263 你可以认为这是一个用于 track 一类问题的。如果关联了反而会导致这个 pr 被 merge 时 github 会自动 close 关联的 issue。但这个 issue 其实不能简单地因为这个 pr 就关闭掉。

所以对于你发现的这个问题,直接提这个 PR 就好了。

int dbg_check_suspend(void);

#endif /* __LWP_DBG_H__ */
#endif /* __LWP_DBG_H__ */
Copy link
Contributor

Choose a reason for hiding this comment

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

这是什么改动?你是不是多删除了行尾的空行?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

是的,我使用formatting工具格式化出来的就是这样。之前我给写驱动的时候尝试添加空行,可是ci过不去。虽然在某些ide上可能会有warning,但我没发现社区有反馈过这个问题的,所以先确保合入主线吧

@unicornx
Copy link
Contributor

还有个问题就是你改完代码后测试过吗?我指的是 doxygen 运行后的结果

我发现 lwp 这个 group 好像目前还没有定义过哎,你用 ingroup 感觉不会有什么效果啊

doxgen 的搭建和测试参考 RTT 仓库的 documentation/doxygen/readme.md

@unicornx unicornx added the Doc This PR/issue related with documents label Jan 17, 2025
@BernardXiong
Copy link
Member

为什么是针对lwp_dbg来添加doxygen?lwp_dbg模块并不完全重要,把lwp整体都梳理清晰更重要吧

So far, lwp component has not enough doxygen comment and community also has need.

Solution: Change Doxyfile for including component/lwp folder and add doxyzen comment for lwp_dbg.h.

Signed-off-by: 1078249029 <[email protected]>
@1078249029
Copy link
Contributor Author

为什么是针对lwp_dbg来添加doxygen?lwp_dbg模块并不完全重要,把lwp整体都梳理清晰更重要吧

我看9263的issue有需求的

@BernardXiong
Copy link
Member

为什么是针对lwp_dbg来添加doxygen?lwp_dbg模块并不完全重要,把lwp整体都梳理清晰更重要吧

我看9263的issue有需求的

那个也指的是lwp的整体doxygen注释。但建议在注释之前,是可以把lwp组件先梳理清晰,这样才会可以避免后续lwp的调整导致之前的doxygen注释又需要重新来过

@1078249029
Copy link
Contributor Author

为什么是针对lwp_dbg来添加doxygen?lwp_dbg模块并不完全重要,把lwp整体都梳理清晰更重要吧

我看9263的issue有需求的

那个也指的是lwp的整体doxygen注释。但建议在注释之前,是可以把lwp组件先梳理清晰,这样才会可以避免后续lwp的调整导致之前的doxygen注释又需要重新来过

您指的是先梳理lwp组件的源码?然后等接口固定下来后再补充doxygen注释?如果是这样的话这个issue是否可以先close,还是保持open?

@unicornx
Copy link
Contributor

unicornx commented Jan 19, 2025

那个也指的是lwp的整体doxygen注释。但建议在注释之前,是可以把lwp组件先梳理清晰,这样才会可以避免后续lwp的调整导致之前的doxygen注释又需要重新来过

您指的是先梳理lwp组件的源码?然后等接口固定下来后再补充doxygen注释?

我理解熊大指的不是梳理 lwp 组件的源码(源码应该不要动),我们要做的是基于现有 lwp 的代码把 doxygen 的框架层面的内容进行梳理,然后再写具体函数的 doxygen 文档。你可以看到现在连 LWP 的 group 都还没有定义好,整体的梳理包括定义更清晰的层次性的 group,这样以后才知道 debug 这部分加到哪个 group 里。否则像我举例的那样直接放在第一级的 lwp 里也许是不恰当的。

我建议对于 doxygen 的改进工作等 #9880 做完后,我们再梳理目前 group 的定义后再填充具体的内容。对于 group 的重新梳理,我的想法是一个一个 group 的梳理,每梳理一个还要同时整理这个 group 对应的 utest。也就是说 #9824#9775 这两项工作会同步协调开展。

@1078249029 当然对于 lwp 这部分的梳理你如果感兴趣的话也可以先展开,提一个 issue,我们在上面把你的设计讨论清楚再提 pr。

如果是这样的话这个issue是否可以先close,还是保持open?

我建议这个 pr 可以先 close。因为前提是要先梳理好 lwp 的 group 组织。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: lwp Component Doc This PR/issue related with documents RT-Smart RT-Thread Smart related PR or issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants