feat(zsys): colorize the log level indicator - #57
Conversation
CONFIG_ZSYS_LOG_COLOR (default y) wraps the level token in ANSI codes --
red ERR, yellow WRN, green INF, uncolored DBG -- in both the sync ("E")
and deferred ("<ERR>") formats. Independent of ESP-IDF's
CONFIG_LOG_COLORS, which only governs ESP_LOG* output.
Backends get zsys_log_level_color() + ZSYS_LOG_COLOR_RESET rather than
zsys_log_format_msg() growing a flag parameter, so the existing public
formatter signature is unchanged.
Part of #34.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Validated on hardware (ESP32-S3, Sync ( Deferred ( This covers the two things the host suite structurally cannot reach: One cosmetic note: DBG emits a bare |
Review found the first cut silently changed the contract of a public API documented at v0.1.0: zsys_log_format_msg() is the formatter the READMEs tell custom backends to use for "UART, RTT, network, file", and it began emitting ANSI escapes into all of them with no opt-out. Split it instead. zsys_log_format_msg() is unchanged and never colors; zsys_log_format_msg_color(msg, buf, size, bool color) is the per-backend switch, mirroring Zephyr's LOG_OUTPUT_FLAG_COLORS on struct log_output. CONFIG_ZSYS_LOG_COLOR stays a global off switch over both. The console backend opts in; a file or socket backend gets what it always got. Also from review: - Document the three divergences from Zephyr's log_output.c on the declaration: ESP-IDF's non-bold codes vs upstream's bold, INF colored by default where upstream gates it behind CONFIG_LOG_INFO_COLOR_GREEN, and level-token-only wrapping vs upstream's whole-line span. - Document why an uncolored level still emits a bare reset -- both ESP-IDF and Zephyr do the same, and it clears color left set by another writer on the same UART. - Assert the color/formatter coupling against the accessor rather than a literal, so the test body is exercised under both settings of CONFIG_ZSYS_LOG_COLOR instead of only the default; cover every level and pin that the plain formatter stays colorless. - Kconfig table in components/zsys/README.md and examples/log_demo, custom-backend snippets in both, CHANGELOG entry. - LOG_LEVEL_DBG instead of a bare 4 in the sync backend bounds check. Part of #34. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Converted to draft pending one HW re-check (see bottom). Review round:
|
There was a problem hiding this comment.
Pull request overview
Adds optional ANSI/VT100 color support for zsys log level indicators, controlled by a new CONFIG_ZSYS_LOG_COLOR Kconfig option, while preserving the existing “always colorless” formatter contract for non-terminal backends.
Changes:
- Introduces
CONFIG_ZSYS_LOG_COLORplus a level→escape accessor (zsys_log_level_color) and reset token (ZSYS_LOG_COLOR_RESET). - Adds
zsys_log_format_msg_color(..., bool color)and keepszsys_log_format_msg()permanently colorless (backend-controlled coloring). - Updates the ESP console backend and adds unit tests + documentation/changelog updates.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/main/test_log.c | Adds unit coverage for colored vs. plain formatting and the level→escape accessor behavior. |
| examples/log_demo/README.md | Documents that zsys_log_format_msg() is colorless and terminals should use the color-aware formatter. |
| components/zsys/src/log.c | Implements color escape selection and the new optional-color formatter variant. |
| components/zsys/src/log_backend_esp.c | Enables color for the console backend in both deferred and sync output formats. |
| components/zsys/README.md | Documents per-backend color behavior and the new APIs/symbols. |
| components/zsys/Kconfig | Adds CONFIG_ZSYS_LOG_COLOR configuration option and help text. |
| components/zsys/include/boreas/zsys/log_backend.h | Exposes ZSYS_LOG_COLOR_RESET, zsys_log_level_color(), and zsys_log_format_msg_color(). |
| CHANGELOG.md | Notes the new default coloring behavior and new formatter API. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| (void)msg; | ||
| (void)buf; | ||
| (void)buf_size; | ||
| (void)color; | ||
| return 0; |
There was a problem hiding this comment.
Good catch, and it applied to the pre-existing zsys_log_format_msg() stub too, so both are fixed in 263512e — zsys_log_format_msg() now delegates to the color variant in the =n block as well, matching the enabled build.
Not theoretical, either: a CONFIG_ZSYS_LOG_MODULE=n linux build compiles both symbols into libzsys.a (nm shows _zsys_log_format_msg and _zsys_log_format_msg_color), so the stubs are reachable public API rather than dead code.
/* Returning 0 claims "wrote an empty string", so leave one behind --
* a caller that prints buf on a non-negative return must not read
* uninitialized memory. */
if (buf_size > 0) {
buf[0] = '\0';
}
return 0;Separately, that config exposed something unrelated to this PR: the test binary builds with CONFIG_ZSYS_LOG_MODULE=n but then hangs at runtime — the suite assumes the log module is present. Filing that on its own.
With CONFIG_ZSYS_LOG_MODULE=n both formatter stubs returned 0 -- which claims "wrote an empty string" -- while leaving buf untouched, so a caller that printed buf on a non-negative return read uninitialized memory. Terminate it. zsys_log_format_msg() now delegates to the color variant in this block too, matching the enabled build. Not theoretical: a =n build compiles both symbols into libzsys.a. Reported by Copilot on #57. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
On-HW re-validation complete (ESP32-S3, IDF v5.4) at 263512e — this re-confirms the paths that changed after the review round, in particular the deferred backend now calling Deferred ( Sync (default) — 255 Tests 0 Failures: Byte-for-byte the same output as the pre-refactor capture, so routing the console backend through the opt-in variant changed nothing on the wire. IRAM guard passes 27/27 on the same ELF. Full verification state:
Marking ready for review. |
The first cut reproduced ESP-IDF's palette, which is what #34 asked for. Reversed on review of the on-target capture: Boreas implements Zephyr's logging API, so LOG_* output should look like Zephyr's. ESP_LOG* traffic from ESP-IDF internals is left alone and keeps ESP-IDF's coloring. Verified against zephyr/subsys/logging/{log_output.c,Kconfig.formatting}: - Bold codes (LOG_COLOR_CODE_* copied verbatim, "\x1B[1;31m") rather than ESP-IDF's non-bold "\033[0;31m". - ERR red and WRN yellow only. INF and DBG are uncolored unless the new ZSYS_LOG_INFO_COLOR_GREEN / ZSYS_LOG_DBG_COLOR_BLUE are set, mirroring upstream's sub-options, which are likewise off by default. - Color spans the level indicator through the end of the message, leaving the timestamp uncolored: upstream calls color_prefix() after timestamp_print() and color_postfix() after the body. The sync format leads with the level, so there the span covers the whole line. - CONFIG_ZSYS_LOG_COLOR renamed CONFIG_ZSYS_LOG_BACKEND_SHOW_COLOR after upstream's CONFIG_LOG_BACKEND_SHOW_COLOR. Never released, so no migration note; help text is upstream's wording. The mixed console this produces is deliberate and documented on the declaration: a UART carrying both LOG_* and ESP_LOG* will show two palettes, and matching ESP-IDF would mean diverging from Zephyr for the API Boreas actually implements. Part of #34. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
On-HW confirmation of the Zephyr palette (ESP32-S3, IDF v5.4) at b8c108c. Both modes, 255 Tests 0 Failures each. Deferred — note the timestamp sits outside the color, matching upstream's Sync — the level leads this format, so the span covers the whole line: Bold Full verification state:
Two Zephyr deltas deliberately left aloneBoth predate this branch, both are format rather than color, and both would break downstream log parsers — worth deciding separately given the consumers pinned to v0.1.0:
Marking ready for review. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (3)
components/zsys/README.md:89
- The README references
CONFIG_ZSYS_LOG_COLOR=nas the global off switch, but this PR introducesCONFIG_ZSYS_LOG_BACKEND_SHOW_COLOR(and the other related symbols). As written, the docs point users to a non-existent/unused config option.
`zsys_log_format_msg_color(msg, buf, sizeof(buf), true)` instead -- the
per-backend switch Zephyr spells `LOG_OUTPUT_FLAG_COLORS`. Backends that format
the `log_msg` fields themselves can reach for `zsys_log_level_color()` and
`ZSYS_LOG_COLOR_RESET` directly. `CONFIG_ZSYS_LOG_COLOR=n` is a global off
switch over all three.
components/zsys/include/boreas/zsys/log_backend.h:164
- This note claims upstream falls back to
LOG_COLOR_CODE_DEFAULTon both prefix and postfix when a level has no color, but the current implementation returns "" for INF/DBG when those options are off (seecomponents/zsys/src/log.c), so callers won’t emit a default/reset prefix. Either adjust the docs to match the actual behavior or changezsys_log_level_color()to return a reset/default code for uncolored levels when color is enabled.
* @note A level with no color still pairs with ZSYS_LOG_COLOR_RESET, so an
* uncolored line carries a bare reset. Upstream does the same --
* color_print() falls back to LOG_COLOR_CODE_DEFAULT whenever
* colors[level] is NULL, on the prefix and the postfix both -- and it
* clears color left set by another writer on the same UART.
CHANGELOG.md:12
- Changelog says the color span leaves “the timestamp” uncolored, but in synchronous mode the ESP backend prefixes the color before the level char and resets at the end, which also colors the
(%lu)timestamp. Consider clarifying that “timestamp left uncolored” applies to the deferred[sec.ms]prefix only, or adjust the sync backend if the intent is to keep its timestamp uncolored too.
`CONFIG_LOG_BACKEND_SHOW_COLOR`) prints errors in bold red and warnings in
bold yellow, spanning the level indicator through the end of the message with
the timestamp left uncolored. INF and DBG are uncolored unless
Closes #34.
CONFIG_ZSYS_LOG_COLOR(default y) wraps the level token in ANSI codes -- red ERR, yellow WRN, green INF, uncolored DBG -- in both the sync (E (123) mod: text) and deferred ([12.345] <INF> mod: text) formats. Independent of ESP-IDF'sCONFIG_LOG_COLORS, which only governsESP_LOG*output, so the two can be set separately.Deviation from the issue
The issue asked for
zsys_log_format_msg()to grow a color flag mirroring upstream'sLOG_OUTPUT_FLAG_COLORS. That changes a public signature documented inlog_backend.hfor out-of-tree backends, right after v0.1.0 went public. Backends instead getzsys_log_level_color(level)andZSYS_LOG_COLOR_RESET-- the same "backend decides" capability with no API break. Worth adding the flag when a second in-tree backend actually needs plain text.The ESP_LOG intercept from the original issue is split out to #56; it has an unresolved double-formatting question that shouldn't hold up this change.
Notes
LOG_COLOR_*, because those are gated onCONFIG_LOG_COLORS-- reusing them would silently couple the two options. Sequences are identical to what ESP-IDF emits.CONFIG_ZSYS_LOG_MSG_MAX_LEN + 64buffer still covers it.zsys_log_level_color()range-checks, so an out-of-range level can't index off the table.Test
CONFIG_ZSYS_LOG_COLOR=yand again with=ntest_log_level_colorasserts the exact escapes and that<ERR>survives intact between them; the=nbranch asserts no\033reaches the buffer.🤖 Generated with Claude Code