-
Notifications
You must be signed in to change notification settings - Fork 39
refactor(esp_tinyusb): TinyUSB task deletion notification driven (part 1/3) #322
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the TinyUSB runtime_config test application to improve test reliability and enable local execution. The changes address concurrent access issues in multitask testing and modernize the pytest configuration for better developer experience.
Key changes:
- Fixed race condition in multitask access test by adding spinlock protection for shared counter
- Converted pytest markers to parametrize format for easier local test execution
- Added comprehensive README documentation for the test application
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pytest_runtime_config.py | Migrated from individual @pytest.mark decorators to @pytest.mark.parametrize for target specification |
| test_multitask_access.c | Added spinlock to protect concurrent access to nb_of_success counter and improved task synchronization |
| README.md | Added new documentation explaining test purpose, structure, tags, and usage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
device/esp_tinyusb/test_apps/runtime_config/main/test_multitask_access.c
Outdated
Show resolved
Hide resolved
9a60cda to
86f222d
Compare
86f222d to
c0e3d66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
device/esp_tinyusb/test_apps/runtime_config/main/test_multitask_access.c
Outdated
Show resolved
Hide resolved
device/esp_tinyusb/test_apps/runtime_config/main/test_multitask_access.c
Show resolved
Hide resolved
c0e3d66 to
aed6a02
Compare
device/esp_tinyusb/test_apps/runtime_config/main/test_multitask_access.c
Show resolved
Hide resolved
|
Hi This PR is ready, PTAL. |
device/esp_tinyusb/test_apps/runtime_config/main/test_multitask_access.c
Outdated
Show resolved
Hide resolved
device/esp_tinyusb/test_apps/runtime_config/main/test_multitask_access.c
Outdated
Show resolved
Hide resolved
aed6a02 to
6ec9db1
Compare
igi540
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall. I left one minor nitpick in an inline comment.
device/esp_tinyusb/test_apps/runtime_config/main/test_multitask_access.c
Outdated
Show resolved
Hide resolved
6ec9db1 to
0c6cf9e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on December 20
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
device/esp_tinyusb/test_apps/runtime_config/main/test_multitask_access.c
Outdated
Show resolved
Hide resolved
…ss test - Fixed concurrent access to the nb_of_success var from several tasks - Changed the logic of getting notification from the worker thread to sem_done - Added the test case for uninstall the tinyusb driver via several tasks - Added README.md
0c6cf9e to
5ac149b
Compare
Description
This is the first part of the plan: Refactor the runtime_config test application
nb_of_successvar from several taskssem_doneinstead of direct notificationruntime_configtest applicationRelated
Testing
"Multitask: Uninstall" [runtime_config][default]Checklist
Before submitting a Pull Request, please ensure the following:
Note
Refactors TinyUSB runtime_config multitask tests to use semaphore-based synchronization with thread-safe counters, adds a new uninstall test, and introduces a README.
runtime_config/main/test_multitask_access.c):sem_done; tasks start via notifications and wait withxSemaphoreTake.volatile+ spinlock) and track task handles for coordinated start.test_init_phy()andtest_deinit_phy()."Multitask: Uninstall"tagged[runtime_config][default].[default], improve assertions and task creation (pdPASS).README.mddescribing the runtime_config test app, focus areas, and tag taxonomy.Written by Cursor Bugbot for commit 5ac149b. This will update automatically on new commits. Configure here.