Skip to content

[doxygen][audio] Fix doxygen comments for audio component #10073

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 1 commit into from
Mar 6, 2025

Conversation

1078249029
Copy link
Contributor

Added comments for data structure and rt_audio_ops in dev_audio.h. Enriched comments for macro group. Moved and renamed folder audio.

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

[

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

#10065

你的解决方案是什么 (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自查

@1078249029
Copy link
Contributor Author

@unicornx 我这里好像不能添加reviewer,只能pin一下了

@unicornx unicornx self-requested a review March 4, 2025 23:58
Copy link
Contributor

@unicornx unicornx left a comment

Choose a reason for hiding this comment

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

commit message 一行不要太长,80 列左右就可以回车换行了。

Copy link
Contributor

Choose a reason for hiding this comment

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

文字和 ”*/“ 之间要加空格,譬如下面 ”audio_control“ 后面缺个空格:

/** @} */ /* End of audio_control*/

这样的地方有好几处,请搜索并改正。

rt_err_t (*start)(struct rt_audio_device *audio, int stream); /**< Turn on the audio device */
rt_err_t (*stop)(struct rt_audio_device *audio, int stream); /**< Turn off the audio device */
rt_ssize_t (*transmit)(struct rt_audio_device *audio, const void *writeBuf, void *readBuf, rt_size_t size); /**< Transmit data between application and device */
void (*buffer_info)(struct rt_audio_device *audio, struct rt_audio_buf_info *info); /**< Get page size of codec or private buffer's info */
Copy link
Contributor

@unicornx unicornx Mar 5, 2025

Choose a reason for hiding this comment

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

这里放在后面感觉一行太长了,代码里我们一般都有对一行限制长度的要求。

原来对结构体成员的 doxygen 注释没有考虑到 callback 的情况,普通结构体成员一般不会那么长。

我建议改良一下,我试了一下,可以将注释移到 callback 成员的前面去,你看看还行,例子如下, 可以写 @brief ,也可以不写, 需要注意的是,如果不写在行尾,则不要写成 /**< , 即不要 <

/**
 * @brief Aduio device operators
 */
struct rt_audio_ops
{
    /** @brief Get audio capabilities information */
    rt_err_t (*getcaps)(struct rt_audio_device *audio, struct rt_audio_caps *caps); 
    
    /** Configure audio devices */
    rt_err_t (*configure)(struct rt_audio_device *audio, struct rt_audio_caps *caps);

    // ......
};

更近一步,对于这种成员是回调函数的结构体,我发现可以写成下面这样,从而可以对 callback 函数做更详细的说明,你看看要不要写成这样,只是写法比较复杂,内容比较多。主要是考虑到可能需要对 callback 函数的参数和返回值等进行更多的描述时会有用。

/**
 * @brief Aduio device operators
 */
struct rt_audio_ops
{
    /** @brief Get audio capabilities information
     *  @param audio the audio device
     *  @param caps the audio capabilities information
     *  @return the operation status, RT_EOK on successful
     */
    rt_err_t (*getcaps)(struct rt_audio_device *audio, struct rt_audio_caps *caps); 
    //......
};

你看看怎么写。定下来我也去更新一下 https://rt-thread.github.io/rt-thread/page_howto_struct.html

rt_uint16_t block_count; /**< Audio block_count information for replay function */
rt_uint32_t total_size; /**< Audio total_size which is equal to block_size multiplying
block_count information for replay function */

Copy link
Contributor

Choose a reason for hiding this comment

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

行尾的注释对齐一下会更好看。

@1078249029 1078249029 force-pushed the driver_doxygen_audio_update branch 2 times, most recently from 56c3f36 to 8a5af9a Compare March 5, 2025 11:41
@1078249029 1078249029 requested a review from unicornx March 5, 2025 11:53
Copy link
Contributor

@unicornx unicornx left a comment

Choose a reason for hiding this comment

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

就把下面提的对齐问题处理一下吧,其他没有了,你再改一下,我就 approve 了。

宏分组的方法挺好的,以后值得推广。我会提个 pr 补充到 how to 里面去

callback 的写法我也会另外提个 pr 补充到 howto 里面去。

Thanks,
Chen

rt_uint8_t *buffer; /**< Audio buffer information */
rt_uint16_t block_size; /**< Audio block_size information for replay function */
rt_uint16_t block_count; /**< Audio block_count information for replay function */
rt_uint32_t total_size; /**< Audio total_size which is equal to block_size multiplying
Copy link
Contributor

Choose a reason for hiding this comment

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

这些行尾注释请对齐。就像前面对宏的那样。

rt_uint16_t samplebits;
rt_uint32_t samplerate; /**< Audio samplerate information */
rt_uint16_t channels; /**< Audio channels information */
rt_uint16_t samplebits; /**< Audio samplebits information */
Copy link
Contributor

Choose a reason for hiding this comment

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

这些行尾注释请对齐。就像前面对宏的那样。

int main_type;
int sub_type;
int main_type; /**< Audio main type, one value of @ref audio_type */
int sub_type; /**< Audio sub type, one value of @ref audio_dsp @ref audio_mixer */
Copy link
Contributor

Choose a reason for hiding this comment

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

这些行尾注释请对齐。就像前面对宏的那样。

} udata;
rt_uint32_t mask; /**< Capabilities mask */
int value; /**< Capabilities value */
struct rt_audio_configure config; /**< Audio samplebits information */
Copy link
Contributor

Choose a reason for hiding this comment

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

这些行尾注释请对齐。就像前面对宏的那样。

of replay in the blocks which is currently being played */
rt_uint32_t pos; /**< Global position of audio replay */
rt_uint8_t event; /**< Event flag */
rt_bool_t activated; /**< Activaty flag */
};
Copy link
Contributor

@unicornx unicornx Mar 6, 2025

Choose a reason for hiding this comment

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

这些行尾注释请对齐。就像前面对宏的那样。尾巴其实不用对齐,前面对齐就好:
可以用 TAB 键,然后保存时 TAB 转空格即可。貌似 RTT 这里缩进不用 TAB 的,和 linux 不一样。

struct rt_audio_replay
{
    struct rt_mempool *mp;      /**< Memory pool for audio replay */
    struct rt_data_queue queue; /**< Replay data queue */
    struct rt_mutex lock;       /**< Replay mutex lock */ 
    ....
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这些行尾注释请对齐。就像前面对宏的那样。尾巴其实不用对齐,前面对齐就好: 可以用 TAB 键,然后保存时 TAB 转空格即可。貌似 RTT 这里缩进不用 TAB 的,和 linux 不一样。

struct rt_audio_replay
{
    struct rt_mempool *mp;      /**< Memory pool for audio replay */
    struct rt_data_queue queue; /**< Replay data queue */
    struct rt_mutex lock;       /**< Replay mutex lock */ 
    ....
};

啊,是我理解错了。这种结构体成员长短不一的情况在 https://github.com/RT-Thread/rt-thread/blob/master/documentation/0.doxygen/example/include/struct.h 并没有说明,还是补充了吧?

Copy link
Contributor

Choose a reason for hiding this comment

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

啊,是我理解错了。这种结构体成员长短不一的情况在 https://github.com/RT-Thread/rt-thread/blob/master/documentation/0.doxygen/example/include/struct.h 并没有说明,还是补充了吧?

ok, 我另外提个 pr 来补充说明一下。

struct rt_device parent; /**< Audio device parents */
struct rt_audio_ops *ops; /**< Audio device operator */
struct rt_audio_replay *replay; /**< Pointer to audio replay structure */
struct rt_audio_record *record; /**< Pointer to audio record structure */
Copy link
Contributor

Choose a reason for hiding this comment

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

这些行尾注释请对齐。就像前面对宏的那样。

@1078249029 1078249029 force-pushed the driver_doxygen_audio_update branch from 8a5af9a to 2205281 Compare March 6, 2025 01:27
@unicornx
Copy link
Contributor

unicornx commented Mar 6, 2025

我发现有个地方可以改进一下,代码会简洁一些,

以下面代码为例:

/**
 * @defgroup audio_control AUDIO_CTL
 *
 * @brief Control audio device.
 */

/**
 * @addtogroup audio_control
 * @{
 */

/**
 * @brief Generate audio command code with @a a
 *
 * @param[in] a offset of command.
 *
 * @return audio device control command code.
 */
#define _AUDIO_CTL(a) (RT_DEVICE_CTRL_BASE(Sound) + a)

#define AUDIO_CTL_GETCAPS                   _AUDIO_CTL(1) /**< Get audio device capabilities */
#define AUDIO_CTL_CONFIGURE                 _AUDIO_CTL(2) /**< Get audio device configuration */
#define AUDIO_CTL_START                     _AUDIO_CTL(3) /**< Start audio device  */
#define AUDIO_CTL_STOP                      _AUDIO_CTL(4) /**< Stop audio device */
#define AUDIO_CTL_GETBUFFERINFO             _AUDIO_CTL(5) /**< Get audio device buffer information */

/** @} */ /* End of audio_control */

这里 defgroup 了一个 audio_control 然后通过 addtogroup 将一些宏组织到一起。但其实可以省掉 addtogroup ,把 ”@{“ 直接写到 defgroup 里,这样代码会更好看。建议改进一下。
例子如下:

/**
 * @defgroup audio_control AUDIO_CTL
 *
 * @brief Control audio device.
 * 
 * @{
 */

/**
 * @brief Generate audio command code with @a a
 *
 * @param[in] a offset of command.
 *
 * @return audio device control command code.
 */
#define _AUDIO_CTL(a) (RT_DEVICE_CTRL_BASE(Sound) + a)

#define AUDIO_CTL_GETCAPS                   _AUDIO_CTL(1) /**< Get audio device capabilities */
#define AUDIO_CTL_CONFIGURE                 _AUDIO_CTL(2) /**< Get audio device configuration */
#define AUDIO_CTL_START                     _AUDIO_CTL(3) /**< Start audio device  */
#define AUDIO_CTL_STOP                      _AUDIO_CTL(4) /**< Stop audio device */
#define AUDIO_CTL_GETBUFFERINFO             _AUDIO_CTL(5) /**< Get audio device buffer information */

/** @} */ /* End of audio_control */

@1078249029 1078249029 requested a review from unicornx March 6, 2025 02:20
Added comments for data structure and rt_audio_ops in dev_audio.h.
Enriched comments for macro group. Moved and renamed folder audio.

Signed-off-by: 1078249029 <[email protected]>
@1078249029 1078249029 force-pushed the driver_doxygen_audio_update branch from 2205281 to c7dbe39 Compare March 6, 2025 03:06
@unicornx unicornx requested a review from Rbb666 March 6, 2025 03:21
@unicornx
Copy link
Contributor

unicornx commented Mar 6, 2025

@Rbb666 我已经 approve 了,你看看没问题请 merge,thanks

@Rbb666 Rbb666 merged commit ed8d0bc into RT-Thread:master Mar 6, 2025
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants