Skip to content

Conversation

@Thalley
Copy link
Contributor

@Thalley Thalley commented Oct 31, 2024

Several tests were using K_FOREVER when allocating the buffer for TX in the system workqueue, which is illegal behavior.

The solution chosen was to create a TX thread to handle TX, similar to the solution used in the audio shell and some sample applications.

This way we can continue to use K_FOREVER when allocting buffers and it will always be done in a round-robin fashion while TXing as much as possible, by always enqueuing all the buffers with mock data.

Since this works for all streams (both broadcast and unicast), it was obvious to use the same implementation for all tests, and thus cleaning up the tests a bit and more them more similar.

fixes #80682

Copy link
Contributor

@rruuaanng rruuaanng Nov 20, 2024

Choose a reason for hiding this comment

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

Why is it can_send, not is_send or is_conf_send.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To follow the stream_tx prefit it sounds weird to have stream_tx_is_send or stream_tx_is_conf_send as stream_tx refers to the module, and not the stream

Choose a reason for hiding this comment

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

The FAIL macro does not add a CR-LF, so I don't think you want to do this change
We should probably fix this at other places as well

Choose a reason for hiding this comment

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

Suggested change
LOG_MODULE_REGISTER(stream_rx, LOG_LEVEL_INF);
LOG_MODULE_REGISTER(bap_stream_rx, LOG_LEVEL_INF);

Not a required change but the other modules use the complete module-name when registering with the logging system.

Choose a reason for hiding this comment

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

question: Do I understand it correctly that when stream_tx_can_send is false we do not actually start streaming for this stream?
If my understanding is correct then probably we shouldn't set the flag_stream_started

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Streams are unidirectional. For RX-only streams stream_tx_can_send will return false, but the stream is still started in the sense that the ASE is in the streaming state

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.e. "started" here does not refer to the TXing state

kruithofa
kruithofa previously approved these changes Dec 4, 2024
cvinayak
cvinayak previously approved these changes Dec 6, 2024
@kartben
Copy link
Contributor

kartben commented Dec 6, 2024

@Thalley needs a rebase

Several tests were using K_FOREVER when allocating the
buffer for TX in the system workqueue, which is illegal behavior.

The solution chosen was to create a TX thread to handle TX,
similar to the solution used in the audio shell and some
sample applications.

This way we can continue to use K_FOREVER when allocting buffers
and it will always be done in a round-robin fashion while
TXing as much as possible, by always enqueuing all the buffers
with mock data.

Since this works for all streams (both broadcast and unicast),
it was obvious to use the same implementation for all tests,
and thus cleaning up the tests a bit and more them more similar.

Signed-off-by: Emil Gydesen <[email protected]>
@Thalley Thalley dismissed stale reviews from cvinayak and kruithofa via 89bfb01 December 6, 2024 14:30
@Thalley Thalley requested review from cvinayak and kruithofa December 6, 2024 14:30
@kartben kartben merged commit 778a9af into zephyrproject-rtos:main Dec 10, 2024
26 checks passed
@Thalley Thalley deleted the audio_bsim_tx_fix branch December 10, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Bluetooth: Audio: BSIM tests use K_FOREVER when allocating TX buffers in system workqueue

6 participants