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

timers: Replace DEBUGASSERT with the error code, reopen from #12484 #14619

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

jasonbu
Copy link
Contributor

@jasonbu jasonbu commented Nov 4, 2024

Summary

If the timer driver is not support both getstatus and tick_getstatus,
we should not raise a panic, return -ENOSYS will prefer.

#7494
#13606
#14514

Impact

when timer driver no support for get status, will no longer panic, just return a -ENOSYS.

Testing

CI-test & local stm32h7

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

nuttxpr commented Nov 4, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although the provided information is minimal. It would benefit from more detail, especially in the Testing section.

Here's a breakdown of why it mostly meets the requirements and where it could be improved:

Strengths:

  • Clear Summary: The summary explains the "why," "what," and "how" of the change, referencing relevant NuttX issues.
  • Addresses a Specific Issue: The PR targets a panic condition, which is a clear improvement.
  • Impact is Described (though briefly): The impact on the user (no panic, error code returned) is stated.

Weaknesses & Areas for Improvement:

  • Impact Lacks Detail: While the user impact is mentioned, the other impact categories are either marked "NO" implicitly or lack detail. Even if there's no impact, explicitly stating "NO" for each category is good practice. For example:

    • Impact on build: NO
    • Impact on hardware: NO
    • Impact on documentation: YES (If the return code behavior is changed, the documentation should be updated to reflect this.)
    • Impact on security: NO (But consider if this change could have indirect security implications. Even if unlikely, it's good to think about it and document the reasoning.)
    • Impact on compatibility: Potentially YES. Returning -ENOSYS instead of panicking is a behavioral change. While arguably an improvement, it's still a change that could affect existing code that might have relied on the panic behavior (however unlikely). This needs further discussion.
  • Testing is Insufficient: "CI-test & local stm32h7" is not enough detail.

    • Build Host(s): Specify the OS, CPU architecture, and compiler used for your local build.
    • Target(s): Be more specific about the STM32H7 board and configuration.
    • Logs: The template requires "Testing logs before change" and "Testing logs after change." Even if the logs are simple, include them. Demonstrate the panic occurring before the change and the -ENOSYS return after the change. This provides concrete evidence that the PR works as intended. Example:
    Testing logs before change (stm32h7-yourconfig):
    PANIC!!! assert failed at file: timer/clock_gettime.c line: 123
    
    Testing logs after change (stm32h7-yourconfig):
    Returned -ENOSYS from clock_gettime()
    

In short: While the core information is present, the PR needs more thoroughness in the Impact and Testing sections to meet the NuttX requirements fully. Adding the missing details will significantly improve the review process and increase confidence in the change.

@xiaoxiang781216 xiaoxiang781216 linked an issue Nov 4, 2024 that may be closed by this pull request
1 task
@xiaoxiang781216
Copy link
Contributor

@jasonbu let's fix all callback, you can cherry-pick this patch and fix it:
#12484

fix the issue report here:
https://lists.apache.org/thread/sys37yf63rq501fd1v8y3zyh6vk10v1d
when driver no support for ops, should not panic, prefer errno.

Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
Signed-off-by: buxiasen <buxiasen@xiaomi.com>
@jasonbu jasonbu changed the title timer/TCIOC_GETSTATUS: return -ENOSYS, no assert when no get status timers: Replace DEBUGASSERT with the error code, reopen from #12484 Nov 4, 2024
@jerpelea jerpelea merged commit a96a4de into apache:master Nov 4, 2024
27 checks passed
@jasonbu jasonbu deleted the timer_getstatus_fix branch November 5, 2024 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Drivers Drivers 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.

5 participants