Skip to content

logging: Use k_uptime_get_32 for high frequency system clock #14074

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

Merged
merged 1 commit into from
Mar 6, 2019

Conversation

nordic-krch
Copy link
Collaborator

If system clock runs fast k_cycle_get_32(), which is the timestamp
source, wraps often (few minutes). This patch changes default
timestamp function to use k_uptime_get_32() if system clock
frequency is higher than 1 MHz and k_cycle_get_32() otherwise.

If system clock runs at 1 MHz, counter will wrap every 71.5 minutes.

Fixes #9043.
Fixes #14010.

Signed-off-by: Krzysztof Chruscinski [email protected]

@jhedberg
Copy link
Member

jhedberg commented Mar 5, 2019

Don't you also need to pass a different value to log_output_timestamp_freq_set() so the new value gets interpreted correctly?

@codecov-io
Copy link

codecov-io commented Mar 5, 2019

Codecov Report

Merging #14074 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14074      +/-   ##
==========================================
+ Coverage   52.61%   52.61%   +<.01%     
==========================================
  Files         307      307              
  Lines       45472    45473       +1     
  Branches    10530    10530              
==========================================
+ Hits        23924    23925       +1     
  Misses      16666    16666              
  Partials     4882     4882
Impacted Files Coverage Δ
subsys/logging/log_core.c 37.45% <100%> (+0.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45c6ead...dd57136. Read the comment docs.

If system clock runs fast k_cycle_get_32(), which is the timestamp
source, wraps often (few minutes). This patch changes default
timestamp function to use k_uptime_get_32() if system clock
frequency is higher than 1 MHz and k_cycle_get_32() otherwise.

If system clock runs at 1 MHz, counter will wrap every 71.5 minutes.

Signed-off-by: Krzysztof Chruscinski <[email protected]>
@nordic-krch
Copy link
Collaborator Author

@jhedberg

Don't you also need to pass a different value to log_output_timestamp_freq_set() so the new value gets interpreted correctly?

Good point. Fixed.

@@ -298,7 +305,7 @@ void log_core_init(void)

/* Set default timestamp. */
timestamp_func = timestamp_get;
log_output_timestamp_freq_set(CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC);
log_output_timestamp_freq_set(freq);
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we actually make this a lot simpler by not requiring a separate static function at all:

if (CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC > 1000000)
        timestamp_func = k_uptime_get_32;
        log_output_timestamp_freq_set(MSEC_PER_SEC);
} else {
        timestamp_func = k_cycle_get_32;
        log_output_timestamp_freq_set(CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC);
}

I'll leave it up to you to decide if it makes sense/is worth it.

@carlescufi carlescufi merged commit fb4f5e7 into zephyrproject-rtos:master Mar 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants