Skip to content

Conversation

@yuxCai
Copy link
Contributor

@yuxCai yuxCai commented Feb 23, 2024

There is no need to allocate TX/RX buffers when the device is an receiver/transmitter only.

@github-actions
Copy link

Hello @yuxCai, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

Copy link
Contributor

Choose a reason for hiding this comment

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

There is already depends on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BT_CTLR_ISO_TX_BUFFERS is set to default BT_ISO_TX_BUF_COUNT if BT_ISO. But when BT_ISO_TX_BUF_COUNT is 0, it will return error because BT_CTLR_ISO_TX_BUFFERS is out of the range

BT_CTLR_ISO_RX_BUFFERS, however, is independent of BT_ISO_RX_BUF_COUNT. I suggest changing the minimum values of the mentioned kconfigs to 0

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
range 0 30
range 1 30

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
range 0 255
range 1 255

yuxCai added a commit to yuxCai/sdk-zephyr that referenced this pull request Feb 23, 2024
Upstream PR: zephyrproject-rtos/zephyr#69382

There is no need to allocate TX/RX buffers when the device is an receiver/transmitter only.

Signed-off-by: Yuxuan Cai <yuxuan.cai@nordicsemi.no>
Copy link
Contributor

@Thalley Thalley left a comment

Choose a reason for hiding this comment

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

CONFIG_BT_ISO_TX_BUF_COUNT and CONFIG_BT_ISO_RX_BUF_COUNT are not specific to the controller, and are also being used by the host, so the commit should not start with Bluetooth: Controller.

Furthermore, setting these to 0 might cause issues in the host:

NET_BUF_POOL_FIXED_DEFINE(iso_tx_pool, CONFIG_BT_ISO_TX_BUF_COUNT,
			  BT_ISO_SDU_BUF_SIZE(CONFIG_BT_ISO_TX_MTU),
			  CONFIG_BT_CONN_TX_USER_DATA_SIZE, NULL);

What happens if we define a pool with 0 count?

It should also not be possible to set BT_ISO_TX_BUF_COUNT=0 when BT_ISO_BROADCASTER=y, or BT_ISO_RX_BUF_COUNT=0 when BT_ISO_SYNC_RECEIVER=y.

There are likely several places in iso.c that needs to be guarded if we allow either of them to be 0.

Also consider making some promptless Kconfig options that depend on these, so that if BT_ISO_TX_BUF_COUNT > 0 then BT_ISO_TX gets enabled, and similar for RX.

@cvinayak cvinayak dismissed their stale review February 25, 2024 06:04

My requests addressed

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Apr 26, 2024
@kruithofa kruithofa removed their request for review April 26, 2024 10:21
@github-actions github-actions bot removed the Stale label Apr 27, 2024
There is no need to allocate TX/RX buffers when the device
is an receiver/transmitter only.

Signed-off-by: Yuxuan Cai <yuxuan.cai@nordicsemi.no>
@kartben kartben force-pushed the zero_ISO_buffer branch from 2c780e7 to 190eff4 Compare June 7, 2024 09:59
@kartben
Copy link
Contributor

kartben commented Jun 7, 2024

rebased to trigger stuck CI job

@jhedberg
Copy link
Member

jhedberg commented Jun 7, 2024

@yuxCai @cvinayak is the "Controller" prefix actually correct, since this is not modifying a controller-specific file. Are these options used elsewhere? If not, then they should probably be moved into subsys/bluetooth/controller/.

@cvinayak
Copy link
Contributor

cvinayak commented Jun 7, 2024

@yuxCai @cvinayak is the "Controller" prefix actually correct, since this is not modifying a controller-specific file. Are these options used elsewhere? If not, then they should probably be moved into subsys/bluetooth/controller/.

As already mentioned, these are not (now) Controller Kconfig options after my change requests where addressed (only value may be inherited). The commit title needs correction. Plus, all code that use these Kconfig define need update to handle value of 0.

@Thalley
Copy link
Contributor

Thalley commented Jun 7, 2024

@yuxCai @cvinayak is the "Controller" prefix actually correct, since this is not modifying a controller-specific file. Are these options used elsewhere? If not, then they should probably be moved into subsys/bluetooth/controller/.

These options are also used by the host. See #69382 (review)

@github-actions
Copy link

github-actions bot commented Aug 7, 2024

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Aug 7, 2024
@github-actions github-actions bot closed this Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants