feat: S08.03 slice 2 CMSDK UART + newlib retargeting#291
Conversation
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1096 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
📝 WalkthroughWalkthroughThis PR replaces semihosting I/O with a CMSDK UART0 MMIO driver and Newlib syscall retargeting for FreeRTOS HelloWorld, adds build/linker and main.c integration, provides host-side fakes and CppUTest cases, and updates CI and DEVLOG to run the image with QEMU -serial stdio. ChangesCMSDK UART + Newlib Retargeting
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Tests/FreeRtos/CMakeLists.txt (1)
10-12: ⚡ Quick winDecouple
CmsdkUartTestfrom the globalFREERTOS_PLUS_TCP_PATHhard-fail.
CmsdkUartTestis intentionally independent of Plus-TCP, but this global fatal guard still blocks it in minimal host environments. Consider scoping the guard toSolidSyslogFreeRtosDatagramTestonly.♻️ Suggested gating split
-if("$ENV{FREERTOS_PLUS_TCP_PATH}" STREQUAL "") - message(FATAL_ERROR "FREERTOS_PLUS_TCP_PATH must be set to build Tests/FreeRtos.") -endif() +set(freeRtosPlusTcpPath "$ENV{FREERTOS_PLUS_TCP_PATH}") ... -add_executable(SolidSyslogFreeRtosDatagramTest +if(freeRtosPlusTcpPath STREQUAL "") + message(STATUS "Skipping SolidSyslogFreeRtosDatagramTest: FREERTOS_PLUS_TCP_PATH not set.") +else() +add_executable(SolidSyslogFreeRtosDatagramTest ... ) ... target_include_directories(SolidSyslogFreeRtosDatagramTest PRIVATE ... - $ENV{FREERTOS_PLUS_TCP_PATH}/source/include + ${freeRtosPlusTcpPath}/source/include ) add_test(NAME SolidSyslogFreeRtosDatagramTest COMMAND SolidSyslogFreeRtosDatagramTest) +endif()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/FreeRtos/CMakeLists.txt` around lines 10 - 12, The global FATAL_ERROR check for FREERTOS_PLUS_TCP_PATH is preventing CmsdkUartTest from building; change the CMake logic in Tests/FreeRtos/CMakeLists.txt so the existence check is only required when the SolidSyslogFreeRtosDatagramTest target (or the code that needs Plus-TCP) is being configured. Concretely, remove or relax the unconditional if("$ENV{FREERTOS_PLUS_TCP_PATH}" STREQUAL "") ... endif() guard and instead wrap that check around the code that adds/configures SolidSyslogFreeRtosDatagramTest (or gate it behind a CMake option/if that indicates Plus-TCP tests), leaving CmsdkUartTest unaffected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Example/FreeRtos/HelloWorld/Syscalls.c`:
- Around line 26-41: Extract the composite bounds check inside _sbrk into a
named static inline predicate (e.g., static inline bool
IsWithinSyscallHeap(char* nextBreak)): forward-declare this helper above _sbrk,
call it from the if statement in _sbrk (replacing the original composite
condition that uses syscallHeap, syscallHeapBreak, sizeof(syscallHeap) and the
cast), and then define the static inline helper immediately below _sbrk so it
encapsulates the pointer arithmetic and size checks (using syscallHeap and
syscallHeapBreak) and returns the boolean result; leave errno and result logic
in _sbrk unchanged.
---
Nitpick comments:
In `@Tests/FreeRtos/CMakeLists.txt`:
- Around line 10-12: The global FATAL_ERROR check for FREERTOS_PLUS_TCP_PATH is
preventing CmsdkUartTest from building; change the CMake logic in
Tests/FreeRtos/CMakeLists.txt so the existence check is only required when the
SolidSyslogFreeRtosDatagramTest target (or the code that needs Plus-TCP) is
being configured. Concretely, remove or relax the unconditional
if("$ENV{FREERTOS_PLUS_TCP_PATH}" STREQUAL "") ... endif() guard and instead
wrap that check around the code that adds/configures
SolidSyslogFreeRtosDatagramTest (or gate it behind a CMake option/if that
indicates Plus-TCP tests), leaving CmsdkUartTest unaffected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93aa6004-cc43-4348-8076-393f6a44bc79
📒 Files selected for processing (9)
.github/workflows/ci.ymlDEVLOG.mdExample/FreeRtos/HelloWorld/CMakeLists.txtExample/FreeRtos/HelloWorld/CmsdkUart.cExample/FreeRtos/HelloWorld/CmsdkUart.hExample/FreeRtos/HelloWorld/Syscalls.cExample/FreeRtos/HelloWorld/main.cTests/FreeRtos/CMakeLists.txtTests/FreeRtos/CmsdkUartTest.cpp
Replace QEMU mps2-an385 HelloWorld's semihosting transport with a
silicon-correct polled CMSDK UART0 driver and newlib retargeting, so
printf/getchar route through `qemu -serial stdio` instead of BKPT
traps. Foundation for slice 3+ where FreeRTOS-Plus-TCP runs concurrently
with the example task — semihosting traps would stall the IP stack on
every host-serviced read/write.
Driver — Example/FreeRtos/HelloWorld/CmsdkUart.{h,c}:
- Polls STATE.TX_FULL before writing DATA, per CMSDK APB UART TRM
(DDI 0479C/D §4.3) and the QEMU model semantics (which set TX_FULL
inside the DATA-write path even when the chardev backend drains
synchronously).
- Memory access injected via CmsdkUartMemoryAccess struct (read32 /
write32 function pointers) — production wires MmioRead32/MmioWrite32
in main.c, tests wire CmsdkUartFake.
- BAUDDIV (=16), TX_EN, RX off, IRQs off all hardcoded — no caller-
facing surface to misconfigure.
Newlib retargeting — Example/FreeRtos/HelloWorld/Syscalls.c:
- _write routes to CmsdkUart_Write. _read returns EOF (slice 3 wires
to UART RX). _sbrk is a 4 KiB bump allocator for newlib re-entrancy
buffers (FreeRTOS heap_4 still owns the kernel/task heap).
- Linker swap: --specs=rdimon.specs replaced with
--specs=nano.specs --specs=nosys.specs.
Tests — Tests/FreeRtos/CmsdkUart{Test,Fake}.{cpp,c,h}:
- 8 host tests, each driven from a failing assertion.
- CmsdkUartFake is a stateful in-memory model of the UART register
block. STATE.TX_FULL clears after a configurable number of STATE
reads (default 2; knob via SetReadsBeforeTxReady). TxOverrunOccurred
flag flips when DATA is written while TX_FULL is set — the spin-loop
test asserts it stays false across two consecutive PutChar calls.
CI — .github/workflows/ci.yml:
- build-freertos-target QEMU args: -nographic + -semihosting-config
replaced with -display none -serial stdio. Banner-grep unchanged.
References (CMSDK contract documented during review):
- Arm DDI 0479C/D §4.3 — register layout, bit semantics
- Arm DAI 0385D §3.7 — UART base addresses on mps2-an385
- QEMU hw/char/cmsdk-apb-uart.c — modelled semantics
- Zephyr drivers/serial/uart_cmsdk_apb.c — canonical poll-out idiom
- mbed-OS targets/TARGET_ARM_FM/TARGET_FVP_MPS2/serial_api.c — Arm
HAL, BAUDDIV ≥ 16 minimum
Closes #290.
DavidCozens
force-pushed
the
feat/s08-03-slice2-cmsdk-uart
branch
from
81fc292 to
e831101
Compare
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1096 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Example/FreeRtos/HelloWorld/CmsdkUart.c`:
- Around line 14-23: The driver currently stores the caller-owned
CmsdkUartMemoryAccess pointer in the global memoryAccess and later dereferences
it (in CmsdkUart_Init and other functions), which can dangle; instead, validate
access is non-NULL in CmsdkUart_Init, copy the contents of the
CmsdkUartMemoryAccess struct into an internal static CmsdkUartMemoryAccess
instance (not a pointer), and use that internal copy for all subsequent calls
(e.g., when invoking write32/read32 and when referencing BAUDDIV_OFFSET,
CTRL_OFFSET, TX_ENABLE); this removes the raw pointer to caller memory and
prevents dangling/NULL dereference while preserving the same API behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f723554d-63d9-473e-b365-97aabd6ee26f
📒 Files selected for processing (11)
.github/workflows/ci.ymlDEVLOG.mdExample/FreeRtos/HelloWorld/CMakeLists.txtExample/FreeRtos/HelloWorld/CmsdkUart.cExample/FreeRtos/HelloWorld/CmsdkUart.hExample/FreeRtos/HelloWorld/Syscalls.cExample/FreeRtos/HelloWorld/main.cTests/FreeRtos/CMakeLists.txtTests/FreeRtos/CmsdkUartFake.cTests/FreeRtos/CmsdkUartFake.hTests/FreeRtos/CmsdkUartTest.cpp
✅ Files skipped from review due to trivial changes (2)
- Tests/FreeRtos/CmsdkUartFake.c
- DEVLOG.md
🚧 Files skipped from review as they are similar to previous changes (4)
- Example/FreeRtos/HelloWorld/CMakeLists.txt
- Tests/FreeRtos/CMakeLists.txt
- .github/workflows/ci.yml
- Example/FreeRtos/HelloWorld/Syscalls.c
| static const CmsdkUartMemoryAccess* memoryAccess = NULL; | ||
| static uintptr_t base = 0U; | ||
|
|
||
| void CmsdkUart_Init(const CmsdkUartMemoryAccess* access, uintptr_t baseAddress) | ||
| { | ||
| memoryAccess = access; | ||
| base = baseAddress; | ||
| access->write32(baseAddress + BAUDDIV_OFFSET, BAUD_DIVISOR); | ||
| access->write32(baseAddress + CTRL_OFFSET, TX_ENABLE); | ||
| } |
There was a problem hiding this comment.
Avoid persisting caller-owned callback table by raw pointer.
At Line 14/Line 19, the driver stores access by pointer and later dereferences it at Line 27 and Line 35. If CmsdkUart_Init receives non-static storage (or invalid callbacks), this can become a dangling/NULL dereference and hard-fault during output.
Suggested fix
`#include` "CmsdkUart.h"
+#include <stdbool.h>
`#include` <stddef.h>
@@
-static const CmsdkUartMemoryAccess* memoryAccess = NULL;
-static uintptr_t base = 0U;
+static CmsdkUartMemoryAccess memoryAccess = {0};
+static uintptr_t baseAddress = 0U;
+static bool isInitialized = false;
void CmsdkUart_Init(const CmsdkUartMemoryAccess* access, uintptr_t baseAddress)
{
- memoryAccess = access;
- base = baseAddress;
- access->write32(baseAddress + BAUDDIV_OFFSET, BAUD_DIVISOR);
- access->write32(baseAddress + CTRL_OFFSET, TX_ENABLE);
+ if ((access != NULL) && (access->read32 != NULL) && (access->write32 != NULL))
+ {
+ memoryAccess = *access; /* copy callbacks by value */
+ baseAddress = baseAddress;
+ memoryAccess.write32(baseAddress + BAUDDIV_OFFSET, BAUD_DIVISOR);
+ memoryAccess.write32(baseAddress + CTRL_OFFSET, TX_ENABLE);
+ isInitialized = true;
+ }
}
void CmsdkUart_PutChar(char c)
{
- while ((memoryAccess->read32(base + STATE_OFFSET) & TX_FULL_BIT) != 0U)
+ if (isInitialized)
{
- /* Spin while the transmit holding register is occupied — writing to
- * DATA in this state sets STATE.TX_OVRE on silicon and the byte is
- * lost. QEMU's CMSDK model drains synchronously inside the DATA
- * write, so this spin never iterates there; it is silicon-correct
- * and the host-side fake exercises both the busy and idle paths. */
+ while ((memoryAccess.read32(baseAddress + STATE_OFFSET) & TX_FULL_BIT) != 0U)
+ {
+ }
+ memoryAccess.write32(baseAddress + DATA_OFFSET, (uint32_t) (unsigned char) c);
}
- memoryAccess->write32(base + DATA_OFFSET, (uint32_t) (unsigned char) c);
}Also applies to: 25-36
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Example/FreeRtos/HelloWorld/CmsdkUart.c` around lines 14 - 23, The driver
currently stores the caller-owned CmsdkUartMemoryAccess pointer in the global
memoryAccess and later dereferences it (in CmsdkUart_Init and other functions),
which can dangle; instead, validate access is non-NULL in CmsdkUart_Init, copy
the contents of the CmsdkUartMemoryAccess struct into an internal static
CmsdkUartMemoryAccess instance (not a pointer), and use that internal copy for
all subsequent calls (e.g., when invoking write32/read32 and when referencing
BAUDDIV_OFFSET, CTRL_OFFSET, TX_ENABLE); this removes the raw pointer to caller
memory and prevents dangling/NULL dereference while preserving the same API
behavior.
There was a problem hiding this comment.
Declining this one — store-by-pointer is intentional and matches the project pattern.
Why: the project rule (CLAUDE.md) is "Don't add error handling, fallbacks, or validation for scenarios that can't happen. Trust internal code and framework guarantees. Only validate at system boundaries (user input, external APIs)." The proposed isInitialized flag, the NULL guards, and the silent-no-op fallback in _PutChar all guard against scenarios that can't happen with the one production caller — main.c passes &MMIO_ACCESS where MMIO_ACCESS is static const, lifetime is the whole program.
Pattern consistency: other library configs hold function/object pointers by reference too — e.g. SolidSyslogStreamSenderConfig carries resolver/stream/endpoint as pointers, no isInitialized flag, no NULL checks. Adopting the suggested defensive pattern here would diverge from the rest of the codebase.
Cost vs benefit: struct-copy + extra state for a theoretical dangling-pointer scenario that can't materialise in our usage. If the API ever grows a stack-allocated caller, we'll revisit.
Documented in the slice-2 DEVLOG entry under "Code review feedback addressed" so this decision isn't lost.
There was a problem hiding this comment.
@DavidCozens, understood — the reasoning is clear and consistent. The static const lifetime of MMIO_ACCESS in main.c makes the dangling-pointer scenario genuinely impossible here, and adding isInitialized guards + struct copying would diverge from the established project pattern without real benefit. I'll withdraw the suggestion.
✏️ Learnings added
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 291
File: Example/FreeRtos/HelloWorld/CmsdkUart.c:17-42
Timestamp: 2026-05-08T17:19:38.382Z
Learning: In the DavidCozens/solid-syslog project, driver/config structs (e.g., `CmsdkUartMemoryAccess` in `Example/FreeRtos/HelloWorld/CmsdkUart.c`) are intentionally stored as raw pointers without NULL guards, `isInitialized` flags, or defensive struct copies. The project policy (CLAUDE.md) prohibits error handling or validation for scenarios that can't happen; production callers always pass pointers to `static const` objects with program lifetime. Do not flag store-by-pointer patterns or missing NULL/initialization guards for internal config/access structs in this codebase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 153
File: Example/Windows/SolidSyslogWindowsExample.c:45-55
Timestamp: 2026-04-18T07:18:35.201Z
Learning: In this repository’s example entry-point C files under `Example/` (e.g., `SolidSyslogWindowsExample.c`, `SolidSyslogExample.c`), it is intentional to omit `argc/argv` NULL/length guards before using `argv[0]` (e.g., calling `ExampleAppName_Set(argv[0])`). C guarantees that when invoked via `main`, `argc > 0` and `argv[0]` is a valid string (§5.1.2.2.1), and the project policy (CLAUDE.md) forbids defensive checks for impossible scenarios. Therefore, do not raise review findings for missing `argc/argv` guards in these example entry points.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 153
File: Example/Windows/SolidSyslogWindowsExample.c:57-66
Timestamp: 2026-04-18T07:18:55.197Z
Learning: In this project’s example entry-point C files, do not flag/require NULL checks after calls to the solid-syslog `Create` functions (e.g., `SolidSyslogWinsockResolver_Create`, `SolidSyslogWinsockDatagram_Create`, `SolidSyslogUdpSender_Create`, `SolidSyslogNullBuffer_Create`, `SolidSyslogNullStore_Create`, `SolidSyslogAtomicCounter_Create`, `SolidSyslogMetaSd_Create`, `SolidSyslogTimeQualitySd_Create`, `SolidSyslogOriginSd_Create`). These `Create` APIs return pointers to static instances and are designed not to fail at construction time; any runtime failures that can occur are surfaced later via the vtable at use-time (e.g., host resolution). Therefore, NULL checks after `Create` calls in example files (such as `Example/Windows/SolidSyslogWindowsExample.c` and similar examples) are intentionally omitted and should not be treated as a bug.
Composite bounds check (pointer arithmetic + cast + two comparisons) moved into a static inline named predicate per CodeRabbit review and the project's intent-naming-predicates rule. Helper forward-declared above _sbrk and defined immediately beneath it. Refs #290.
CmsdkUartTest tests UART board glue and never includes Plus-TCP headers, so the global FATAL_ERROR was wrongly blocking it in FREERTOS_KERNEL_PATH-only environments. Wrap the existence check around SolidSyslogFreeRtosDatagramTest only — print a STATUS message and skip that target when the path is unset, leave CmsdkUartTest unaffected. Per CodeRabbit nitpick. Refs #290.
CmsdkUartMemoryAccess gains a sleep(int milliseconds) hook called inside PutChar's STATE.TX_FULL spin so the polled wait doesn't burn CPU on real silicon. Production wires vTaskDelay(pdMS_TO_TICKS(1)); the host fake counts the calls and a new test asserts the spin yields between status reads. Driver body extracted into intent-named static inline helpers — Init becomes SetBaudDivisor() then EnableTransmitter(); PutChar becomes while (TransmitterIsBusy()) Yield(); WriteDataRegister(c). Helpers forward-declared at the top of the file, defined immediately beneath their first caller. No internal comments — names and CMSDK_UART.md carry the documentation. Refs #290.
Captures the polled-mode TX/RX register sequences, bit semantics, and
QEMU-vs-silicon traps that informed the v2 driver. Co-located with
CmsdkUart.{h,c} so a future reader finds the contract one directory
entry away. RX and concurrency sections are slice-3+ scope, included
so the slice-3 author doesn't re-derive them.
DEVLOG entry rewritten to cover the sleep injection, the static-inline
helper refactor, the docs landing place, and the CodeRabbit findings
(two folded in, one declined with rationale).
Refs #290.
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1096 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
|
@coderabbitai responses to the two review passes: Tests/FreeRtos/CMakeLists.txt — Example/FreeRtos/HelloWorld/Syscalls.c — extract Example/FreeRtos/HelloWorld/CmsdkUart.c — copy-by-value + NULL guards (major) — Declined (replied inline with full rationale). The store-by-pointer pattern is intentional, matches the rest of the codebase ( Thanks for the review. |
|
Tip For best results, initiate chat on the files or code changes.
(╯°□°）╯ (one more rabbit sulking that its NULL-guard idea was rightly rejected) ✏️ Learnings added
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Example/FreeRtos/HelloWorld/CMSDK_UART.md`:
- Line 268: The fenced code block containing the poll_in() and
read_byte_blocking() examples is missing a language tag which triggers
markdownlint MD040; update the opening fence from ``` to ```text so the block
begins with ```text (e.g., change the start of the block that contains poll_in()
and read_byte_blocking() to ```text) to satisfy the linter while leaving the
block contents unchanged.
In `@Example/FreeRtos/HelloWorld/main.c`:
- Around line 23-25: RtosSleep currently calls
vTaskDelay(pdMS_TO_TICKS((TickType_t) milliseconds)) which can yield 0 ticks for
small millisecond values; change the implementation in RtosSleep to compute
TickType_t ticks = pdMS_TO_TICKS((TickType_t) milliseconds) and if milliseconds
> 0 and ticks == 0 set ticks = 1 before calling vTaskDelay(ticks) so any
positive millisecond request sleeps at least one tick and preserves the TX_FULL
throttling in CmsdkUart_PutChar.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dca1f5f9-ece4-4e89-b83a-b53fffbc5227
📒 Files selected for processing (10)
DEVLOG.mdExample/FreeRtos/HelloWorld/CMSDK_UART.mdExample/FreeRtos/HelloWorld/CmsdkUart.cExample/FreeRtos/HelloWorld/CmsdkUart.hExample/FreeRtos/HelloWorld/Syscalls.cExample/FreeRtos/HelloWorld/main.cTests/FreeRtos/CMakeLists.txtTests/FreeRtos/CmsdkUartFake.cTests/FreeRtos/CmsdkUartFake.hTests/FreeRtos/CmsdkUartTest.cpp
✅ Files skipped from review due to trivial changes (1)
- DEVLOG.md
🚧 Files skipped from review as they are similar to previous changes (6)
- Tests/FreeRtos/CmsdkUartFake.h
- Example/FreeRtos/HelloWorld/CmsdkUart.h
- Tests/FreeRtos/CmsdkUartTest.cpp
- Example/FreeRtos/HelloWorld/CmsdkUart.c
- Tests/FreeRtos/CmsdkUartFake.c
- Example/FreeRtos/HelloWorld/Syscalls.c
|
|
||
| ### 3.4 Read-then-clear pseudocode | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced block at Line 268.
This currently triggers markdownlint MD040 (“fenced-code-language”) and can break docs lint in CI.
Suggested patch
-```
+```text
poll_in():
if (STATE & RXFULL) == 0:
return EMPTY
@@
read_byte_blocking():
while (STATE & RXFULL) == 0:
Yield() # vTaskDelay(1) — same pattern as TX spin
return DATA
-```
+```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 268-268: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Example/FreeRtos/HelloWorld/CMSDK_UART.md` at line 268, The fenced code block
containing the poll_in() and read_byte_blocking() examples is missing a language
tag which triggers markdownlint MD040; update the opening fence from ``` to
```text so the block begins with ```text (e.g., change the start of the block
that contains poll_in() and read_byte_blocking() to ```text) to satisfy the
linter while leaving the block contents unchanged.
| static void RtosSleep(int milliseconds) | ||
| { | ||
| vTaskDelay(pdMS_TO_TICKS((TickType_t) milliseconds)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "configTICK_RATE_HZ:"
rg -nP '^\s*#\s*define\s+configTICK_RATE_HZ\b' --type c --type h -C1
echo
echo "pdMS_TO_TICKS macro definition:"
rg -nP '^\s*#\s*define\s+pdMS_TO_TICKS\b' --type c --type h -C2Repository: DavidCozens/solid-syslog
Length of output: 706
🏁 Script executed:
cat -n Example/FreeRtos/HelloWorld/main.c | head -40Repository: DavidCozens/solid-syslog
Length of output: 1450
🏁 Script executed:
rg -n "RtosSleep|TX_FULL|CmsdkUart" Example/FreeRtos/HelloWorld/main.c -A2 -B1Repository: DavidCozens/solid-syslog
Length of output: 571
🏁 Script executed:
cat -n Example/FreeRtos/HelloWorld/CmsdkUart.h | head -100Repository: DavidCozens/solid-syslog
Length of output: 1018
🏁 Script executed:
rg -n "sleep|Sleep|delay|Delay" Example/FreeRtos/HelloWorld/ -A2 -B1 --type cRepository: DavidCozens/solid-syslog
Length of output: 2093
🏁 Script executed:
rg -n "YIELD_MILLISECONDS" Example/FreeRtos/HelloWorld/ -B2 -A2 --type cRepository: DavidCozens/solid-syslog
Length of output: 696
🏁 Script executed:
cat -n Example/FreeRtos/HelloWorld/CmsdkUart.c | head -100Repository: DavidCozens/solid-syslog
Length of output: 2203
🏁 Script executed:
# Check naming compliance for the changed code
cat -n Example/FreeRtos/HelloWorld/main.c | sed -n '1,50p'Repository: DavidCozens/solid-syslog
Length of output: 1794
🏁 Script executed:
# Verify clang-format compliance by checking indentation
git diff --no-index /dev/null Example/FreeRtos/HelloWorld/main.c 2>/dev/null | head -60 || cat Example/FreeRtos/HelloWorld/main.c | head -50Repository: DavidCozens/solid-syslog
Length of output: 1742
Fix RtosSleep to ensure a minimum 1-tick delay when milliseconds > 0.
With configTICK_RATE_HZ = 100 (configured in FreeRTOSConfig.h), pdMS_TO_TICKS(1) evaluates to 0 via integer division. This causes vTaskDelay(0) (yield-only) to be called instead of sleeping a tick, undermining the TX_FULL spin-loop throttling in CmsdkUart_PutChar().
Suggested patch
static void RtosSleep(int milliseconds)
{
- vTaskDelay(pdMS_TO_TICKS((TickType_t) milliseconds));
+ TickType_t ticks = 0;
+ if (milliseconds > 0)
+ {
+ ticks = pdMS_TO_TICKS((uint32_t) milliseconds);
+ if (ticks == 0U)
+ {
+ ticks = 1U;
+ }
+ }
+ vTaskDelay(ticks);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Example/FreeRtos/HelloWorld/main.c` around lines 23 - 25, RtosSleep currently
calls vTaskDelay(pdMS_TO_TICKS((TickType_t) milliseconds)) which can yield 0
ticks for small millisecond values; change the implementation in RtosSleep to
compute TickType_t ticks = pdMS_TO_TICKS((TickType_t) milliseconds) and if
milliseconds > 0 and ticks == 0 set ticks = 1 before calling vTaskDelay(ticks)
so any positive millisecond request sleeps at least one tick and preserves the
TX_FULL throttling in CmsdkUart_PutChar.
1 participant
Purpose
Slice 2 of S08.03 (parent: #268; story: #290). Replaces the QEMU mps2-an385 HelloWorld's semihosting transport with a silicon-correct polled CMSDK UART0 driver and newlib retargeting, so
printf/getcharroute throughqemu -serial stdioinstead ofBKPTtraps.This is the foundation for slice 3+, where FreeRTOS-Plus-TCP and the example task run concurrently. Semihosting
BKPTtraps pause the entire VM during every host-serviced read/write — on a blocking_readwith no Behave input ready, the IP stack and theServicetask would also stall. CMSDK UART0 is plain MMIO at0x40004000: the IP stack and console run independently, and Behave will drive the QEMU image over stdin/stdout exactly as it drives the Linux/Windows examples today.Closes #290.
Change Description
Driver —
Example/FreeRtos/HelloWorld/CmsdkUart.{h,c}STATE.TX_FULLbefore writingDATA, per CMSDK APB UART TRM (DDI 0479C/D §4.3) and confirmed against the QEMU model (hw/char/cmsdk-apb-uart.c) and the Zephyr / mbed-OS reference drivers. WritingDATAwhileTX_FULL=1setsSTATE.TX_OVREon silicon and the byte is lost — QEMU's chardev backend drains synchronously inside the DATA-write path so the bug never manifests there, but it would drop bytes on real hardware.CmsdkUartMemoryAccessstruct ofread32/write32function pointers. Production wiresMmioRead32/MmioWrite32inmain.c(castuintptr_t→volatile uint32_t*, deref). Tests wireCmsdkUartFakewhoseRead32/Write32intercept register accesses against an in-memory model with tunable behaviour.BAUDDIV=16,CTRL.TX_EN=1, RX off, no IRQs. No caller-facing way to misconfigure — kills the silicon traps the contract document called out (BAUDDIV<16 silently broken on silicon, DATA-before-TX_EN implementation-defined).Newlib retargeting —
Example/FreeRtos/HelloWorld/Syscalls.c_write→CmsdkUart_Write._readreturns EOF (slice 3 wires it to UART RX)._sbrkis a 4 KiB bump allocator for newlib re-entrancy buffers (FreeRTOS heap_4 still owns the kernel/task heap).--specs=rdimon.specs→--specs=nano.specs --specs=nosys.specs.Tests —
Tests/FreeRtos/CmsdkUart{Test,Fake}.{cpp,c,h}8 host tests against the fake, each driven from a failing assertion before the matching production line:
InitWritesBaudDivisorInitEnablesTransmitterPutCharWritesByteToDataRegisterPutCharWritesTheGivenBytePutCharSpinsForTxFullToClearBeforeWritingNextBytewhile (state & TX_FULL)poll — two consecutive PutChars must not trip the fake's overrun flagPutCharWritesImmediatelyWhenTransmitterIsAlwaysReadySetReadsBeforeTxReady(0)— proves the spin path also works under always-ready (the QEMU chardev model's actual behaviour) without spurious overrunWriteOfSingleByteEmitsThatByteWrite→PutCharWriteOfMultipleBytesEmitsAllByteWithoutOverrunThe
CmsdkUartFakemodelsSTATE.TX_FULLset on DATA writes and cleared after N STATE reads (default 2, knob viaSetReadsBeforeTxReady); recordsTX_OVREand a stickytxOverrunOccurredflag when DATA is written whileTX_FULL=1. W1C semantics on STATE/INTSTATUS modelled per the QEMU model source.CI —
.github/workflows/ci.ymlbuild-freertos-targetQEMU args:-nographic -semihosting-config enable=on,target=native→-display none -serial stdio. Banner-grep unchanged. End-to-end:printf→ newlib_write→CmsdkUart_Write→CmsdkUart_PutChar→ MMIO at0x40004000→ QEMU CMSDK UART model →-serial stdio→ host stdout.Test Evidence
cpputest-freertos:sha-44efeaehost TDD:SolidSyslogTests(1088 tests),SolidSyslogFreeRtosDatagramTest(slice 1),CmsdkUartTest(8 tests) — all green.cpputest-freertos-cross:sha-44efeaecross build + QEMUmps2-an385with-display none -serial stdio— banner emitted,rc=124(timeout success path; scheduler keeps idling so GDB can attach interactively).clang-format --dry-run --WerroroverCore/Interface Core/Source Tests Example— clean. (TEST_GROUPwith no data members trips clang-format's class-detection heuristic — added a localised// clang-format off/onpair around the fixture.)The other Linux jobs (
build-linux-gcc,build-linux-clang,sanitize-linux-gcc,coverage-linux-gcc,analyze-tidy,analyze-cppcheck,analyze-iwyu) don't compile any of the changed code: every new file lives underExample/FreeRtos/orTests/FreeRtos/, both gated onFREERTOS_KERNEL_PATH(host TDD viacpputest-freertos) orCMAKE_CROSSCOMPILING + arm(cross build). They're skipped intentionally.Areas Affected
Example/FreeRtos/HelloWorld/— board glue (driver, syscalls, main, linker spec).Tests/FreeRtos/CMakeLists.txt,Tests/FreeRtos/CmsdkUart{Test,Fake}.{cpp,c,h}— new host-TDD target with no FreeRTOS-Plus-TCP coupling..github/workflows/ci.yml—build-freertos-targetQEMU args only.No changes to
Core/,Platform/Posix/,Platform/Windows/,Platform/FreeRtos/, or any of the existing examples.References (CMSDK contract — informed the v2 driver)
hw/char/cmsdk-apb-uart.c— modelled semantics (W1C, TXFULL set inside DATA write, chardev backpressure that hides overrun under stdio)drivers/serial/uart_cmsdk_apb.c— canonical poll-out idiomtargets/TARGET_ARM_FM/TARGET_FVP_MPS2/serial_api.c— Arm HAL,BAUDDIV ≥ 16minimumSlice 3 pre-flight
Confirmed
/opt/freertos/plus-tcp/source/portable/NetworkInterface/MPS2_AN385/is present in the cross image (NetworkInterface.c+ether_lan9118/). The LAN9118 NIC driver is in place — slice 3's IP-stack bring-up isn't blocked on missing portable code.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation
Chores