Skip to content

Conversation

@Thalley
Copy link
Contributor

@Thalley Thalley commented Nov 7, 2024

Add conversion macros from milliseconds to various units. The purpose of these macros is to make it more clear/easier for users to set and read values using milliseconds rather than the various BT units which may be in 0.625, 1.25 or 10ms units.

This is especially useful when comparing related values using different units, such as advertising interval (0.625ms units) and periodic advertising interval units (1.25ms units).

Users will have to be aware that these macros can provide slightly different values than what is provided, if the provided values do not match the units.

A followup PR for audio can be found here #81093 which uses more of these macros.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure where to put these. We already have various conversion functions like BT_GAP_PER_ADV_INTERVAL_TO_MS and BT_CONN_INTERVAL_TO_MS, but they are placed here and there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't sure whether to use INT or INTERVAL. We seem to be using both

Choose a reason for hiding this comment

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

Personally I'd prefer INTERVAL, it is more consistent with the previous macro BT_GAP_PER_ADV_INTERVAL_TO_MS. Not going to block this because at the end of the day INT also works

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer "interval".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to INTERVAL

@Thalley
Copy link
Contributor Author

Thalley commented Nov 11, 2024

What do the reviewers think of the prefix and location of these macros?

Should they be in gap and prefixed with BT_GAP like they are today, or should they be moved to the individual parts (e.g. adv => BT_ADV and BT_GAP_ISO_INTERVAL_TO_MS => BT_ISO_INTERVAL_TO_MS, etc.?

@Thalley Thalley force-pushed the bt_ms_to_unit_conv branch 2 times, most recently from 4bd2837 to fff387f Compare November 11, 2024 16:04
@Thalley
Copy link
Contributor Author

Thalley commented Nov 11, 2024

Added unit tests

Comment on lines +36 to +58
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't sure whether to use hex as expected (typical use is hex), or decimal. Let me know what you prefer

Choose a reason for hiding this comment

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

hex is nice

@kruithofa
Copy link

What do the reviewers think of the prefix and location of these macros?

Should they be in gap and prefixed with BT_GAP like they are today, or should they be moved to the individual parts (e.g. adv => BT_ADV and BT_GAP_ISO_INTERVAL_TO_MS => BT_ISO_INTERVAL_TO_MS, etc.?

I don't think we have too many conversion functions like this yet; anyways it is nice to have these grouped together, so I'd say keep them in gap

kruithofa
kruithofa previously approved these changes Nov 12, 2024
jhedberg
jhedberg previously approved these changes Nov 12, 2024
@Thalley Thalley dismissed stale reviews from jhedberg and kruithofa via 625a276 November 14, 2024 14:27
@Thalley
Copy link
Contributor Author

Thalley commented Nov 14, 2024

Added microsecond versions of the macros as well, as well as added the value ranges. Since we don't have the value ranges defined in MS/US, I opted to just add them as literal values instead of values that could be @refed.

alwa-nordic
alwa-nordic previously approved these changes Nov 14, 2024
Add conversion macros from milliseconds to various units.
The purpose of these macros is to make it more clear/easier
for users to set and read values using milliseconds rather
than the various BT units which may be in 0.625, 1.25 or 10ms
units.

This is especially useful when comparing related values using
different units, such as advertising interval (0.625ms units)
and periodic advertising interval units (1.25ms units).

Users will have to be aware that these macros can provide slightly
different values than what is provided, if the provided values
do not match the units.

Signed-off-by: Emil Gydesen <[email protected]>
@nashif nashif merged commit 8703381 into zephyrproject-rtos:main Nov 16, 2024
26 checks passed
@Thalley Thalley deleted the bt_ms_to_unit_conv branch November 16, 2024 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Bluetooth Audio area: Bluetooth Controller area: Bluetooth HCI Bluetooth HCI Driver area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Bluetooth Qualification Bluetooth Qualification -related issues and pull requests area: Bluetooth area: Samples Samples

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

7 participants