Bluetooth: Host: Improve documentation of bt_le_ext_adv_start_param
#87358
Conversation
zephyrbot
added
area: Bluetooth Host
zephyrbot
requested review from
cvinayak,
hermabe,
jhedberg,
rugeGerritsen,
sjanc,
Thalley and
theob-pro
include/zephyr/bluetooth/bluetooth.h
Outdated
| * | ||
| * When using high duty cycle directed connectable advertising then | ||
| * this parameters must be set to a non-zero value less than or equal | ||
| * to the maximum of @ref BT_GAP_ADV_HIGH_DUTY_CYCLE_MAX_TIMEOUT. | ||
| * | ||
| * If privacy @kconfig{CONFIG_BT_PRIVACY} is enabled then the timeout | ||
| * must be less than @kconfig{CONFIG_BT_RPA_TIMEOUT}. | ||
| * | ||
| * See Core 4.E.7.8.56 parameter "Duration" for background information. |
There was a problem hiding this comment.
Please spell out specific version and other details. This helps in locating the correct thing.
| * See Core 4.E.7.8.56 parameter "Duration" for background information. | |
| * See parameter "Duration" in Core Specification Version 5.4 Vol. 4 Part E, Section 7.8.56, for background information. |
There was a problem hiding this comment.
Agree (and please use version 6.0 instead of 5.4)
There was a problem hiding this comment.
In this file we refer to 5.4, so I suggested 5.4, generally core spec section numbers do not change across revisions.
There was a problem hiding this comment.
Hmm, I always thought we would refer to the newest version of the spec available when adding the reference. We don't have any guidelines on this though
There was a problem hiding this comment.
Ok. It does not matter that much because section numbers don't change in core specifications across versions. Just FYI this may not be true for other profiles and spec (certainly not true for Mesh).
There was a problem hiding this comment.
@Thalley will it be worth creatin an issue regarding this and using that issue as basis to update coding guidlines to specifically state this?
There was a problem hiding this comment.
Just FYI this may not be true for other profiles and spec (certainly not true for Mesh).
Yeah, I've noticed, which is also why it's important to provide the version number :) Arguably we could omit it for the BT Core spec, as it doesn't change, but there still needs to be a minimum version where it was added
include/zephyr/bluetooth/bluetooth.h
Outdated
| * This callback is invoked when the limit set in @ref | ||
| * bt_le_ext_adv_start_param.timeout or @ref | ||
| * bt_le_ext_adv_start_param.num_events is reached. |
There was a problem hiding this comment.
| * This callback is invoked when the limit set in @ref | |
| * bt_le_ext_adv_start_param.timeout or @ref | |
| * bt_le_ext_adv_start_param.num_events is reached. | |
| * This callback is invoked when the limit set in | |
| * @ref bt_le_ext_adv_start_param.timeout or | |
| * @ref bt_le_ext_adv_start_param.num_events is reached. |
Not a blocker, but I do prefer having the @ref on the same line as the value it references
There was a problem hiding this comment.
Good point. I will fix it.
I missed that because I used a generic line re-wrap tool. Wrapping text manually is not fun, so I think we should think about how this can be automated. Ideally clang-format or another tool would take care of formatting Doxygen too. But I have not seen anything that can do this job. That's why I think we should maybe start using the #foo instead of @ref foo, so that there is no white space. At least it's a point in favor of it.
There was a problem hiding this comment.
That's why I think we should maybe start using the #foo instead of @ref foo, so that there is no white space. At least it's a point in favor of it.
I wouldn't be opposed to that, given the reasoning
There was a problem hiding this comment.
If you want to change style of referencing etc, it will be good to do that as a global change in seperate PR.
include/zephyr/bluetooth/bluetooth.h
Outdated
| * | ||
| * When using high duty cycle directed connectable advertising then | ||
| * this parameters must be set to a non-zero value less than or equal | ||
| * to the maximum of @ref BT_GAP_ADV_HIGH_DUTY_CYCLE_MAX_TIMEOUT. | ||
| * | ||
| * If privacy @kconfig{CONFIG_BT_PRIVACY} is enabled then the timeout | ||
| * must be less than @kconfig{CONFIG_BT_RPA_TIMEOUT}. | ||
| * | ||
| * See Core 4.E.7.8.56 parameter "Duration" for background information. |
There was a problem hiding this comment.
Agree (and please use version 6.0 instead of 5.4)
alwa-nordic
force-pushed
the
adv-lim-doc
branch
2 times, most recently
from
e0565f5 to
a9a4ff9
Compare
|
I have applied all suggestions. The second push is a rebase on main. |
This clarifies the documentation of `bt_le_ext_adv_start_param` and `bt_le_ext_adv_cb.sent`, and links them by via references. Signed-off-by: Aleksander Wasaznik <aleksander.wasaznik@nordicsemi.no>
alwa-nordic
dismissed stale reviews from Thalley, omkar3141, and jhedberg
via
1d3a297
alwa-nordic
force-pushed
the
adv-lim-doc
branch
from
a9a4ff9 to
1d3a297
Compare
|
I had to update the documentation after I noticed the following section:
|
| * The advertising set can either have been stopped by a timeout or | ||
| * because the specified number of advertising events has been reached. |
There was a problem hiding this comment.
I read this as the only two reasons for this callback. But, I'm not sure after looking at the code. It looks like it might also be called when the advertising set spawns a connection. @jhedberg Do you know?
There was a problem hiding this comment.
Not off the top of my head, unfortunately. Sounds like a reasonable assumption, though.
This clarifies the documentation of
bt_le_ext_adv_start_paramandbt_le_ext_adv_cb.sent, and links them by via references.