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

fix compile error #15441

Merged
merged 2 commits into from
Jan 8, 2025
Merged

fix compile error #15441

merged 2 commits into from
Jan 8, 2025

Conversation

hujun260
Copy link
Contributor

@hujun260 hujun260 commented Jan 7, 2025

Summary

fix compile error
Configuration/Tool: sim/segger
In file included from segger/SystemView/SEGGER/SEGGER_SYSVIEW.h:65,
from segger/SystemView/SEGGER/SEGGER_SYSVIEW_Int.h:64,
from segger/SystemView/SEGGER/SEGGER_SYSVIEW.c:146:
segger/SystemView/SEGGER/SEGGER_SYSVIEW.c: In function '_VPrintHost': Error: segger/SystemView/SEGGER/SEGGER_SYSVIEW_ConfDefaults.h:557:51: error: implicit declaration of function 'SEGGER_RTT_LOCK'; did you mean 'SEGGER_RTT_Read'? [-Werror=implicit-function-declaration]
557 | #define SEGGER_SYSVIEW_LOCK() SEGGER_RTT_LOCK()
| ^~~~~~~~~~~~~~~
segger/SystemView/SEGGER/SEGGER_SYSVIEW.c:387:35: note: in expansion of macro 'SEGGER_SYSVIEW_LOCK'
387 | #define RECORD_START(PacketSize) SEGGER_SYSVIEW_LOCK();
| ^~~~~~~~~~~~~~~~~~~
segger/SystemView/SEGGER/SEGGER_SYSVIEW.c:1005:5: note: in expansion of macro 'RECORD_START'
1005 | RECORD_START(SEGGER_SYSVIEW_INFO_SIZE + SEGGER_SYSVIEW_MAX_STRING_LEN + 2 * SEGGER_SYSVIEW_QUANTA_U32 + SEGGER_SYSVIEW_MAX_ARGUMENTS * SEGGER_SYSVIEW_QUANTA_U32);
| ^~~~~~~~~~~~
Error: segger/SystemView/SEGGER/SEGGER_SYSVIEW_ConfDefaults.h:570:51: error: implicit declaration of function 'SEGGER_RTT_UNLOCK'; did you mean 'SEGGER_RTT_Init'? [-Werror=implicit-function-declaration]
570 | #define SEGGER_SYSVIEW_UNLOCK() SEGGER_RTT_UNLOCK()
| ^~~~~~~~~~~~~~~~~
segger/SystemView/SEGGER/SEGGER_SYSVIEW.c:390:35: note: in expansion of macro 'SEGGER_SYSVIEW_UNLOCK'
390 | #define RECORD_END() SEGGER_SYSVIEW_UNLOCK()
| ^~~~~~~~~~~~~~~~~~~~~
segger/SystemView/SEGGER/SEGGER_SYSVIEW.c:1015:5: note: in expansion of macro 'RECORD_END'
1015 | RECORD_END();

Note: Please adhere to Contributing Guidelines.

Impact

none

Testing

ci

@github-actions github-actions bot added Area: Drivers Drivers issues Size: S The size of the change in this PR is small labels Jan 7, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 7, 2025

[Experimental Bot, please feedback here]

No, this PR does not meet the NuttX requirements. Here's why and how to fix it:

Missing Information:

  • Summary: While it mentions a compile error, it lacks crucial details:
    • Why is the change necessary? Simply stating "fix compile error" is insufficient. Explain why the error occurs. Was a dependency updated? Was there a logic flaw?
    • What functional part is changed? Be specific. Is it related to SystemView integration? RTT? A specific driver?
    • How does the change work? What code was added/modified to resolve the error? Just providing the error message isn't helpful.
  • Impact: Even if you believe there's no user-facing impact, explicitly state "NO" for each impact category. If the fix involves including new header files or changes linking, the build might be impacted. Review carefully.
  • Testing: "ci" is inadequate. Provide details about your local testing. Did you run the simulator? On what host? What commands did you use to build and test? Include relevant snippets of the build output (success/failure) before and after the change. CI results are supplemental to your local testing, not a replacement for it.

How to Improve the PR:

  1. Explain the root cause: Why was SEGGER_RTT_LOCK undeclared? Was a header file missing? Was a configuration option incorrect?
  2. Describe the solution: What did you change to fix the error? Did you add a header file? Did you modify a configuration setting? Provide a clear, concise explanation of the code changes. Example: "Added #include <SEGGER_RTT.h> to SEGGER_SYSVIEW.c to resolve the missing declaration of SEGGER_RTT_LOCK and SEGGER_RTT_UNLOCK."
  3. Detailed Impact Assessment: Go through each impact category and explicitly state "NO" or "YES" with a brief explanation if "YES". Example: "Impact on build: YES. Requires the SEGGER_RTT library to be linked."
  4. Thorough Testing Information:
    • Build Host: Specify your OS, CPU architecture, and compiler version. Example: "Linux x86_64, GCC 12.2.0"
    • Target: Specify the target architecture, board, and configuration. Example: "sim:nsh"
    • Logs: Provide actual build logs and test output before and after the change, even if it's just a successful build. Focus on the relevant parts. Don't just paste entire log files.

Example of a better Summary:

"Fixes a compile error in the SystemView integration for the sim/segger configuration caused by missing declarations for SEGGER_RTT_LOCK and SEGGER_RTT_UNLOCK. The error occurred because SEGGER_RTT.h was not included. This change adds the necessary include directive to SEGGER_SYSVIEW.c to resolve the issue."

By providing more complete information, you'll make it easier for reviewers to understand and approve your PR.

drivers/segger/note_sysview.c Outdated Show resolved Hide resolved
drivers/segger/config/SEGGER_RTT_Conf.h Outdated Show resolved Hide resolved
drivers/segger/config/SEGGER_RTT_Conf.h Outdated Show resolved Hide resolved
@anchao
Copy link
Contributor

anchao commented Jan 7, 2025

@hujun260 If you continue to make changes to the kernel code without any testing, I will have to reject all future submissions for merging into the community !

…er/config/SEGGER_RTT_Conf.h"

This reverts commit 5e7c640.

Signed-off-by: hujun5 <[email protected]>
@github-actions github-actions bot added the Area: OS Components OS Components issues label Jan 7, 2025
Error: /github/workspace/sources/nuttx/include/nuttx/spinlock_type.h:57:3: error: unknown type name 'atomic_t'
368   57 |   atomic_t owner;
369      |   ^~~~~~~~
370Error: /github/workspace/sources/nuttx/include/nuttx/spinlock_type.h:58:3: error: unknown type name 'atomic_t'
371   58 |   atomic_t next;
372      |   ^~~~~~~~

Signed-off-by: hujun5 <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit fabf4a5 into apache:master Jan 8, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Drivers Drivers issues Area: OS Components OS Components issues Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] at32f437-mini/systemview: builds fail with error: implicit declaration of function 'SEGGER_RTT_LOCK'
5 participants