nuttxpr commented on PR #2621: URL: https://github.com/apache/nuttx-apps/pull/2621#issuecomment-2381070406
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) **Summary:** The PR summary is generally okay but could be more detailed. For example: * **Why is 'dup2' support necessary for the crypto driver?** What problem does it solve? * **What kind of illegal parameter detection is being added for AES, and why is it important?** * **Adding new algorithms is a significant change. What are the specific benefits of adding ripemd160/poly1305/bignum for an alternative mbedtls implementation?** **Impact:** * **"N/A" is likely not accurate.** Even if there's no direct user impact, there are almost always impacts to consider for changes of this nature. Think about: * **Build:** Do the new algorithms or crypto driver changes require any new dependencies or build flags? * **Hardware:** Are these changes specific to certain architectures or boards? * **Documentation:** Will any documentation need to be added or updated to explain the new features or changes? * **Security:** Security-related changes should always be carefully assessed. Are there any potential security implications of the new algorithms or changes to the crypto driver? * **Compatibility:** Will these changes affect compatibility with existing applications or systems using the previous crypto implementation? **Testing:** * While mentioning CI and test suites is good, providing more specific details will make the review process smoother: * **Build Host(s):** Specify the operating systems, CPUs, and compiler versions used for testing. * **Target(s):** List the specific architectures, boards, and configurations used for testing. * **Testing Logs:** Instead of just saying "testing logs here," provide snippets of relevant logs demonstrating the issue before the change and the successful outcome after the change. Focus on the most critical parts. **Overall:** The PR does not fully meet the NuttX requirements yet. It needs more specific information in the Summary and Impact sections and more detailed evidence of testing. -- 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]
