Bluetooth: Controller: Ext Adv Auxiliary PDUs with ticks_slot_window

[cvinayak]

Add implementation for Extended Advertising Auxiliary PDUs
to use ticks slot window feature.

This will allow the periodic scheduling of AUX_ADV_IND PDUs
to drift upto its next interval when overlapping with other
states/roles that cannot be moved around, to avoid skipping
them.

Having an active Extended Advertising simultaneously with
an ISO Synchronized Receiver or Connected ISO connection
will now have less ISO SDU loss.

The feature is a one-line assignment to use, but a conditional
compilation mess! Lets address that when it comes to #65125

[Thalley]

Would like a reply on #80733 (comment)

[cvinayak]

@Thalley I had to add additional commits to lower the HEAP size allotted and reduce supported Broadcast ISO instance where required to make room for increasing RAM usage in upstream main.

[Thalley]

Do we need this reduction for mesh?

[cvinayak]

Just being consistent with other conf files. I did not have the expertise to validate why we need HEAP at the first place at all. Is it like IPC uses unallocated RAM and this value overlaps such that IPC can function?

[Thalley]

Do we need this reduction for all ISO .confs?

Isn't it only the combined one?
Or is this just done to be consistent for all of them?

[cvinayak]

Just being consistent with other conf files.

[Thalley]

Ditto, do we need this reduction for DF?

[cvinayak]

Just being consistent with other conf files.

[Thalley]

Could this be further explained in a GH issue?

This isn't something I understand as it is written here

[cvinayak]

Yes, I will create a detailed GH issue in the coming days... need to put some effort on simple sample rather that the audio tests to be able to reproduce/demo the issue.

The related code is:

zephyr/samples/bluetooth/hci_ipc/src/main.c

Lines 165 to 199 in 9a80457

	static void hci_ipc_rx(uint8_t *data, size_t len)
	{
	uint8_t pkt_indicator;
	struct net_buf *buf = NULL;
	size_t remaining = len;
	LOG_HEXDUMP_DBG(data, len, "IPC data:");
	pkt_indicator = *data++;
	remaining -= sizeof(pkt_indicator);
	switch (pkt_indicator) {
	case HCI_IPC_CMD:
	buf = hci_ipc_cmd_recv(data, remaining);
	break;
	case HCI_IPC_ACL:
	buf = hci_ipc_acl_recv(data, remaining);
	break;
	case HCI_IPC_ISO:
	buf = hci_ipc_iso_recv(data, remaining);
	break;
	default:
	LOG_ERR("Unknown HCI type %u", pkt_indicator);
	return;
	}
	if (buf) {
	k_fifo_put(&tx_queue, buf);
	LOG_HEXDUMP_DBG(buf->data, buf->len, "Final net buffer:");
	}
	}

This function can return without call to k_fifo_put(), and there is no resumption/dequeue of Rx when the buffers are available later in time, causing the buffers/stream inside IPC to be stagnate until any next trigger that would call the hci_ipc_rx() again.

The next trigger would be a num complete event (or disconnect or any reports?) going upstream (one side of the Newton's Cradle, say one item strikes), consequently (other side of the Newton's Cradle, one item now strikes) new buffers are enqueued from upstream.

This effect can happen in either direction, I believe.

[Thalley]

As I mentioned in #80733 (comment): This is messy. We shouldn't have overlays/extra conf files that need to be added in a specific order to work, especially when it's not documented.

[cvinayak]

There is repo wide use of extra_configs in sample.yaml and testcase.yaml files to use overrides of Kconfig values from prj.conf and use of EXTRA_CONF_FILE is better organization wise compared to use of list of Kconfigs under extra_configs.

OVERLAY_CONFIG is deprecated, but it did convey that something was overlay-ed!

[Thalley]

OVERLAY_CONFIG is deprecated, but it did convey that something was overlay-ed!

Right :D I should stop calling them overlays

There is repo wide use of extra_configs in sample.yaml and testcase.yaml files to use overrides of Kconfig values from prj.conf and use of EXTRA_CONF_FILE is better organization wise compared to use of list of Kconfigs under extra_configs.

If we have several places where we use multiple extra configs that needs to be specific in a specific order, that's not a good reason to do it here either. IMO that just show that we have this other places too.

A file called testcase.yaml does/should not be used as documentation. If these values are required for this specific board to work, then that needs to be documented and/or applied automatically.

[cvinayak]

It is documented in the snippets section that the processing order of the snippets, .conf and .overlay files are "in the order they are listed".

zephyr/doc/build/snippets/writing.rst

Lines 98 to 126 in 2879607

	Processing order
	****************
	Snippets are processed in the order they are listed in the :makevar:`SNIPPET`
	variable, or in the order of the ``-S`` arguments if using west.
	To apply ``bar`` after ``foo``:
	.. code-block:: console
	cmake -Sapp -Bbuild -DSNIPPET="foo;bar" [...]
	cmake --build build
	The same can be achieved with west as follows:
	.. code-block:: console
	west build -S foo -S bar [...] app
	When multiple snippets set the same configuration, the configuration value set
	by the last processed snippet ends up in the final configurations.
	For instance, if ``foo`` sets ``CONFIG_FOO=1`` and ``bar`` sets
	``CONFIG_FOO=2`` in the above example, the resulting final configuration will
	be ``CONFIG_FOO=2`` because ``bar`` is processed after ``foo``.
	This principle applies to both Kconfig fragments (``.conf`` files) and
	devicetree overlays (``.overlay`` files).

[Thalley]

Since this is an overlay, what's the purpose or overlaying these values when they are already provided by the "main" conf file?

Shouldn't the overlay only contain values that are different?

[cvinayak]

These are Bluetooth Common and Host Kconfigs that have references (inheritance of values/counts) in the Controller Kconfig options, hence relisting them explicitly to ensure the dependencies are consistent when this overlay file is included in the build.

[Thalley]

hence relisting them explicitly to ensure the dependencies are consistent when this overlay file is included in the build.

But in the case that e.g. CONFIG_BT_PERIPHERAL=n in the "main" file, we don't want to enable it in the overlay.

I think this will make it harder to maintain as it is effectively code duplication, and any changes has to be done to both files to keep the nRF52 and nRF53 build similar.

I'm not sure I see the advantage of duplicating them

[cvinayak]

I have removed the commit that makes the nrf52 same as nrf53... will address that in a separate PR, may be having different Kconfig for nRF52 and nRF53 could be better in long run to discover bugs.

I will explore use of snippets, say "bt-ll-sw-split-experimental" that hopefully can be common across supported platforms.

[Thalley]

Yeah, we really need a better solution for this :)

[cvinayak]

#AutoPTS run zephyr nrf53 BAP/UCL/STR/BV-523-C

[Thalley]

#AutoPTS run zephyr nrf53 BAP/UCL/STR/BV-523-C

[codecoup-tester]

Scheduled PR #80733 (comment), board: nrf53, estimated start time: 14:15:41, test case count: 1, estimated duration: 0:01:30

Test cases to be run

BAP/UCL/STR/BV-523-C

[codecoup-tester]

AutoPTS Bot results:
No failed test found.

Successful tests (1)

BAP BAP/UCL/STR/BV-523-C PASS (2)