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

sched/clock: call up_timer_gettime() to get higher resolution #15139

Merged
merged 1 commit into from
Dec 12, 2024

Conversation

GUIDINGLI
Copy link
Contributor

Summary

This is continue work of #15038

sched/clock: call up_timer_gettime() to get higher resolution
driver/timers: remove wall clock diff from lower half
Some SOC chipsets use global clock generator to unify the time of each MCU,
that means after MCUs is start-up, the timer will not start counting from 0.
this PR will record the wall timer of global timer and recalculate the ticks.

Impact
N/A

Testing
ci-check

@github-actions github-actions bot added Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Dec 11, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 11, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements as described. While it provides a link to prior work and a brief explanation, it lacks crucial details.

Here's what's missing:

  • Summary: Needs more detail. How does up_timer_gettime() provide higher resolution? What is the "wall clock diff" and why is its removal important? The explanation about the global clock generator is helpful, but more detail on the recalculation of ticks is needed.
  • Impact: Simply stating "N/A" is insufficient. Consider each point individually. Even if there's no impact, explicitly state "NO" for each category. The changes described likely impact hardware and drivers, so claiming no impact is questionable.
  • Testing: "ci-check" is not sufficient. Provide actual log output before and after the change, demonstrating the improvement. Also specify the Build Host(s) and Target(s) used for testing. Simply stating "ci-check" doesn't convey what was tested or the results.

The PR needs substantial improvement in providing specific details for each of the required sections to meet the NuttX guidelines.

@anchao
Copy link
Contributor

anchao commented Dec 12, 2024

I have made similar changes before, but the problems are:

  1. system tick will not start from 0, which will affect programs that need to use CLOCK_MONOTONIC
  2. The acquisition of ticks will consume more CPU cycles (g_system_ticks -> up_timer_gettime and clock_ticks2time)
  3. g_system_ticks may no need any more and which will to be removed in most scenarios

@anchao
Copy link
Contributor

anchao commented Dec 12, 2024

let us keep moving, @GUIDINGLI please fix the build break

Copy link
Contributor

@anchao anchao left a comment

Choose a reason for hiding this comment

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

@xiaoxiang781216 xiaoxiang781216 merged commit 4a51c21 into apache:master Dec 12, 2024
27 checks passed
anchao added a commit to anchao/nuttx that referenced this pull request Dec 12, 2024
anchao added a commit to anchao/nuttx that referenced this pull request Dec 12, 2024
jerpelea pushed a commit that referenced this pull request Dec 12, 2024
jerpelea pushed a commit to jerpelea/nuttx that referenced this pull request Dec 13, 2024
xiaoxiang781216 pushed a commit that referenced this pull request Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants