Bluetooth: CAP: Add support for doing just disable for unicast stop #74187
Conversation
Thalley
force-pushed
the
cap_initiator_unicast_stop_update
branch
9 times, most recently
from
20bfc6f
to
13ff2c6
Compare
Thalley
force-pushed
the
cap_initiator_unicast_stop_update
branch
from
13ff2c6
to
bcbb88d
Compare
Thalley
force-pushed
the
cap_initiator_unicast_stop_update
branch
from
bcbb88d
to
6b14334
Compare
Thalley
force-pushed
the
cap_initiator_unicast_stop_update
branch
2 times, most recently
from
11939f8
to
01b5556
Compare
Thalley
force-pushed
the
cap_initiator_unicast_stop_update
branch
from
01b5556
to
4904b5c
Compare
zephyrbot
added
area: Bluetooth
area: Bluetooth Qualification
zephyrbot
requested review from
asbjornsabo,
Casper-Bonde-Bose,
fredrikdanebjer,
jhedberg,
kruithofa,
larsgk,
MariuszSkamra,
niym-ot,
pin-zephyr and
sjanc
| } | ||
| } | ||
|
|
||
| if (!can_disable && !can_stop && !can_release) { |
There was a problem hiding this comment.
Does -EALREADY make sense here?
The GNU spec. states it as:
'EALREADY
The socket socket is non-blocking and already has a pending connection in progress (see EINPROGRESS above).'
And while its true that we "already has a pending connection in progress", I think the intent of the wording is when we try to connect a new one.
Could EBUSY make more sense here?
There was a problem hiding this comment.
The case is here is that all streams are already stopped, and thus -EBUSY doesn't make any sense.
The alternative is that we return 0, but then it's hard for an application to determine whether a request was successfully sent, or if nothing was done.
We need a way to tell the caller that no request was sent because it would not result in any state changes
There was a problem hiding this comment.
I guess this make sense. In regardless, I certainly think 0 would be wrong here :D
Thalley
force-pushed
the
cap_initiator_unicast_stop_update
branch
from
4904b5c
to
ca8f521
Compare
Thalley
force-pushed
the
cap_initiator_unicast_stop_update
branch
from
ca8f521
to
83a839f
Compare
The unicast_stop function is changed to primarily do a BAP disable instead of a release, with optional support for releasing the streams once they have been disabled. This also adds unittests for the procedure which also allow us to remove the invalid param testing from the BSIM tests. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
Thalley
force-pushed
the
cap_initiator_unicast_stop_update
branch
from
83a839f
to
5448837
Compare
|
|
||
| bt_cap_common_start_proc(BT_CAP_COMMON_PROC_TYPE_STOP, param->count); | ||
| if (!can_disable && can_disable_stream(bap_stream)) { |
There was a problem hiding this comment.
Somehow missing the link to my original comment. Anyway good point that this is in a loop.
It still could be rewritten as
can_disable = can_disable || can_disable_stream(bap_stream);
which still would do the short circuiting, but I am not sure if that's more readable than what you have now
There was a problem hiding this comment.
Hmm, I think the current version is more explicit. Not sure if there's any performance changes between the two, but I think can_disable = can_disable || can_disable_stream(bap_stream) might be worth as it always does 1 assign and 1 compare, whereas the current version always do 1 comparison and optionally an assign.
|
Investigating a possible issue |
There is indeed an issue, but it's not caused by this PR nor easily fixable by this PR, and will be fixed in #79014 |
The unicast_stop function is changed to primarily do a
BAP disable instead of a release, with optional
support for releasing the streams once they have been disabled.
fixes #70818