Migrate renode test to new container#754
Conversation
danielinux
commented
Apr 16, 2026
danielinux
commented
- fix nRF52 UART
hal/nrf52.c: - Use static volatile buffer for UARTE DMA source instead of stack variable address. GCC 15.2 with -Os optimized away the store to the stack slot, causing the DMA to read zeros. - Set UART0_ENABLE to 4 (UARTE mode) per NRF52840 datasheet. tools/test-expect-version/test-expect-version.c: - Replace deprecated termio.h and linux/serial.h with sys/ioctl.h for compatibility with newer glibc. tools/renode/docker-test.sh: - Remove unused RENODE_CHECKOUT env var. tools/scripts/renode-test-update.sh: - Add robust UART wait functions with timeouts and liveness checks. - Log Renode output to /tmp/renode.log for diagnostics on failure. - Use run_expect_version helper with configurable timeout.
There was a problem hiding this comment.
Pull request overview
Migrates Renode-based CI tests to a prebuilt GHCR container image and hardens the Renode update test harness, while also adjusting nRF52 UART TX handling.
Changes:
- Switch Renode CI to pull/run a GHCR-hosted Renode image and add GHCR login/permissions in Renode workflows.
- Improve Renode update test robustness with UART/renode startup timeouts and log capture.
- Adjust nRF52 UART/UARTE initialization and TX buffer handling.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/test-expect-version/test-expect-version.c | Adjusts Linux-only includes used by the UART version probe tool. |
| tools/scripts/renode-test-update.sh | Adds timeouts, readiness checks, and Renode logging for more reliable automated Renode update testing. |
| tools/renode/docker-test.sh | Moves from building a local Docker image to pulling/running a GHCR image for Renode tests. |
| hal/nrf52.c | Updates nRF52 UART/UARTE enable value and uses a static TX buffer for EasyDMA TX. |
| .github/workflows/test-x86-fsp-qemu.yml | Adds apt mirror workaround + retry options to stabilize package installation. |
| .github/workflows/test-build-kontron-vx3060-s2.yml | Adds apt mirror workaround + retry options to stabilize package installation. |
| .github/workflows/test-renode-smallstack.yml | Adds GHCR permissions + login for pulling the Renode container. |
| .github/workflows/test-renode-sha384.yml | Adds GHCR permissions + login for pulling the Renode container. |
| .github/workflows/test-renode-sha3.yml | Adds GHCR permissions + login for pulling the Renode container. |
| .github/workflows/test-renode-nrf52.yml | Adds GHCR permissions + login for pulling the Renode container. |
| .github/workflows/test-renode-noasm.yml | Adds GHCR permissions + login for pulling the Renode container. |
| .github/workflows/test-renode-noasm-smallstack.yml | Adds GHCR permissions + login for pulling the Renode container. |
| .github/workflows/test-renode-fastmath.yml | Adds GHCR permissions + login for pulling the Renode container. |
| .github/workflows/test-renode-fastmath-smallstack.yml | Adds GHCR permissions + login for pulling the Renode container. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| -e "s|https\?://azure\.archive\.ubuntu\.com/ubuntu/?|http://mirror.arizona.edu/ubuntu/|g" \ | ||
| /etc/apt/sources.list || true | ||
|
|
||
| for f in /etc/apt/sources.list.d/*.list; do | ||
| sudo sed -i \ | ||
| -e "s|https\?://azure\.archive\.ubuntu\.com/ubuntu/?|http://mirror.arizona.edu/ubuntu/|g" \ | ||
| "$f" | ||
| done | ||
|
|
||
| for f in /etc/apt/sources.list.d/*.sources; do | ||
| sudo sed -i \ | ||
| -e "s|https\?://azure\.archive\.ubuntu\.com/ubuntu/?|http://mirror.arizona.edu/ubuntu/|g" \ | ||
| -e "s|https\?://azure\.archive\.ubuntu\.com|http://mirror.arizona.edu|g" \ | ||
| "$f" | ||
| done | ||
|
|
||
| if grep -qE '^[[:space:]]*https?://azure\.archive\.ubuntu\.com/ubuntu/?' /etc/apt/apt-mirrors.txt; then | ||
| sudo sed -i 's|https\?://azure\.archive\.ubuntu\.com/ubuntu/|http://mirror.arizona.edu/ubuntu/|g' /etc/apt/apt-mirrors.txt |
There was a problem hiding this comment.
This workflow rewrites Ubuntu apt sources from https://... to an http:// mirror. Even though apt verifies signed repositories, keeping HTTPS avoids unnecessary downgrade/MITM risk and aligns with the default secure transport. If the mirror supports it, prefer https://mirror.arizona.edu/... (or another HTTPS mirror) rather than HTTP.
| -e "s|https\?://azure\.archive\.ubuntu\.com/ubuntu/?|http://mirror.arizona.edu/ubuntu/|g" \ | |
| /etc/apt/sources.list || true | |
| for f in /etc/apt/sources.list.d/*.list; do | |
| sudo sed -i \ | |
| -e "s|https\?://azure\.archive\.ubuntu\.com/ubuntu/?|http://mirror.arizona.edu/ubuntu/|g" \ | |
| "$f" | |
| done | |
| for f in /etc/apt/sources.list.d/*.sources; do | |
| sudo sed -i \ | |
| -e "s|https\?://azure\.archive\.ubuntu\.com/ubuntu/?|http://mirror.arizona.edu/ubuntu/|g" \ | |
| -e "s|https\?://azure\.archive\.ubuntu\.com|http://mirror.arizona.edu|g" \ | |
| "$f" | |
| done | |
| if grep -qE '^[[:space:]]*https?://azure\.archive\.ubuntu\.com/ubuntu/?' /etc/apt/apt-mirrors.txt; then | |
| sudo sed -i 's|https\?://azure\.archive\.ubuntu\.com/ubuntu/|http://mirror.arizona.edu/ubuntu/|g' /etc/apt/apt-mirrors.txt | |
| -e "s|https\?://azure\.archive\.ubuntu\.com/ubuntu/?|https://mirror.arizona.edu/ubuntu/|g" \ | |
| /etc/apt/sources.list || true | |
| for f in /etc/apt/sources.list.d/*.list; do | |
| sudo sed -i \ | |
| -e "s|https\?://azure\.archive\.ubuntu\.com/ubuntu/?|https://mirror.arizona.edu/ubuntu/|g" \ | |
| "$f" | |
| done | |
| for f in /etc/apt/sources.list.d/*.sources; do | |
| sudo sed -i \ | |
| -e "s|https\?://azure\.archive\.ubuntu\.com/ubuntu/?|https://mirror.arizona.edu/ubuntu/|g" \ | |
| -e "s|https\?://azure\.archive\.ubuntu\.com|https://mirror.arizona.edu|g" \ | |
| "$f" | |
| done | |
| if grep -qE '^[[:space:]]*https?://azure\.archive\.ubuntu\.com/ubuntu/?' /etc/apt/apt-mirrors.txt; then | |
| sudo sed -i 's|https\?://azure\.archive\.ubuntu\.com/ubuntu/|https://mirror.arizona.edu/ubuntu/|g' /etc/apt/apt-mirrors.txt |
| -e "s|https\?://azure\.archive\.ubuntu\.com/ubuntu/?|http://mirror.arizona.edu/ubuntu/|g" \ | ||
| /etc/apt/sources.list || true | ||
| for f in /etc/apt/sources.list.d/*.list; do | ||
| sudo sed -i \ | ||
| -e "s|https\?://azure\.archive\.ubuntu\.com/ubuntu/?|http://mirror.arizona.edu/ubuntu/|g" \ | ||
| "$f" | ||
| done | ||
| for f in /etc/apt/sources.list.d/*.sources; do | ||
| sudo sed -i \ | ||
| -e "s|https\?://azure\.archive\.ubuntu\.com/ubuntu/?|http://mirror.arizona.edu/ubuntu/|g" \ | ||
| -e "s|https\?://azure\.archive\.ubuntu\.com|http://mirror.arizona.edu|g" \ | ||
| "$f" | ||
| done | ||
| if grep -qE '^[[:space:]]*https?://azure\.archive\.ubuntu\.com/ubuntu/?' /etc/apt/apt-mirrors.txt; then | ||
| sudo sed -i 's|https\?://azure\.archive\.ubuntu\.com/ubuntu/|http://mirror.arizona.edu/ubuntu/|g' /etc/apt/apt-mirrors.txt |
There was a problem hiding this comment.
This workflow rewrites Ubuntu apt sources from https://... to an http:// mirror. Even though apt verifies signed repositories, keeping HTTPS avoids unnecessary downgrade/MITM risk and aligns with the default secure transport. If the mirror supports it, prefer https://mirror.arizona.edu/... (or another HTTPS mirror) rather than HTTP.
| -e "s|https\?://azure\.archive\.ubuntu\.com/ubuntu/?|http://mirror.arizona.edu/ubuntu/|g" \ | |
| /etc/apt/sources.list || true | |
| for f in /etc/apt/sources.list.d/*.list; do | |
| sudo sed -i \ | |
| -e "s|https\?://azure\.archive\.ubuntu\.com/ubuntu/?|http://mirror.arizona.edu/ubuntu/|g" \ | |
| "$f" | |
| done | |
| for f in /etc/apt/sources.list.d/*.sources; do | |
| sudo sed -i \ | |
| -e "s|https\?://azure\.archive\.ubuntu\.com/ubuntu/?|http://mirror.arizona.edu/ubuntu/|g" \ | |
| -e "s|https\?://azure\.archive\.ubuntu\.com|http://mirror.arizona.edu|g" \ | |
| "$f" | |
| done | |
| if grep -qE '^[[:space:]]*https?://azure\.archive\.ubuntu\.com/ubuntu/?' /etc/apt/apt-mirrors.txt; then | |
| sudo sed -i 's|https\?://azure\.archive\.ubuntu\.com/ubuntu/|http://mirror.arizona.edu/ubuntu/|g' /etc/apt/apt-mirrors.txt | |
| -e "s|https\?://azure\.archive\.ubuntu\.com/ubuntu/?|https://mirror.arizona.edu/ubuntu/|g" \ | |
| /etc/apt/sources.list || true | |
| for f in /etc/apt/sources.list.d/*.list; do | |
| sudo sed -i \ | |
| -e "s|https\?://azure\.archive\.ubuntu\.com/ubuntu/?|https://mirror.arizona.edu/ubuntu/|g" \ | |
| "$f" | |
| done | |
| for f in /etc/apt/sources.list.d/*.sources; do | |
| sudo sed -i \ | |
| -e "s|https\?://azure\.archive\.ubuntu\.com/ubuntu/?|https://mirror.arizona.edu/ubuntu/|g" \ | |
| -e "s|https\?://azure\.archive\.ubuntu\.com|https://mirror.arizona.edu|g" \ | |
| "$f" | |
| done | |
| if grep -qE '^[[:space:]]*https?://azure\.archive\.ubuntu\.com/ubuntu/?' /etc/apt/apt-mirrors.txt; then | |
| sudo sed -i 's|https\?://azure\.archive\.ubuntu\.com/ubuntu/|https://mirror.arizona.edu/ubuntu/|g' /etc/apt/apt-mirrors.txt |
| UART0_BAUDRATE = BAUD_115200; | ||
| UART0_ENABLE = 1; | ||
| UART0_ENABLE = 4; | ||
| } |
There was a problem hiding this comment.
UART0_ENABLE is being set to 4 while this implementation is using EasyDMA-style UARTE registers (TXD_PTR/TXD_MAXCOUNT). In this repo the UARTE enable value is 8 (e.g., test-app/emu-test-apps/nrf5340/uart.c:60), and enabling the wrong peripheral mode can prevent TX from ever starting in Renode/hardware. Consider defining a named constant for UARTE enable and using the correct value here.
| --workdir ${DOCKER_WORKSPACE} \ | ||
| ${DOCKER_TAG} \ | ||
| ${DOCKER_IMAGE} \ | ||
| /bin/bash -c "tools/scripts/renode-test-update.sh $@ 2>&1 > ${DOCKER_TEST_RESULTS_PATH}/logs.txt" |
There was a problem hiding this comment.
The docker run command redirects output as 2>&1 > ..., which sends stderr to the original stdout and only stdout to the file. If the intention is to capture both streams in logs.txt (as implied by later cat ${HOST_LOG_FILENAME}), swap the redirection order (or use &>), so stderr is also written to the log file.
| /bin/bash -c "tools/scripts/renode-test-update.sh $@ 2>&1 > ${DOCKER_TEST_RESULTS_PATH}/logs.txt" | |
| /bin/bash -c "tools/scripts/renode-test-update.sh $@ > ${DOCKER_TEST_RESULTS_PATH}/logs.txt 2>&1" |
| --workdir ${DOCKER_WORKSPACE} \ | ||
| ${DOCKER_TAG} \ | ||
| ${DOCKER_IMAGE} \ | ||
| /bin/bash -c "tools/scripts/renode-test-update.sh $@ 2>&1 > ${DOCKER_TEST_RESULTS_PATH}/logs.txt" |
There was a problem hiding this comment.
/bin/bash -c "tools/scripts/renode-test-update.sh $@ ..." interpolates the host script’s arguments directly into a shell command string without escaping. This allows accidental breakage (spaces/quotes) and also enables command injection if an argument contains shell metacharacters. Prefer passing arguments to docker run as real argv (avoid bash -c), or ensure each argument is safely shell-escaped before building the command string.
| /bin/bash -c "tools/scripts/renode-test-update.sh $@ 2>&1 > ${DOCKER_TEST_RESULTS_PATH}/logs.txt" | |
| /bin/bash -c 'tools/scripts/renode-test-update.sh "$@" > '"${DOCKER_TEST_RESULTS_PATH}"'/logs.txt 2>&1' -- "$@" |
| declare -r HOST_LOG_FILENAME=${HOST_LOG_PATH}/logs.txt | ||
|
|
||
| declare -r DOCKER_TAG=renode_nrf52 | ||
| declare -r DOCKER_IMAGE="${DOCKER_IMAGE:-ghcr.io/wolfssl/wolfboot-ci-renode:latest}" |
There was a problem hiding this comment.
The default Renode image is :latest, which makes CI non-reproducible and can introduce breakages when the image updates. Consider pinning to a version tag or (preferably) an immutable digest, and bump it intentionally when needed.
| declare -r DOCKER_IMAGE="${DOCKER_IMAGE:-ghcr.io/wolfssl/wolfboot-ci-renode:latest}" | |
| declare -r DOCKER_IMAGE="${DOCKER_IMAGE:-ghcr.io/wolfssl/wolfboot-ci-renode:1.15.3}" |
