Bluetooth: controller: Fix ISO broadcaster pre-transmission groups > 1 #77568
Conversation
f2ab986 to
cc18a3a
Compare
cc18a3a to
736ca10
Compare
|
@mtpr-ot This PR was never put to ready for review, intentional? Seems this is valid change request, if there is no objection, I will put this for review and follow up (provided can do some testing if possible) |
|
Hi @cvinayak - Morten recently left Oticon, so I'm not sure if he is still active with this account. In any case, we are using this change downstream, so I think it is just an oversight that this PR never left draft. |
Ok, I will follow up on the failing bsim CI test, suspect an overlapping/drifting ACL role leading to missing ISO SDU reception... |
|
The original author has provided the permissions: "Maintainers are allowed to edit this pull request. " |
736ca10 to
5a58fd9
Compare
| (lll->ptc_curr * lll->pto); | ||
| payload_count = lll->payload_count + payload_index - lll->bn; | ||
| if (lll->ptc_curr) { | ||
| uint8_t ptx_idx = lll->ptc_curr - 1; /* max. nse 5 bits */ |
There was a problem hiding this comment.
Since ptc_curr is uint32_t ptc_curr:4;, then shouldn't this be
| uint8_t ptx_idx = lll->ptc_curr - 1; /* max. nse 5 bits */ | |
| uint8_t ptx_idx = lll->ptc_curr - 1; /* max. nse 4 bits */ |
Which then also affect the bits of several of the values below?
There was a problem hiding this comment.
Now I do not remember why
ptcis 4 bits! ... it can be as much as (NSE - 1); 1 transmission followed by (NSE - 1) pre-transmissions.@Tronil and @wopu-ot , are you testing the boundary conditions internally in your tests for this PR changes?
We do not have extensive test coverage on the broadcaster side, no
There was a problem hiding this comment.
@Tronil while testing this PR for PTO=2 and Group Count = 2, I discovered a bug in ull_sync_iso.c that subsequent BIS subevent reception failed; ptc was not assigned the correct value. This bug will not show up if only one BIS is synchronized to.
There was a problem hiding this comment.
Shouldn't the comment be updated?
| (lll->ptc_curr * lll->pto); | ||
| payload_count = lll->payload_count + payload_index - lll->bn; | ||
| if (lll->ptc_curr) { | ||
| uint8_t ptx_idx = lll->ptc_curr - 1; /* max. nse 5 bits */ |
For pre-transmission groups > 1, the broadcaster link layer would fetch incorrect payloads from the TX node queue. Update payload index calculation to reference correct TX payload. Fix PTO FIXMEs and range checks. Signed-off-by: Morten Priess <mtpr@oticon.com>
9661220 to
d3fefcb
Compare
| (lll->ptc_curr * lll->pto); | ||
| payload_count = lll->payload_count + payload_index - lll->bn; | ||
| if (lll->ptc_curr) { | ||
| uint8_t ptx_idx = lll->ptc_curr - 1; /* max. nse 5 bits */ |
There was a problem hiding this comment.
Shouldn't the comment be updated?
Add back the implementation in the Controller that tries to enable pre-transmissions to improve time diversity to aid a remote ISO Sync Receiver role device. Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
d3fefcb to
7f70919
Compare
Fixup payload_index to be uint16_t to avoid index overflow. Do not remember why ptc is 4 bits, where as it must be 5 bit value similar to nse; added an assertion check until it is fixed. Fix ISO Broadcaster and ISO Sync Receiver for PTO > 1 and use of Pre-Transmission Group Counts. Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
7f70919 to
de3f005
Compare
For pre-transmission groups > 1, the broadcaster link layer would fetch incorrect payloads from the TX node queue.
Update payload index calculation to reference correct TX payload. Fix PTO FIXMEs and range checks.
bsim bap_broadcast_audio_source/sink tested with PTO=2 PTO Group Count = 2:
bsim_bap_broadcast_source_sink_pto_2_ptogc_2.zip