Skip to content

Bluetooth: ISO: Expand defines and their text #93007

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Thalley
Copy link
Contributor

@Thalley Thalley commented Jul 11, 2025

Add a few missing defines for limits used in ISO.
Add missing units and provide the value in both
hex and decimal (with units) for easier-to-read support.

@Thalley Thalley force-pushed the iso_defines_updates branch 2 times, most recently from 1f57250 to a5f51b3 Compare July 11, 2025 10:44
@Thalley Thalley requested a review from Copilot July 11, 2025 10:45
Copilot

This comment was marked as outdated.

@Thalley Thalley force-pushed the iso_defines_updates branch from a5f51b3 to b9af16a Compare July 11, 2025 11:13
@Thalley Thalley requested a review from Copilot July 11, 2025 11:14
Copilot

This comment was marked as outdated.

Add a few missing defines for limits used in ISO.
Add missing units and provide the value in both
hex and decimal (with units) for easier-to-read support.

Signed-off-by: Emil Gydesen <[email protected]>
@Thalley Thalley force-pushed the iso_defines_updates branch from b9af16a to e5689b7 Compare July 11, 2025 12:15
@Thalley Thalley requested a review from Copilot July 11, 2025 12:15
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances the ISO limits in iso.h by adding missing define constants and improving comment clarity with explicit units and decimal representations.

  • Added missing minimum retransmission defines (CONNECTED_RTN_MIN, BROADCAST_RTN_MIN) and standardized BROADCAST_CODE_SIZE.
  • Expanded inline comments to include both hex and decimal values with units.
  • Introduced new subinterval limit defines and detailed notes on interval dependencies.
Comments suppressed due to low confidence (5)

include/zephyr/bluetooth/iso.h:80

  • The numeric grouping uses periods (4.000.000) which can be misread as a decimal; switch to commas (4,000,000 µs) or no separators (4000000 µs) for clarity.
/** Maximum controller delay in microseconds (4.000.000 us) */

include/zephyr/bluetooth/iso.h:88

  • The comment implies 4 ms but the defined max (0x0C80 × 1.25 ms) equals 4000 ms (4 s). Update to "4,000 ms (4 s)" or simply "4 seconds" for accuracy.
/** Maximum ISO interval in units of 1.25 ms (4.000 ms) */

include/zephyr/bluetooth/iso.h:92

  • Similar to the ISO interval comment, this reads as 4 ms but 0x0FA0 = 4000 ms; revise to "4,000 ms" or "4 seconds" and add a space before the closing */.
/** Maximum latency value in milliseconds (4.000 ms)*/

include/zephyr/bluetooth/iso.h:128

  • The grouping and magnitude are unclear: 0x4000 × 10 ms = 163,840 ms. Change to "163,840 ms" or "163.84 s" for consistency.
/** Maximum BIG sync timeout value in units of 10 ms (163.840 ms) */

include/zephyr/bluetooth/iso.h:167

  • The decimal value is incorrect: 0x00009C3F = 39999 µs. Update the comment to "39,999 µs" (or "39.999 ms") to match the define.
 */

Copy link

@Thalley Thalley marked this pull request as ready for review July 11, 2025 13:51
@github-actions github-actions bot added area: Bluetooth area: Bluetooth ISO Bluetooth LE Isochronous Channels labels Jul 11, 2025
@Thalley
Copy link
Contributor Author

Thalley commented Jul 11, 2025

Unfortunately we don't really have a defined way for thousand separators (https://discord.com/channels/720317445772017664/884782246010175528/1393207293440426115), so went with . in this case.

#define BT_ISO_CONTROLLER_DELAY_MIN 0x000000
/** Maximum controller delay in microseconds (4 s) */
/** Maximum controller delay in microseconds (4.000.000 us) */
Copy link
Contributor

@cvinayak cvinayak Jul 14, 2025

Choose a reason for hiding this comment

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

Isn't this a number localization choice? Is there any guidelines documented @kartben ?

We have used 0.625 us, 1.25 ms 1250 us, 10000 microseconds etc. in public APIs.

BLUETOOTH CORE SPECIFICATION Version 6.1 | Vol 3, Part C, Appendix A Timers and Constants, mostly uses seconds and millisecond units in US English formats? (I may be wrong here on which localization that it is).

Suggested change
/** Maximum controller delay in microseconds (4.000.000 us) */
/** Maximum controller delay in microseconds (4000000 us) */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't this a number localization choice? Is there any guidelines documented @kartben ?

Asked about this on Discord as well (https://discord.com/channels/720317445772017664/884782246010175528/1393207293440426115), and there a few cases where the lack of defined formatting for large numbers can be an issue

BLUETOOTH CORE SPECIFICATION Version 6.1 | Vol 3, Part C, Appendix A Timers and Constants, mostly uses seconds and millisecond units in US English formats

They usually mix units in the HCI spec, e.g. they may give a value (in microseconds) a range of 400 ms to 4 s

Not sure if that is better or not, but I do like that when a value is in a specific unit (e.g. microseconds), then the range is also expressed in values of that unit, instead of mixing both microseconds and seconds. Some times the BT Core spec also uses 10 ms (centisecond) as the unit, but due to how rarely that unit is used, they always write "10 ms" instead, and arguably we should too, but I don't have a strong opinion here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another thing I was also considering was to the decimal, instead of hexadecimal for the #defines, so that the comment in most cases didn't have to specify the value in a human-readable format, but I also think it's nice that the values are defined the same way as they are in the BT HCI spec (in hexadecimal).

For example,

/** Maximum latency value in milliseconds (4.000 ms)*/
#define BT_ISO_LATENCY_MAX          0x0FA0

could also have been

/** Maximum latency value in milliseconds (0xFA0 ms)*/
#define BT_ISO_LATENCY_MAX          4000U

@Thalley Thalley requested a review from kartben July 14, 2025 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Bluetooth ISO Bluetooth LE Isochronous Channels area: Bluetooth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants