nuttxpr commented on PR #3000: URL: https://github.com/apache/nuttx-apps/pull/3000#issuecomment-2662775885
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR description, while a good start, doesn't fully meet the NuttX requirements. Here's a breakdown and suggestions for improvement: **Missing/Insufficient Information:** * **Summary:** While you explain the *why*, you need to be more explicit about *what* changed and *how*. Mention specific files modified or added. For example: "This PR adds a `cyclictest` utility under `tools/testing/cyclictest`. It provides alternative waiting mechanisms using semaphores (when `CONFIG_SYSTEMTICK_HOOK` is enabled) and timer devices, along with measurement options using `clock_gettime` and timer devices. This allows for more accurate latency measurements than the existing patch in `benchmarks/rt-tests`." * **Impact:** Be more specific and answer *all* the questions with YES/NO and explanations. Your current statement is vague. Examples: * **Is new feature added?** YES (cyclictest utility) * **Is existing feature changed?** NO * **Impact on user:** YES (Users can now utilize a dedicated cyclictest utility with enhanced features) * **Impact on build:** YES (Need to add `tools/testing/cyclictest` to the build system - describe how, e.g., a new Kconfig option) * **Impact on hardware:** Potentially YES (if using the timer device method - specify which timers are supported). If no specific hardware changes are needed, explicitly state NO and briefly explain why. * **Impact on documentation:** YES (Documentation needs to be added explaining how to build and use the new cyclictest utility, including configuration options). * **Impact on security:** NO (Provide a brief justification. E.g., "No new system calls or privileged operations are introduced.") * **Impact on compatibility:** NO (Provide a brief justification. E.g., "No breaking changes to existing APIs.") * **Testing:** Provide much more detail. "Locally on a custom ATSAMV7 board" is not sufficient. * **Build Host(s):** Specify OS version, CPU architecture, compiler version (e.g., "Linux Ubuntu 20.04, x86_64, GCC 9.4.0"). * **Target(s):** Be specific about the ATSAMV7 board and its configuration (e.g., "ATSAMV71-XULT, configured with CONFIG_SYSTEMTICK_HOOK=y and using Timer/Counter 0"). * **Testing logs:** You *must* include actual log output demonstrating the cyclictest's functionality, both before (showing the limitations of the existing approach or the absence of the tool) and after the change. Show different test cases (different wait/measurement methods). Even a few lines demonstrating successful execution and basic output would be better than nothing. **Revised PR Description Example (incorporating the above suggestions):** ``` ## Summary This PR adds a `cyclictest` utility under `tools/testing/cyclictest`. It provides alternative waiting mechanisms using semaphores (when `CONFIG_SYSTEMTICK_HOOK` is enabled) and timer devices, along with measurement options using `clock_gettime` and timer devices. This allows for more accurate latency measurements than the existing patch in `benchmarks/rt-tests`. Files added: `tools/testing/cyclictest/cyclictest.c`, `tools/testing/cyclictest/Kconfig`, `tools/testing/cyclictest/Makefile`. ## Impact * **Is new feature added?** YES (cyclictest utility) * **Is existing feature changed?** NO * **Impact on user:** YES (Users can now utilize a dedicated cyclictest utility with enhanced features for measuring latency.) * **Impact on build:** YES (A new Kconfig option `CONFIG_TOOLS_CYCLICTEST` has been added to enable building the cyclictest utility.) * **Impact on hardware:** YES (The timer device wait method utilizes Timer/Counter 0. This could potentially conflict with other uses of this timer.) * **Impact on documentation:** YES (Documentation has been added to the `tools/testing` directory explaining how to build and use the cyclictest utility, including its configuration options.) * **Impact on security:** NO (No new system calls or privileged operations are introduced.) * **Impact on compatibility:** NO (No breaking changes to existing APIs.) ## Testing I confirm that changes are verified on a local setup and work as intended: * **Build Host(s):** Linux Ubuntu 22.04, x86_64, GCC 11.2.0 * **Target(s):** ATSAMV71-XULT, configured with `CONFIG_SYSTEMTICK_HOOK=y` and `CONFIG_TOOLS_CYCLICTEST=y`, using Timer/Counter 0. Testing logs before change (demonstrating lack of dedicated cyclictest and limitations of existing methods): ``` # No cyclictest utility found in NuttX $ cyclictest bash: cyclictest: command not found # Using existing inadequate method (e.g., from benchmarks/rt-tests - provide a concrete example) ... (output showing limitations in precision or functionality) ... ``` Testing logs after change: ``` # Using semaphore wait method $ cyclictest -p 90 -d 0 -n -h 0 -t 1 -i 200000 -l 1000 # ... output showing consistent latency measurements ... # Using timer device wait method $ cyclictest -p 90 -d 1 -n -h 0 -t 1 -i 200000 -l 1000 # ... output showing consistent latency measurements ... ``` ``` By providing this level of detail, you significantly increase the chances of your PR being accepted quickly. Remember to adapt this example to accurately reflect your specific changes and testing results. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
