Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bluetooth: Controller: Enable ISO support in the SDC #12508

Merged
merged 1 commit into from Oct 5, 2023
Merged

Bluetooth: Controller: Enable ISO support in the SDC #12508

merged 1 commit into from Oct 5, 2023

Conversation

rugeGerritsen
Copy link
Contributor

This commit enables the experimental ISO support in the SoftDevice Controller. See the changelog for more details.

@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 3, 2023
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Oct 3, 2023 •
edited

Test specification

CI/Jenkins/NRF

  • Integration Platforms

CI/Jenkins/integration

Test Module File based changes Manually selected West overwrite
test-fw-nrfconnect-ble X
test-fw-nrfconnect-ble_samples X
test-fw-nrfconnect-chip X
test-fw-nrfconnect-fem X
test-fw-nrfconnect-rs X
test-fw-nrfconnect-zigbee X
test-sdk-find-my X
test-sdk-homekit X
test-sdk-sidewalk X

Detailed information of selected test modules

Note: This message is automatically posted and updated by the CI

sean-madigan
sean-madigan reviewed Oct 3, 2023
View reviewed changes
subsys/bluetooth/controller/hci_driver.c Outdated

#if defined(CONFIG_BT_CTLR_SYNC_ISO)
#define SDC_MEM_BIS_SINK \
SDC_MEM_PER_BIG(CONFIG_BT_CTLR_SYNC_ISO_STREAM_COUNT) + \
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm currently trying to merge a fix to change this to CONFIG_BT_CTLR_SCAN_SYNC_ISO_SET

Copy link
Contributor

Choose a reason for hiding this comment

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

and here https://github.com/nrfconnect/sdk-nrf/pull/12508/files#diff-ca9ee596a131877b2cbc629ccf2e965015db73c3d5cc74be46a064c4bfbf81a8R1019

@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

@Tschet1 Tschet1 self-requested a review October 4, 2023 06:34
Tschet1
Tschet1 reviewed Oct 4, 2023
View reviewed changes
subsys/bluetooth/controller/hci_internal.c
(sdc_hci_cmd_le_remove_iso_data_path_t const *)cmd_params,
(sdc_hci_cmd_le_remove_iso_data_path_return_t *)event_out_params);

case SDC_HCI_OPCODE_CMD_LE_ISO_TEST_END:
Copy link
Contributor

Choose a reason for hiding this comment

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

we can do that in a separate PR, but should we consider ifdef-ing out the test commands?

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 agree. We could consider adding more ifdefs for both the ISO test commands and also the Create CIG/BIG Test commands, Read Link quality and others

subsys/bluetooth/controller/hci_internal.c Outdated
(sdc_hci_cmd_le_iso_test_end_t const *)cmd_params,
(sdc_hci_cmd_le_iso_test_end_return_t *)event_out_params);

case SDC_HCI_OPCODE_CMD_LE_SET_HOST_FEATURE:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be in a CONFIG_BT_CTLR_SET_HOST_FEATURE ?

Copy link
Contributor

Choose a reason for hiding this comment

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

should it also be outside the CONFIG_BT_CTLR_ISO, to be aligned with https://github.com/nrfconnect/sdk-nrf/pull/12508/files#diff-f5bf9d2614b19c06ca472417c8004daeae8125af4a59c8ec6983180c9ddae3daR560 ?

subsys/bluetooth/controller/hci_internal.c Outdated
@@ -40,6 +40,7 @@ static bool command_generates_command_complete_event(uint16_t hci_opcode)
{
switch (hci_opcode) {
case SDC_HCI_OPCODE_CMD_LC_DISCONNECT:
case SDC_HCI_OPCODE_CMD_CB_WRITE_CONN_ACCEPT_TIMEOUT:
Copy link
Contributor

Choose a reason for hiding this comment

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

The SDC_HCI_OPCODE_CMD_CB_WRITE_CONN_ACCEPT_TIMEOUT creates a command complete

Tschet1
Tschet1 requested changes Oct 4, 2023
View reviewed changes
Copy link
Contributor

@Tschet1 Tschet1 left a comment •
edited

Choose a reason for hiding this comment

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

.

Tschet1
Tschet1 requested changes Oct 4, 2023
View reviewed changes
subsys/bluetooth/controller/hci_internal.c Outdated
(sdc_hci_cmd_le_iso_test_end_t const *)cmd_params,
(sdc_hci_cmd_le_iso_test_end_return_t *)event_out_params);

case SDC_HCI_OPCODE_CMD_LE_SET_HOST_FEATURE:
Copy link
Contributor

Choose a reason for hiding this comment

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

should it also be outside the CONFIG_BT_CTLR_ISO, to be aligned with https://github.com/nrfconnect/sdk-nrf/pull/12508/files#diff-f5bf9d2614b19c06ca472417c8004daeae8125af4a59c8ec6983180c9ddae3daR560 ?

rtreccani
rtreccani approved these changes Oct 4, 2023
View reviewed changes
Copy link
Contributor

@rtreccani rtreccani left a comment

Choose a reason for hiding this comment

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

LGTM

subsys/bluetooth/controller/hci_driver.c
#if defined(CONFIG_BT_CTLR_ISO_TX_BUFFERS)
static int iso_handle(struct net_buf *acl)
{
LOG_DBG("");
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the point of this?

@rugeGerritsen rugeGerritsen force-pushed the enable_iso_support branch 2 times, most recently from cd4866f to a8ee792 Compare October 4, 2023 11:27
@rugeGerritsen
Bluetooth: Controller: Enable ISO support in the SDC
9f2240d 
This commit enables the experimental ISO support in the SoftDevice
Controller. See the changelog for more details.

Signed-off-by: Rubin Gerritsen <rubin.gerritsen@nordicsemi.no>
@rugeGerritsen rugeGerritsen removed DNM changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Oct 5, 2023
@nordicjm nordicjm merged commit b37c09b into nrfconnect:main Oct 5, 2023
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rtreccani rtreccani rtreccani approved these changes

@Tschet1 Tschet1 Tschet1 approved these changes

@sean-madigan sean-madigan sean-madigan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@rugeGerritsen @NordicBuilder @rtreccani @Tschet1 @sean-madigan @nordicjm