Bluetooth: ISO: Introduce bt_iso_chan_send_ts #68611
Conversation
Thalley
force-pushed
the
iso_ts_send_fix
branch
2 times, most recently
from
57950cc to
770bf50
Compare
zephyrbot
added
area: Samples
zephyrbot
requested review from
Casper-Bonde-Bose,
MariuszSkamra,
Vudentz,
asbjornsabo,
fredrikdanebjer,
hermabe,
jhedberg,
kruithofa,
larsgk,
pin-zephyr and
sjanc
|
For reviewers: The alternative would have been something like bt_iso_chan_send(struct bt_iso_chan *chan, struct net_buf *buf, uint16_t seq_num,
uint32_t ts, bool has_ts) |
The bt_iso_chan_send function could take an optional timestamp by using 0 as an indicator. The issue with this approach was that a timestamp value of 0 is valid, and could cause potential issue with syncing streams in a group. To fully support transmitting with and without timestamp, bt_iso_chan_send_ts has been introduced, which is the only function of the two (bt_iso_chan_send being the other) that supports timestamps. A new function, rather than adding a boolean to the existing, was chosen as it simplifies the individual functions as well as making it more explicit what the function does. Since the bt_iso_chan_send function is used by LE audio, both the BAP and CAP send functions have similarly been updated. Likewise, all tests and samples have been updated to use the updated function(s), and BT_ISO_TIMESTAMP_NONE has been removed. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
Thalley
force-pushed
the
iso_ts_send_fix
branch
from
770bf50 to
58a96f1
Compare
| */ | ||
| int bt_iso_chan_send(struct bt_iso_chan *chan, struct net_buf *buf, | ||
| uint16_t seq_num, uint32_t ts); | ||
| int bt_iso_chan_send_ts(struct bt_iso_chan *chan, struct net_buf *buf, uint16_t seq_num, |
There was a problem hiding this comment.
I would advise that we consider the naming.
As it stands now, there is a risk that developers will find the vanilla bt_iso_chan_send and not be aware of the huge benefits bt_iso_chan_send_ts has.
Suggest:
bt_iso_chan_send_no_ts or bt_iso_chan_send_ts_none along side bt_iso_chan_send_ts.
There was a problem hiding this comment.
Hmm, I see your point, although it seems weird to mention what is omitted in the function name. This would be similar to having a negated flag - for example the HCI ISO data packet sets a flag if the TS is included. Your suggestion would be similar to setting a flag when it was not included.
I suggest that we keep it as is for 2 reasons:
- None of our samples or tests have been using TS, so it's clearly not widely used (and not the default)
- Omitting
tsfrom the non-TS version is more aligned with the core spec in terms of setting the flag when it is there
koffes
left a comment
koffes
left a comment
There was a problem hiding this comment.
Only a comment/question on naming.
|
@fabiobaltieri Should this not have the 3.6 milestone? It fixes a bug |
Yeah probably, IIRC I was batch triaging, read the title "introduce XXXX", sounded like a feature and hit the 3.7 tag -- but it still got your attention so in the end it worked out :-) Feel free to tag these yourself, I periodically go through approved+untagged and take a guess or ask but if you tag them for us it's going to make everyone's life easier. |
|
@jhedberg Please review. |
That is because this is (mainly) ISO, which is under the host directory, and not Audio specific :) |
Last activity: 18 months ago (review)
Recent activities:
- review: Review on 'Bluetooth: Audio: Shell: Fix build errors for USB=n'
zephyrproject-rtos#72733
(2024-05-16T06:13:15Z)
- review: Review on 'Bluetooth: ISO: Introduce bt_iso_chan_send_ts'
zephyrproject-rtos#68611
(2024-02-07T07:53:39Z)
- review: Review on 'Bluetooth: BAP: Fix issue with setting invalid iso
data path'
zephyrproject-rtos#67835
(2024-01-22T10:15:03Z)
Signed-off-by: Benjamin Cabé <benjamin@zephyrproject.org>
11 participants
The bt_iso_chan_send function could take an optional timestamp by using 0 as an indicator. The issue with this approach was that a timestamp value of 0 is valid, and could cause potential issue with syncing streams in a group.
To fully support transmitting with and without timestamp, bt_iso_chan_send_ts has been introduced, which is the only function of the two (bt_iso_chan_send being the other) that supports timestamps.
A new function, rather than adding a boolean to the existing, was chosen as it simplifies the individual functions as well as making it more explicit what the function does.
Since the bt_iso_chan_send function is used by LE audio, both the BAP and CAP send functions have similarly been updated. Likewise, all tests and samples have been updated to use the updated function(s), and BT_ISO_TIMESTAMP_NONE has been removed.
Fixes #68293