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

Added ability to change task notification index for streambuffers #939

Merged

Conversation

glemco
Copy link
Contributor

@glemco glemco commented Dec 28, 2023

Description

This PR adds the modification described in https://forums.freertos.org/t/multiple-message-buffers-per-task-interferences/17663:

The code change necessary to enable your scenario should be relatively simple though - adding a member to the stream buffer structure that holds the task notification index to use with that stream buffer, and a utility API that sets that index

In short, all calls to xTaskNotify* APIs are now using the *Indexed version and a new field was added to StreamBuffer_t to store the notification value used for that buffer.
Now, this value can be get and set using new functions (uxStreamBufferGetStreamBufferNotificationIndex and vStreamBufferSetStreamBufferNotificationIndex) to avoid changing the overall API.

Test Steps

  • Create any stream buffer, by default this will use 0 (tskDEFAULT_INDEX_TO_NOTIFY) as index.
  • Change the index using vStreamBufferSetStreamBufferNotificationIndex
    • Of course the configuration should allow this with a proper value of configTASK_NOTIFICATION_ARRAY_ENTRIES
    • The value must be changed before any task waits for the buffer, otherwise we may never wake them up
  • Use the buffer as normal, it should block and resume as usual
  • Sending a notification to the task to a different index should not wake up the task.

Checklist:

Related Issue

https://forums.freertos.org/t/multiple-message-buffers-per-task-interferences/17663

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link

codecov bot commented Dec 28, 2023

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (4568507) 93.78% compared to head (42e5cea) 93.43%.

Files Patch % Lines
stream_buffer.c 36.84% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #939      +/-   ##
==========================================
- Coverage   93.78%   93.43%   -0.35%     
==========================================
  Files           6        6              
  Lines        3186     3199      +13     
  Branches      885      890       +5     
==========================================
+ Hits         2988     2989       +1     
- Misses         91      103      +12     
  Partials      107      107              
Flag Coverage Δ
unittests 93.43% <36.84%> (-0.35%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@AniruddhaKanhere AniruddhaKanhere left a comment

Choose a reason for hiding this comment

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

Hello @glemco, thank you for taking the time to raise this pull request by following up on the forum post.

The change looks good to me and seems correct. I am approving this change in advance contingent on the fixing of the failing checks.

Thanks again!

@glemco
Copy link
Contributor Author

glemco commented Dec 29, 2023

/bot run formatting

@glemco
Copy link
Contributor Author

glemco commented Dec 29, 2023

Thanks @AniruddhaKanhere for the approval, I'm trying to figure out how to fix the failed steps, besides formatting which is trivial, I'm not quite sure why is the link verifier complaining and how to convince the coverage report that the functions are checked (or at least they should be in FreeRTOS/FreeRTOS#1150, which obviously cannot be merged before this change in the kernel)

@AniruddhaKanhere
Copy link
Member

Hey @glemco,

Apologies for the late response.

The link check issue seems to have been a transient failure. We are aware of this circular dependency between tests and the PR checks. We can ignore the failing unit tests for now.

Spell check is failing though. To fix that, you need to add the list of outlier spellings in this file.

@glemco
Copy link
Contributor Author

glemco commented Jan 4, 2024

Thanks @AniruddhaKanhere for the explanation! It seems after your merge the spellcheck is not failing any more (probably another transient failure? It didn't seem related to the code I changed)
Does it mean this PR is now ready or is there anything else I could take care of?

Signed-off-by: Gaurav Aggarwal <[email protected]>
Copy link

sonarqubecloud bot commented Jan 4, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

2 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@aggarg aggarg changed the title Added possibility to change notification index for streambuffers Added ability to change task notification index for streambuffers Jan 4, 2024
@AniruddhaKanhere
Copy link
Member

Thank you for this PR @glemco and I apologize for the confusion with the checks. We are looking into what caused those transient failures. Maybe it was just that this branch was out of sync with the main repository.

Thank you @aggarg for the approval!

I shall be merging this PR now.

@AniruddhaKanhere AniruddhaKanhere merged commit 1947dd2 into FreeRTOS:main Jan 4, 2024
14 of 16 checks passed
@glemco glemco deleted the streambuffer_notification_index branch January 5, 2024 06:24
laroche pushed a commit to laroche/FreeRTOS-Kernel that referenced this pull request Apr 18, 2024
Update Readme for the FreeRTOS_PLUS_TCP_ECHO_QEMU_msp2

Signed-off-by: Gaurav Aggarwal <[email protected]>
Co-authored-by: Gaurav Aggarwal <[email protected]>
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.

4 participants