nuttxpr commented on PR #15459: URL: https://github.com/apache/nuttx/pull/15459#issuecomment-2576616433
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR does not fully meet the NuttX requirements as described. While it fills out the template, the content is lacking crucial detail and appears contradictory. Here's why: * **Summary is insufficient:** "adding SPI macros" isn't descriptive enough. *Which* macros? What functionality do they enable? Why were they missing? The summary should clearly articulate the problem being solved. Saying "there is no SPI of stm32g4 series" contradicts the statement that this isn't a new feature. If there was no SPI support before, and now there is some level of support, that IS a new feature (or at least enabling an existing driver for a new target). * **Impact is unclear and likely inaccurate:** Claiming NO impact across the board is highly suspicious for a change adding support for a new microcontroller series. At a minimum, there should be an impact on hardware (stm32g4 is now supported) and likely on documentation (if this enables a previously unusable feature). The "backward/forward/interoperability" YES is unexplained, which is a problem. * **Testing is inadequate:** Simply stating the host OS and compiler version is not enough. What board was used? What configuration? What tests were run *before* and *after* the change? The required "testing logs" sections are empty. Without showing how the change was validated, the PR is incomplete. Even basic "before: SPI didn't work, after: SPI works" log snippets would be better than nothing. **How to improve the PR:** 1. **Clarify the Summary:** Explain precisely which macros were added and why. Clearly state if this enables SPI functionality where it was previously unavailable. If this fixes a bug, describe the bug and how the fix addresses it. 2. **Re-evaluate the Impact:** Be honest about the changes. This almost certainly impacts hardware and potentially documentation. Explain the "backward/forward/interoperability" impact clearly. 3. **Provide Real Testing Data:** Include logs or test results demonstrating the functionality before and after the change. Specify the exact target board and configuration used for testing. Describe the test procedure. Example of improved testing section: ``` ## Testing I confirm that changes are verified on local setup and works as intended: * Build Host(s): Ubuntu 20.04, x86_64, arm-none-eabi-gcc (15:9-2019-q4-0ubuntu1) 9.2.1 * Target(s): stm32g474re-nucleo, using the `nsh` configuration. Testing logs before change: ``` nsh> spi info spi0: No such device ``` Testing logs after change: ``` nsh> spi info spi0: Mode: 0 Bits/word: 8 ``` Tested by configuring the SPI driver and verifying communication with an external device (specify the device). ``` By providing more detail and evidence, the PR will be much more likely to be accepted. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
