Bluetooth: Audio: Fix bad docs for codec helper functions #81799

Thalley · 2024-11-22T23:28:24Z

Several of the codec helper functions used @RetVal incorrectly Several functions referenced @ccid_list incorrectly

kartben · 2024-11-23T00:34:39Z

include/zephyr/bluetooth/audio/audio.h

@@ -845,9 +845,9 @@ int bt_audio_codec_cfg_freq_hz_to_freq(uint32_t freq_hz);
 * @param codec_cfg The codec configuration to extract data from.
 *
 * @retval A @ref bt_audio_codec_cfg_freq value


Missed this one

kartben · 2024-11-23T00:36:18Z

include/zephyr/bluetooth/audio/audio.h

@@ -890,9 +890,9 @@ int bt_audio_codec_cfg_frame_dur_us_to_frame_dur(uint32_t frame_dur_us);
 * @param codec_cfg The codec configuration to extract data from.
 *
 * @retval A @ref bt_audio_codec_cfg_frame_dur value


kartben · 2024-11-23T00:41:47Z

include/zephyr/bluetooth/audio/audio.h

- * @retval -EINVAL if arguments are invalid.
- * @retval The converted frequency value in Hz.
+ * @retval -EINVAL Arguments are invalid.
+ * @retval frequency The converted frequency value in Hz.


You want this one listed first I think, i.e. the most likely return value first (applies throughout the PR)

Related, and since we don't have guidelines just yet and I very much value your thoughts on this topic and the fact we can use this PR as a way to settle on best practices for documenting return values, how about the "non-distinctive" value be described using @return? Basically, this: https://docs.rtems.org/branches/master/eng/coding-doxygen.html#function-declarations

So I take it you're not in favor of using @return then? Or maybe there was too much info in my comment and you missed that part :)

Suggested change

* @retval frequency The converted frequency value in Hz.

* @return The converted frequency value in Hz.

how about the "non-distinctive" value be described using @return

So this is suggesting to use @retval for specific values, and @return for values that may range?

So in this example this would be something like

* @retval -EINVAL Arguments are invalid. * * @return The converted frequency value in Hz.

I'm not opposed to it, depending on how it would render in the built docs :) It does make sense to only use @retval for specific values. Speaking of, should we use - @ref EINVAL? or add something like a @errno{EINVAL}?

I think it makes sense to propose an RFC to discuss this

Speaking of, should we use - @ref EINVAL? or add something like a @errno{EINVAL}?

Good question. Given that retval accepts ONE token representing the return value, the former likely won't work, and not sure about the latter either. That being said, I like the idea as making it easier to navigate to the list of available errnos can only be goodness.

+1 to address the next iteration (combining usage of @return and @retval) in a follow-up RFC

kartben · 2024-11-25T10:43:23Z

include/zephyr/bluetooth/audio/audio.h

- * @retval -EINVAL if arguments are invalid.
- * @retval The converted frequency value in Hz.
+ * @retval -EINVAL Arguments are invalid.
+ * @retval frequency The converted frequency value in Hz.


Speaking of, should we use - @ref EINVAL? or add something like a @errno{EINVAL}?

Good question. Given that retval accepts ONE token representing the return value, the former likely won't work, and not sure about the latter either. That being said, I like the idea as making it easier to navigate to the list of available errnos can only be goodness.

+1 to address the next iteration (combining usage of @return and @retval) in a follow-up RFC

kruithofa

LGTM, just some nitpicks

kruithofa · 2024-11-25T11:53:35Z

include/zephyr/bluetooth/audio/audio.h

@@ -869,8 +869,8 @@ int bt_audio_codec_cfg_set_freq(struct bt_audio_codec_cfg *codec_cfg,
 *
 * @param frame_dur The assigned numbers frame duration to convert.
 *
- * @retval -EINVAL if arguments are invalid.
- * @retval The converted frame duration value in microseconds.
+ * @retval duraction The converted frame duration value in microseconds.


Suggested change

* @retval duraction The converted frame duration value in microseconds.

* @retval duration The converted frame duration value in microseconds.

kruithofa · 2024-11-25T11:55:44Z

include/zephyr/bluetooth/audio/audio.h

- * @retval -ENOMEM if the new value could not set or added due to memory
+ * @retval data_len The data_len of @p codec_cfg on success
+ * @retval -EINVAL Arguments are invalid
+ * @retval -ENOMEM The new value could not set or added due to memory


Suggested change

* @retval -ENOMEM The new value could not set or added due to memory

* @retval -ENOMEM The new value could not be set or added due to lack of memory

Same text change for the other places where -ENOMEM is returned

kruithofa · 2024-11-25T12:00:40Z

include/zephyr/bluetooth/audio/audio.h

@@ -777,8 +777,8 @@ int bt_audio_data_parse(const uint8_t ltv[], size_t size,
 *                  Any found data will be little endian.
 *
 * @retval Length The length of found @p data (may be 0).


Maybe not part of this PR, but using a lowercase l is consistent with the parameter description in the other
fucntions

Suggested change

* @retval Length The length of found @p data (may be 0).

* @retval length The length of found @p data (may be 0).

kruithofa · 2024-11-25T12:03:14Z

include/zephyr/bluetooth/audio/audio.h

- * @retval The data_len of @p codec_cfg on success
- * @retval -EINVAL if arguments are invalid
- * @retval -ENOMEM if the new value could not set or added due to memory
+ * @retval data_len The data_len of @p codec_cfg on success


In some other functions where the return value is len the description says length of ..., maybe the same should be done for data_len and meta_len, here and in other functions

Suggested change

* @retval data_len The data_len of @p codec_cfg on success

* @retval data_len The data length of @p codec_cfg on success

Hmm, I get your point. data_len length here refer to the data_len field of codec_cfg. Will rewrite a bit

+1, this too caught my eye fwiw :)

kruithofa

One nitpick, but nothing blocking

kruithofa · 2024-11-27T08:55:30Z

include/zephyr/bluetooth/audio/audio.h

- * @retval The meta_len of @p codec_cfg on success
- * @retval -EINVAL if arguments are invalid
- * @retval -ENOMEM if the new value could not set or added due to memory
+ * @retval meta_len The meta_len of @p codec_cfg on success


Shall we do the same as for the data_len field of codec_cfg?

Suggested change

* @retval meta_len The meta_len of @p codec_cfg on success

* @retval meta_len The @p codec_cfg.meta_len on success

Good catch, will update

kruithofa · 2024-11-27T08:56:30Z

include/zephyr/bluetooth/audio/audio.h

@@ -1099,8 +1099,8 @@ int bt_audio_codec_cfg_meta_set_val(struct bt_audio_codec_cfg *codec_cfg,
 * @param codec_cfg  The codec data to set the value in.
 * @param type       The type id to unset.
 *
- * @retval The meta_len of @p codec_cfg on success
- * @retval -EINVAL if arguments are invalid
+ * @retval meta_len The meta_len of @p codec_cfg on success


Suggested change

* @retval meta_len The meta_len of @p codec_cfg on success

* @retval meta_len The @p codec_cfg.meta_len on success

@RetVal

Several of the codec helper functions used @RetVal incorrectly Several functions referenced @ccid_list incorrectly Signed-off-by: Emil Gydesen <[email protected]>

Thalley self-assigned this Nov 22, 2024

kartben requested changes Nov 23, 2024

View reviewed changes

Thalley marked this pull request as ready for review November 25, 2024 10:05

Thalley force-pushed the codec_doc_fixes branch from db24524 to 45b37e9 Compare November 25, 2024 10:05

zephyrbot added area: Bluetooth Audio area: Bluetooth labels Nov 25, 2024

zephyrbot requested review from asbjornsabo, babrsn, Casper-Bonde-Bose, fredrikdanebjer, jhedberg, jthm-ot, kruithofa, larsgk, MariuszSkamra, niym-ot, pin-zephyr and sjanc November 25, 2024 10:05

Thalley requested a review from kartben November 25, 2024 10:12

kartben previously approved these changes Nov 25, 2024

View reviewed changes

kruithofa reviewed Nov 25, 2024

View reviewed changes

Thalley dismissed kartben’s stale review via 8cef66b November 25, 2024 16:36

Thalley force-pushed the codec_doc_fixes branch from 45b37e9 to 8cef66b Compare November 25, 2024 16:36

Thalley requested review from kartben and kruithofa November 25, 2024 16:36

Thalley force-pushed the codec_doc_fixes branch from 8cef66b to 8d3e70e Compare November 25, 2024 20:25

kruithofa previously approved these changes Nov 27, 2024

View reviewed changes

Bluetooth: Audio: Fix bad docs for codec helper functions

d4c6ee4

Several of the codec helper functions used @RetVal incorrectly Several functions referenced @ccid_list incorrectly Signed-off-by: Emil Gydesen <[email protected]>

Thalley dismissed kruithofa’s stale review via d4c6ee4 November 27, 2024 18:26

Thalley force-pushed the codec_doc_fixes branch from 8d3e70e to d4c6ee4 Compare November 27, 2024 18:26

Thalley requested a review from kruithofa November 27, 2024 18:26

kruithofa approved these changes Nov 28, 2024

View reviewed changes

kartben approved these changes Nov 28, 2024

View reviewed changes

kartben merged commit f254280 into zephyrproject-rtos:main Nov 28, 2024
24 checks passed

Thalley deleted the codec_doc_fixes branch November 28, 2024 12:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bluetooth: Audio: Fix bad docs for codec helper functions #81799

Bluetooth: Audio: Fix bad docs for codec helper functions #81799

Thalley commented Nov 22, 2024

kartben Nov 23, 2024

kartben Nov 23, 2024

kartben Nov 23, 2024 •

edited

Loading

kartben Nov 25, 2024

Thalley Nov 25, 2024

kartben Nov 25, 2024

kartben Nov 25, 2024

kruithofa left a comment

kruithofa Nov 25, 2024

kruithofa Nov 25, 2024

kruithofa Nov 25, 2024

kruithofa Nov 25, 2024

Thalley Nov 25, 2024

kartben Nov 25, 2024

kruithofa left a comment

kruithofa Nov 27, 2024

Thalley Nov 27, 2024

kruithofa Nov 27, 2024

	* @retval frequency The converted frequency value in Hz.
	* @return The converted frequency value in Hz.

	* @retval duraction The converted frame duration value in microseconds.
	* @retval duration The converted frame duration value in microseconds.

	* @retval -ENOMEM The new value could not set or added due to memory
	* @retval -ENOMEM The new value could not be set or added due to lack of memory

	* @retval Length The length of found @p data (may be 0).
	* @retval length The length of found @p data (may be 0).

	* @retval data_len The data_len of @p codec_cfg on success
	* @retval data_len The data length of @p codec_cfg on success

	* @retval meta_len The meta_len of @p codec_cfg on success
	* @retval meta_len The @p codec_cfg.meta_len on success

Bluetooth: Audio: Fix bad docs for codec helper functions #81799

Bluetooth: Audio: Fix bad docs for codec helper functions #81799

Conversation

Thalley commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartben Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kruithofa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kruithofa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartben Nov 23, 2024 •

edited

Loading