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.

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

nimble/host: Add Initial Unicast implementation #1622

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KKopyscinski
Copy link
Contributor

This commit provides initial implementation of ISO in host.

@KKopyscinski KKopyscinski force-pushed the leaudio_iso_initial branch 4 times, most recently from f7e5a28 to c9df490 Compare September 25, 2023 11:06
@KKopyscinski KKopyscinski force-pushed the leaudio_iso_initial branch 2 times, most recently from 9b2461d to 777e183 Compare October 3, 2023 06:07
@KKopyscinski
Copy link
Contributor Author

KKopyscinski commented Oct 3, 2023

@rymanluk I also checked build again, resolved some issues and added missing code from cd795d0

@KKopyscinski KKopyscinski force-pushed the leaudio_iso_initial branch 2 times, most recently from 38f8717 to 71ffab7 Compare October 9, 2023 07:15
static int
ble_hs_rx_iso(struct os_mbuf *om, void *arg)
{
#if MYNEWT_VAL(BLE_ISO)
Copy link
Contributor

Choose a reason for hiding this comment

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

@andrzej-kaczmarek please check if MYNEWT_VAL name is OK for you

@@ -1276,9 +1276,6 @@ ble_ll_hci_le_cmd_proc(const uint8_t *cmdbuf, uint8_t len, uint16_t ocf,
break;
#endif /* BLE_LL_ISO_BROADCASTER */
#if MYNEWT_VAL(BLE_LL_CFG_FEAT_LL_ISO)
case BLE_HCI_OCF_LE_READ_ISO_TX_SYNC:
Copy link
Contributor

Choose a reason for hiding this comment

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

why we are removing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not implemented in original patch, but is implemented in upstream, here: https://github.com/apache/mynewt-nimble/pull/1622/files/71ffab70ca8289793dbfba862938865a75e97568#diff-b69485df5b88a9df6b5628d97643bd77c54001a4027fc3a0384ae016308dd2f4R1308

basically case is duplicated, once as empty boilerplate and second time with actual code

Copy link
Contributor

Choose a reason for hiding this comment

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

if we are having duplicated code already merged - please create separate patch for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this will be moved to seperate patch

mask |= 0x0000000040000000;
}

#if MYNEWT_VAL(BLE_ISO)
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering if we should not have two MYNEWT_VALs - one for Broadcast and one for unicast.

@andrzej-kaczmarek any opinion on that?

* Enable the following LE events:
* 0x0000000040000000 LE Request Peer SCA Complete event
*/
mask |= 0x0000000040000000;
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be separate patch I guess.

@@ -102,6 +102,10 @@ syscfg.defs:
value: 0
restrictions:
- 'BLE_ISO if 1'
BLE_ISO_LOOPBACK:
Copy link
Contributor

Choose a reason for hiding this comment

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

this should not be part of this patch. Maybe later on could be added

@@ -173,6 +178,48 @@ static int s_ble_hci_device = MYNEWT_VAL(BLE_SOCK_LINUX_DEV);
static int s_ble_hci_device = 0;
#endif

static int
ble_hci_sock_iso_tx(struct os_mbuf *om)
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering if this transport patch could be in separate patch ?

@@ -497,10 +507,21 @@ syscfg.defs:
description: 'Minimum level for the BLE EATT log.'
value: 1

BLE_HS_LEAUDIO_MOD:
Copy link
Contributor

Choose a reason for hiding this comment

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

this LOGs could be separate patch. I think it was used in the Application using ISO right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe? I don't see this LOG being used anywhere, so I can remove it

}

static void
ble_iso_sent_big_complete_event(struct ble_iso_group *big,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should split this patch for Broadcast and Unicast.

@KKopyscinski KKopyscinski marked this pull request as draft October 17, 2023 06:53
@KKopyscinski KKopyscinski force-pushed the leaudio_iso_initial branch 2 times, most recently from cd31421 to af6937b Compare December 8, 2023 13:47
nimble/host/src/ble_hs_hci.c Fixed Show fixed Hide fixed
nimble/host/src/ble_hs_hci.c Fixed Show fixed Hide fixed
@KKopyscinski KKopyscinski changed the title nimble/host: Add Initial ISO implementation nimble/host: Add Initial Unicast implementation Dec 11, 2023
@KKopyscinski KKopyscinski force-pushed the leaudio_iso_initial branch 2 times, most recently from 9c29813 to 64622e9 Compare December 12, 2023 07:40
This commit provides initial implementation ofunicast ISO in host.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants