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

nxgdb/irq: add irqinfo command #15743

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

XuNeo
Copy link
Contributor

@XuNeo XuNeo commented Feb 2, 2025

Note: Please adhere to Contributing Guidelines.

Summary

Similar to nsh command, irqinfo shows the registered IRQ callbacks and arguements.
Arguements is shown as a function when possible.
If intrumentation is enabled, it also prints the IRQ count etc.

(gdb) irqinfo
IRQ  COUNT      TIME   RATE   HANDLER                                          ARGUMENT
0    0          0      N/A    mps_reserved                             0x0 <sensor_unregister>
2    0          0      N/A    mps_nmi                                  0x0 <sensor_unregister>
3    0          0      N/A    arm_hardfault                            0x0 <sensor_unregister>
4    0          0      N/A    arm_memfault                             0x0 <sensor_unregister>
5    0          0      N/A    arm_busfault                             0x0 <sensor_unregister>
6    0          0      N/A    arm_usagefault                           0x0 <sensor_unregister>
11   1          0      N/A    arm_svcall                               0x0 <sensor_unregister>
12   0          0      N/A    arm_dbgmonitor                           0x0 <up_debugpoint_remove>
14   0          0      N/A    mps_pendsv                               0x0 <up_debugpoint_remove>
15   6581421    0      N/A    systick_interrupt                        0x100010c <g_systick_lower>
49   2          0      N/A    uart_cmsdk_tx_interrupt                  0x1000010 <g_uart0port>
50   0          0      N/A    uart_cmsdk_rx_interrupt                  0x1000010 <g_uart0port>
59   2          0      N/A    uart_cmsdk_ov_interrupt                  0x1000010 <g_uart0port>
(gdb)

Impact

New feature.

Testing

Tested with qemu live debugging.

@github-actions github-actions bot added Area: Tooling Size: M The size of the change in this PR is medium labels Feb 2, 2025
@nuttxpr
Copy link

nuttxpr commented Feb 2, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to mostly meet the NuttX requirements, but could be improved. Here's a breakdown:

Strengths:

  • Clear Summary: The summary explains the purpose, functionality, and how it works. The example output is very helpful.
  • Impact Section Mostly Addressed: The impact section is started, and the new feature is identified.
  • Testing Evidence Provided: Testing information, including the platform and method (qemu/gdb) is present. This is good.

Weaknesses and Areas for Improvement:

  • Incomplete Impact Assessment: While the "new feature" is mentioned, all other impact fields are left blank with "NO" or unanswered. Even if there's no impact, explicitly stating "NO" and briefly explaining why is helpful (e.g., "Impact on build: NO - This change only affects the gdb stub and does not modify the build system."). This demonstrates that the impact has been considered.
  • Missing "Before" and "After" Logs: The testing section includes placeholders for logs but doesn't provide actual logs. This makes it difficult to verify the change's effectiveness. Show the irqinfo output before your change and after your change.
  • Missing Build Host Details: Be more specific about your build host. "Linux" isn't enough. Include the distribution (e.g., "Ubuntu 22.04"), the compiler version (e.g., "gcc 11.3.0"), and any other relevant details.
  • Missing Target Details: Be more specific about your target. "qemu" is a good start, but what architecture? (e.g., "qemu-system-arm"). Which board configuration? This is essential for reproducibility.
  • Consider User Impact: Even though it's a debugging command, consider if there is any user impact. For example, does it consume more memory? Does it introduce any potential performance overhead, even if negligible?

Recommendation:

To fully meet the requirements, fill in the missing details in the Impact and Testing sections. Provide concrete "before" and "after" logs demonstrating the change's functionality. Be as specific as possible about your build and target environments. Even if the answer is "NO" for most impact items, provide a brief justification. This makes the PR review much easier and increases the likelihood of it being accepted.

@XuNeo XuNeo force-pushed the nxgdb-irqinfo-command branch from 5b85368 to fddac21 Compare February 2, 2025 13:43
Copy link
Contributor

@jerpelea jerpelea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add commit description

@linguini1
Copy link
Contributor

Could you add some information about this to the documentation website? Maybe here?

@XuNeo XuNeo force-pushed the nxgdb-irqinfo-command branch from fddac21 to 4a7f8ce Compare February 5, 2025 02:42
@github-actions github-actions bot added the Area: Documentation Improvements or additions to documentation label Feb 5, 2025
XuNeo and others added 4 commits February 5, 2025 10:43
1. Add method to check if an object has specified field.
2. Fix Array iterator that walrus expression should store result of the
   function, instead of the compare result.

Note that walrus operation has lowest priority except ','.

Signed-off-by: xuxingliang <[email protected]>
(gdb) irqinfo
IRQ  COUNT      TIME   RATE   HANDLER                                          ARGUMENT
0    0          0      N/A    mps_reserved                             0x0 <sensor_unregister>
2    0          0      N/A    mps_nmi                                  0x0 <sensor_unregister>
3    0          0      N/A    arm_hardfault                            0x0 <sensor_unregister>
4    0          0      N/A    arm_memfault                             0x0 <sensor_unregister>
5    0          0      N/A    arm_busfault                             0x0 <sensor_unregister>
6    0          0      N/A    arm_usagefault                           0x0 <sensor_unregister>
11   1          0      N/A    arm_svcall                               0x0 <sensor_unregister>
12   0          0      N/A    arm_dbgmonitor                           0x0 <up_debugpoint_remove>
14   0          0      N/A    mps_pendsv                               0x0 <up_debugpoint_remove>
15   6581421    0      N/A    systick_interrupt                        0x100010c <g_systick_lower>
49   2          0      N/A    uart_cmsdk_tx_interrupt                  0x1000010 <g_uart0port>
50   0          0      N/A    uart_cmsdk_rx_interrupt                  0x1000010 <g_uart0port>
59   2          0      N/A    uart_cmsdk_ov_interrupt                  0x1000010 <g_uart0port>
(gdb)

Signed-off-by: xuxingliang <[email protected]>
The NuttX GDB python plugin has been moved to tools/pynuttx/nxgdb.
Update all documentation including this path.

Signed-off-by: Neo Xu <[email protected]>
Add a table of commands we extend using GDB python API.
Add irqinfo command documentation.

Signed-off-by: Neo Xu <[email protected]>
@XuNeo XuNeo force-pushed the nxgdb-irqinfo-command branch from 4a7f8ce to 2c93e0d Compare February 5, 2025 02:44
@XuNeo
Copy link
Contributor Author

XuNeo commented Feb 5, 2025

please add commit description

Done

Could you add some information about this to the documentation website? Maybe here?

Done, added to GDB with Python with a new table of commands. Currently only includes irqinfo.

@XuNeo
Copy link
Contributor Author

XuNeo commented Feb 6, 2025

Hi @jerpelea ,

Please help to review the PR again. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Documentation Improvements or additions to documentation Area: Tooling Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants