Skip to content

Conversation

@nordic-krch
Copy link
Contributor

As suggested by @aescolar stripped down version of #10049 with LOG_PANIC() added to fault handlers. Keeping printk messages there (see discussion: #10049 (comment))

@codecov-io
Copy link

codecov-io commented Sep 19, 2018

Codecov Report

Merging #10098 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10098      +/-   ##
==========================================
- Coverage   52.48%   52.48%   -0.01%     
==========================================
  Files         213      213              
  Lines       26135    26137       +2     
  Branches     5633     5634       +1     
==========================================
- Hits        13718    13717       -1     
- Misses      10160    10162       +2     
- Partials     2257     2258       +1
Impacted Files Coverage Δ
subsys/logging/log_core.c 69.1% <0%> (-0.96%) ⬇️
arch/posix/core/fatal.c 20.68% <0%> (-0.74%) ⬇️

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 79e8f79...01dfab7. Read the comment docs.

Copy link
Member

@aescolar aescolar left a comment

Choose a reason for hiding this comment

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

Just 2 minor grammar issues


+2 for POSIX arch changes,
&+2 for logger changes
& Nothing obviously wrong when looking at other archs.

Copy link
Member

Choose a reason for hiding this comment

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

Minor grammar thing: "Returns immediately if +the + logger + is + already in panic mode."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

If you feel like correcting also this line: "On panic+, the+ logger [..]"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@nordic-krch nordic-krch force-pushed the log_panic branch 2 times, most recently from 606eb8b to 62af5d3 Compare September 19, 2018 12:34
Added LOG_PANIC to fault handlers to ensure that log is flush and
logger processes messages in a blocking way in fault handler.

Signed-off-by: Krzysztof Chruscinski <[email protected]>
Ensure that only first log_panic() sends panic signal to backends

Signed-off-by: Krzysztof Chruscinski <[email protected]>
{
struct log_backend const *backend;

if (panic_mode) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortuantely there are many paths in error handling. So i see scenarios where one fault function which called log_panic will call another one and i don't want to multiple panic calls to backends. Note that we want to enter panic mode as soon as possible to get as much logs as possible (flush as early as possible).

@nashif nashif merged commit 87d177a into zephyrproject-rtos:master Sep 27, 2018
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