Bluetooth: ISO: Add additional information for ISO streams #45077

Thalley · 2022-04-22T16:17:32Z

Add all information from the ISO established events
and provide the information in the get_info function.

The use cases of each field heavily depends on what
the ISO streams are used for.

Most, if not all, of the field can be used by the
higher layers to improve quality and/or reliability
of e.g. audio streams that use ISO.

Signed-off-by: Emil Gydesen [email protected]

fixes #44546

Thalley · 2022-04-22T16:22:15Z

include/zephyr/bluetooth/iso.h

+	/** The transport PHY  */
+	uint8_t  phy;
+
+	/** The burst number */
+	uint8_t  bn;
+
+	/** The maximum PDU size in octets */
+	uint16_t max_pdu;


PHY and PDU size is already stored as part of the struct bt_iso_chan_io_qos struct. Should we omit them from these structs?

I like that you get all the values from the established event in the same structure, hence I prefer this implementation. But do agree that it could be confusing that you have the value two places.

Thalley · 2022-04-22T16:23:11Z

include/zephyr/bluetooth/iso.h

+	uint16_t interval;
+};
+
+struct bt_iso_sync_receiver_info {


This is the same as bt_iso_broadcaster_info just without the sync_delay. Should we use the same struct for both, and then just document that sync_delay is for the broadcaster only?

From an application data path point of view there is no difference between a broadcast and a unicast. Perhaps it would make sense to have the data common between unicast and broadcast in one structure (BN, PHY, max_pdu, number_of_subevents and interval) to have a generic data path handler - but there is also value in having simple structures with only valid members. Even if the documentation mentions a value is only valid in some cases a developer might miss that - especially during maintenance where you only modify the code.

The difference between unicast and broadcast is taht unicast may have 2 pairs of latency, flush timeout, etc. as it can be bidirectional.

Thalley · 2022-04-22T16:24:07Z

include/zephyr/bluetooth/iso.h

+	/** The flush timeout in ms */
+	uint32_t flush_timeout;


The flush timeout is typically given as a value in units of iso_interval, but thought it made more sense to provide the value as a unit of MS by doing iso_interval * flush_timeout

If we supply the value in time, I don't think ms is sufficient unless it is a float. 7.5ms is a standard iso-interval and that cannot be expressed in units of ms - except for even FT :-)
Unit could be micro seconds.

The units from the event is 1.25ms. Using microseconds would unfortunately require a 64-bit value to hold this value (max FT * max iso_interval > 2^32-1).

Change to use unit of 1.25ms (we do that for many other interval issues).

Thalley · 2022-04-22T16:25:04Z

include/zephyr/bluetooth/iso.h

+	/** The ISO interval in ms */
+	uint16_t interval;


Values such as PHY, BN and interval are common between the 3 structs. Should we move them out of these and store them directly in struct bt_iso_info?

I like that - then you always have the values available where you need them (in the data path) and don't need for each application to implement it's own mapping from connection instance to connection parameters.
But again the unit of interval shall support x.25ms and x.5ms hence perhaps use micro seconds?

See #45077 (comment)

I move the NSE and interval to the outer struct, but the remaining fields are not easily moved. While all have e.g. max_pdu, the unicast struct have 2 values for that (given that it can be bidirectional).

The unicast and broadcast data path values (PDU, bn, latency, etc.) are also not the same, as e.g. broadcast has PTO and unicast has flush timeout

Casper-Bonde-Bose · 2022-04-25T09:26:31Z

subsys/bluetooth/host/iso.c

+		central->phy = evt->c_phy;
+		central->latency = sys_get_le16(evt->c_latency);


I think PHY is relevant even if c_max_pdu is 0. For example the phy in use is relevant to know when evaluating the link budget - you might need to increase TX power if a 2 mbit link is used for ACK/NACK, which might not be needed for a 1mbit link.
I expect a typical unidirectional stream will use 2mbit for upstream (audio data) whereas downstream (ack's) should be 1mbit to have robust acks/nack transfers.

Right. I'll just leave the values as they come from the event then :)

Casper-Bonde-Bose · 2022-04-25T09:31:26Z

subsys/bluetooth/host/iso.c

+	info->irc = evt->irc;
+	info->interval = sys_le16_to_cpu(evt->iso_interval);
+	/* Transform to ms */
+	info->pto = info->interval * evt->pto;


for intervals of x.25, x.5 and x.75ms this calculation multiplies the error if the unit of evt->iso_interval is ms - but I think the unit of that is slot-pairs (1.25ms). hence you need to multiply by 1.25ms to get ms. Maybe micro seconds is a better unit to use?

See #45077 (comment)

Casper-Bonde-Bose · 2022-04-25T09:32:22Z

subsys/bluetooth/host/iso.c

+	info->irc = evt->irc;
+	info->interval = sys_le16_to_cpu(evt->iso_interval);
+	/* Transform to ms */
+	info->pto = info->interval * evt->pto;


Same as above - need to multiply by 1.25.

See #45077 (comment)

Casper-Bonde-Bose

Ups, left my comments as independent comments rather than review comments - sorry.
Unit needs to be fixed - remaining is up to you.

Casper-Bonde-Bose

Great work Emil!

cvinayak

Minor nitpick otherwise LGTM.

cvinayak · 2022-04-25T16:27:04Z

subsys/bluetooth/host/iso.c

+	unicast_info->cig_sync_delay = sys_get_le24(evt->cig_sync_delay);
+	unicast_info->cis_sync_delay = sys_get_le24(evt->cis_sync_delay);
+
+


Suggested change

Add all information from the ISO established events and provide the information in the get_info function. The use cases of each field heavily depends on what the ISO streams are used for. Most, if not all, of the field can be used by the higher layers to improve quality and/or reliability of e.g. audio streams that use ISO. Signed-off-by: Emil Gydesen <[email protected]>

github-actions bot added area: API Changes to public APIs area: Bluetooth area: Bluetooth Host Bluetooth Host (excluding BR/EDR) labels Apr 22, 2022

Thalley commented Apr 22, 2022

View reviewed changes

Thalley force-pushed the iso_additional_info branch from e432a41 to 88179cd Compare April 22, 2022 16:27

Thalley marked this pull request as ready for review April 22, 2022 18:12

Thalley requested review from alwa-nordic, jhedberg and Vudentz as code owners April 22, 2022 18:12

Thalley requested review from rick1082, cvinayak, alexsven and Casper-Bonde-Bose April 22, 2022 18:12

Casper-Bonde-Bose reviewed Apr 25, 2022

View reviewed changes

Casper-Bonde-Bose requested changes Apr 25, 2022

View reviewed changes

Thalley force-pushed the iso_additional_info branch 3 times, most recently from 4357773 to 6b94e29 Compare April 25, 2022 12:40

Thalley requested a review from Casper-Bonde-Bose April 25, 2022 12:40

Thalley force-pushed the iso_additional_info branch from 6b94e29 to 0044688 Compare April 25, 2022 14:34

Casper-Bonde-Bose previously approved these changes Apr 25, 2022

View reviewed changes

cvinayak requested changes Apr 25, 2022

View reviewed changes

Thalley dismissed Casper-Bonde-Bose’s stale review via d4428ff April 26, 2022 07:07

Thalley force-pushed the iso_additional_info branch from 0044688 to d4428ff Compare April 26, 2022 07:07

Thalley requested review from cvinayak and Casper-Bonde-Bose April 26, 2022 07:07

Casper-Bonde-Bose approved these changes Apr 26, 2022

View reviewed changes

cvinayak approved these changes Apr 26, 2022

View reviewed changes

carlescufi merged commit c972ffd into zephyrproject-rtos:main Apr 26, 2022

Thalley deleted the iso_additional_info branch July 8, 2022 09:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bluetooth: ISO: Add additional information for ISO streams #45077

Bluetooth: ISO: Add additional information for ISO streams #45077

Thalley commented Apr 22, 2022

Thalley Apr 22, 2022

Casper-Bonde-Bose Apr 25, 2022

Thalley Apr 22, 2022

Casper-Bonde-Bose Apr 25, 2022

Thalley Apr 25, 2022

Thalley Apr 22, 2022

Casper-Bonde-Bose Apr 25, 2022

Thalley Apr 25, 2022 •

edited

Loading

Thalley Apr 22, 2022

Casper-Bonde-Bose Apr 25, 2022

Thalley Apr 25, 2022

Thalley Apr 25, 2022

Casper-Bonde-Bose Apr 25, 2022 •

edited

Loading

Thalley Apr 25, 2022

Casper-Bonde-Bose Apr 25, 2022

Thalley Apr 25, 2022

Casper-Bonde-Bose Apr 25, 2022

Thalley Apr 25, 2022

Casper-Bonde-Bose left a comment

Casper-Bonde-Bose left a comment

cvinayak left a comment

cvinayak Apr 25, 2022

Thalley Apr 26, 2022

		central->phy = evt->c_phy;
		central->latency = sys_get_le16(evt->c_latency);

		unicast_info->cig_sync_delay = sys_get_le24(evt->cig_sync_delay);
		unicast_info->cis_sync_delay = sys_get_le24(evt->cis_sync_delay);

Bluetooth: ISO: Add additional information for ISO streams #45077

Bluetooth: ISO: Add additional information for ISO streams #45077

Conversation

Thalley commented Apr 22, 2022

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

Thalley Apr 25, 2022 • 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

Casper-Bonde-Bose Apr 25, 2022 • 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

Choose a reason for hiding this comment

Casper-Bonde-Bose left a comment

Choose a reason for hiding this comment

Casper-Bonde-Bose left a comment

Choose a reason for hiding this comment

cvinayak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thalley Apr 25, 2022 •

edited

Loading

Casper-Bonde-Bose Apr 25, 2022 •

edited

Loading