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

examples: fix coding standard issues for nimble_blecent and nimble_bleprpt #3043

Merged
merged 2 commits into from
Mar 30, 2025

Conversation

raiden00pl
Copy link
Member

Summary

  1. examples/nimble_bleprph: fix various coding standard issues and fix compilation for make

  2. examples/nimble_blecent: fix various coding standard issues and fix compilation for make

Impact

fix various coding standard issues, no functional changes

Testing

compile nimble_bleprph and nimble_blecent with make

fix various coding standard issues for examples/nimble_blecent

also fix compilation for make

Signed-off-by: raiden00pl <[email protected]>
fix various coding standard issues examples/nimble_bleprph

also fix compilation for make

Signed-off-by: raiden00pl <[email protected]>
@nuttxpr
Copy link

nuttxpr commented Mar 28, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements, but it's missing some crucial details. While it addresses the sections, the content is too brief for a proper review.

Here's what needs improvement:

  • Summary: "Fix various coding standard issues" is vague. List the specific coding standard violations that were fixed. What was the actual compilation issue? This helps reviewers understand the scope and nature of the changes.
  • Impact: While functional changes may not exist, you should still specify NO for each impact category explicitly. For example:
    • Impact on user: NO
    • Impact on build: NO (except for fixing the compilation issue, which should be elaborated on)
    • ...etc...
  • Testing: Simply stating "compile with make" is insufficient. Provide:
    • Specific build host details: OS version, compiler version (e.g., "Ubuntu 22.04, GCC 11.3.0").
    • Specific target details: Architecture, board, configuration (e.g., "sim:qemu-arm").
    • Relevant logs: "Testing logs before/after" should show that the compilation issue is resolved and demonstrate the intended behavior (even if it's unchanged functionality). Include compiler output (errors before, successful compilation after) and, ideally, runtime output demonstrating the examples still work.

Example of Improved Content:

Summary:

  1. examples/nimble_bleprph: Fixed coding standard violations related to indentation (using tabs instead of spaces), variable naming (using underscores instead of camelCase), and missing comments. The compilation issue was caused by an incorrect include path for a header file (xyz.h).

  2. examples/nimble_blecent: Similar coding standard violations as above were fixed (indentation, naming). The compilation issue stemmed from a missing definition of a macro (XYZ_DEFINE).

Impact:

  • Impact on user: NO
  • Impact on build: YES (Previously, nimble_bleprph and nimble_blecent failed to compile due to the mentioned include and macro issues. This PR fixes the build process, allowing successful compilation.)
  • Impact on hardware: NO
  • Impact on documentation: NO
  • Impact on security: NO
  • Impact on compatibility: NO

Testing:

  • Build Host: Ubuntu 22.04, GCC 11.3.0
  • Target: sim:qemu-arm

Testing logs before change (nimble_bleprph):

examples/nimble_bleprph/src/main.c:12:10: fatal error: xyz.h: No such file or directory
 #include <xyz.h>
          ^~~~~~~~
compilation terminated.
make: *** [Makefile:xyz_target] Error 1

Testing logs after change (nimble_bleprph):

... (Successful compilation output) ...

(Similar logs for nimble_blecent before and after). Ideally, include some output demonstrating the example running correctly after the fix.

By providing these details, you'll significantly increase the chances of your PR being reviewed and merged quickly.

@lupyuen lupyuen requested review from acassis and cederom March 29, 2025 02:32
@xiaoxiang781216 xiaoxiang781216 merged commit fa8daf4 into apache:master Mar 30, 2025
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants