core/debug.h: refactor functions, implement automatic prefixing - #22525
core/debug.h: refactor functions, implement automatic prefixing#22525mguetschow wants to merge 13 commits into
Conversation
Just retested on that hardware, works as expected after increasing the |
|
considering to replace the function separator |
I think the way you're doing it right now is fine. Perhaps I'd prefer I think this would make it a lot easier to tell apart log lines that don't have a prefix but a func or thread from those with a prefix/subsystem. Right now, you could confuse one line's prefix with the func or thread of another |
There was a problem hiding this comment.
my thoughts, summarised. not necessarily required to merge this PR but perhaps still worth considering
- Love this. Extremely helpful for
unicoaplogs - Future work: enable logging based on prefix
- Future work: same stuff for log.h
- Would be nice to harmonise coloured logging with log.h, and to think about how the log level would be expressed visually
- Perhaps worth reconsidering the format, see above
- Can you make the prefix bold please
- Prompt Engineering Bro Max: would it be a lot of effort to let users change the colour of individual parts using env vars?
- Can we infer a default prefix from MODULE
- Multiple prefixes per file through some underlying macros that accept the prefix as an additional parameter?
Agree those would be nice for the future. In general reconsidering
Like your proposal, will adapt.
Can do.
What do you mean with individual parts? prefix vs func vs thread? What's the use-case? I'd leave this for a potential follow-up in any case.
Potential Follow-up
Good idea, will try how that works out. |
As something for a follow up, perhaps folks want to set individual colors/styles for prefix, thread, func, time, and separator individually -- like a bash prompt. but these sort of kindergarten games we can do later I guess, if at all. |
Those two are now done. I think this is ready for another round of review. I will rework the git history when we agree on a solution. |
backwards-compatible by not changing any behavior as long as DEBUG_PREFIX is unset or empty set a prefix per file by defining DEBUG_PREFIX enable prefixing every call to debug functions with the function name and/or the thread name via configuration options CONFIG_DEBUG_SHOW_FUNC and CONFIG_DEBUG_SHOW_FUNC
6d4c802 to
2a26e01
Compare
|
Squashed and rebased on top of #22621 Ready for final review? |
|
I was just thinking... either we add a config guard to the ansi style header or at every call site, so including debug.h, we require a check for a config macro that toggles ansi escape sequence support. I think there are valid reasons to turn off colour:
|
I am strongly in favor of the config guard in the ansi style header instead of the extra burden on the call site. But agree that a toggle makes sense. |
|
Let's smuggle the config guard in there as part of this PR? I mean as the first impl using ANSI styling Id argue it's justified |
|
I'd suggest |
crasbe
left a comment
There was a problem hiding this comment.
I didn't look through all the changed files where the DEBUG macros were updated.
| /* this would typically be set application-wide via Kconfig or make */ | ||
| #define CONFIG_DEBUG_SHOW_THREAD 1 | ||
|
|
||
| #define ENABLE_DEBUG 1 |
There was a problem hiding this comment.
Can we add something to stop Coccinelle complaining about ENABLE_DEBUG being set to 1? To avoid adding warnings to our static tests? 🤔
|
|
see 8a7ba3b |
|
took the liberty of changing debug logging and docs: 9041f0c. goal: make all of this more customisable.
naming debatable.
|
|
@carl-tud while this indeed makes it more customisable, it seems a bit confusing as well with all those different macros: I wonder if we could get rid of Also, wouldn't it be the more likely usecase wanting to redirect debug logging from all files? Isn't it then a bit cumbersome to add a custom debug function to all files where |
Hm, I would like to have two levels of indirection:
Taken together, you can define a new DEBUG/DEBUG_PUTS macro, either to something completely different, or use the existing The
You can do that by putting
We can but thought it would be nice to think debug.h through before merging given the 'refactored' claim. And perhaps What should come in a separate PR is some harmonisation of #define LOG_UNIT "this.test"
#include "log.h"
#define ENABLE_DEBUG 1
#include "debug.h"
LOG_ERROR(format, value, string);
LOG_WARNING(format, value, string);
LOG_INFO(format, value, string);
LOG_DEBUG(format, value, string);
DEBUG(format, value, string);
|
| #define DEBUG_(prefix, ...) \ | ||
| do { \ | ||
| if (ENABLE_DEBUG && __debug_sufficient_stack(true)) { \ | ||
| if (strlen(prefix) > 0) { \ |
There was a problem hiding this comment.
This can be a sizeof instead of a strlen if we proceed to use static string literal concatenation anway



Contribution description
This PR improves printf debugging with the
debug.hfile. It adds an automatic prefix to every printout with"")<isr>).It is backwards-compatible by not changing the behavior as long as
DEBUG_PREFIXis unset in a file.I also sneaked in some more fixes to the
debug.hfile and some nice coloring for the prefix prints.Testing procedure
Apply the following diff
and see it working with
I've tested it on
nativeand I think I remember to have tested it on annrf52840dk, too.Issues/PRs references
This has helped me in the past to debug issues such as #21843
Declaration of AI-Tools / LLMs usage:
AI-Tools / LLMs that were used are: