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

applications: nrf5340_audio: Add a deinitialise function #16296

Merged
merged 1 commit into from
Jul 29, 2024

Conversation

gWacey
Copy link
Contributor

@gWacey gWacey commented Jul 2, 2024

The data FIFO module did not have a deinitialise function, so added one, with a twister test.

@gWacey gWacey requested review from a team, anangl and rlubos as code owners July 2, 2024 14:20
@gWacey gWacey requested review from koffes, andvib, rick1082 and alexsven and removed request for a team July 2, 2024 14:20
@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 Jul 2, 2024
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Jul 2, 2024

Test specification

CI/Jenkins/NRF

  • Integration Platforms

CI/Jenkins/integration

Test Module File based changes Manually selected West overwrite

Detailed information of selected test modules

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

@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.

@@ -167,6 +167,22 @@ int data_fifo_empty(struct data_fifo *data_fifo)
return 0;
}

int data_fifo_deinit(struct data_fifo *data_fifo)
{
__ASSERT_NO_MSG(data_fifo != NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend using ASSERT with message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On this, It is good to test like this, but if there is an issue in the release that would have exercised this assert but it has been optimised out, then the codes action would be undetermined and potentially difficult to debug. Would be more code but better in my option to explicitly test for NULL. Actually it shouldn't be more than a test zero and branch (with branch prediction this will be even better). An error is already returned and tested for, so the code could gracefully fail, rather than go off somewhere into memory!

/**
* @brief Deinitialise the data_fifo.
*
* @note The fifo is emptied first, so it is the users responsibility to release any data items it
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the user does not release all the data items?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What will happen to any buffers that are carried in the message?
We get a message and load it with a pointer to some data (on a slab for example). When all the messages are released what happens to this item and the slab it is on? It may be lost to the system!

tests/lib/data_fifo/src/main.c Outdated Show resolved Hide resolved
include/data_fifo.h Outdated Show resolved Hide resolved
@gWacey gWacey force-pushed the Data-FIFO-Deinitalise branch 3 times, most recently from 26e16b6 to 0b6efef Compare July 16, 2024 14:10
@gWacey gWacey requested a review from tejlmand as a code owner July 16, 2024 14:10
@github-actions github-actions bot added doc-required PR must not be merged without tech writer approval. manifest labels Jul 17, 2024
@github-actions github-actions bot added changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. and removed doc-required PR must not be merged without tech writer approval. manifest ble mesh Label for ble mesh PRbot. Add this if PR is related to ble mesh and you need to get review. labels Jul 23, 2024
@coderbyheart coderbyheart removed their request for review July 23, 2024 09:13
@koffes koffes self-requested a review July 29, 2024 14:01
    The data FIFO module did not have a deinitialize function so
	added one, with a twister test.

Signed-off-by: Graham Wacey <graham.wacey@nordicsemi.no>
@rlubos rlubos merged commit c706bb2 into nrfconnect:main Jul 29, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants