Uh oh!
There was an error while loading. Please reload this page.
Moved the demo into apps/ and made both targets build it - #57
Open
fdesbiens wants to merge 1 commit into
Open
Conversation
The framework documented a portable application layer it did not have: each target owned its own demo under app/, so nothing checked that a demo could actually move between boards. This adds apps/, moves the NUCLEO-F401RE demo into apps/threadx_demo/main.c unchanged in behaviour, and has the PolarFire SoC Icicle Kit build that same source as a second executable beside its LM75 monitor. CI runs it under Renode on 32-bit Cortex-M4 and 64-bit RISC-V and asserts on the same console output from both, so the claim is now enforced rather than stated. apps/ holds portable demos; targets/<Vendor>/<BOARD>/app/ holds board-specific ones, and a target's app/CMakeLists.txt picks either. The PolarFire LM75 monitor stays a target app because it genuinely models a sensor. Linking a second executable first required removing a hard undefined reference: polarfire_bsp's trap.c called console_rx_isr_callback(), a symbol only its own demo defined, so any other application had to define a PolarFire-specific ISR callback just to link. bsp/console.h gains a registration call in the shape bsp_self_test() already established: typedef void (*bsp_console_rx_fn)(char c, void *context); void bsp_console_set_rx_handler(bsp_console_rx_fn handler, void *context); The board stores a nullable pointer and checks it before dispatching, so bytes arriving with no handler attached are dropped instead of faulting, and an application that ignores console input defines nothing. The LM75 demo registers its existing handler in main(); the NUCLEO-F401RE polls USART2 and so stores a handler it never invokes, which keeps the contract uniform enough to register against unconditionally. A weak symbol was rejected: it keeps the up-call and is a GCC extension in a C99 codebase. Building the demo for a second architecture found two real defects: - Every %lu in the demo was wrong on one target. ThreadX defines ULONG as unsigned long on the Cortex-M4 port and unsigned int on the RISC-V 64 one, so each value now casts to unsigned long, matching the existing idiom in the LM75 demo. - Both boards' _sbrk() underflow self-test asked for a fixed -128, which is an underflow only when nothing has allocated yet. The NUCLEO's passed by accident of a printf buffer malloc that failed against a small reservation; the PolarFire's failed outright once a demo reached printf first. Both now hand back one byte more than has ever been taken, which underflows whatever ran before them. Thread stacks are sized in machine words rather than bytes, since every saved register and the newlib printf() call chain double in width on a 64-bit hart: 1024 bytes on Cortex-M4 as before, 2048 on RISC-V, measured peak 1048. Measured PolarFire heap use under printf, the first stdio on that board, is 2944 bytes; the run also completes with BSP_HEAP_RESERVE_BYTES cut to 4 KB, so the 64 KB reservation holds unchanged. Verified locally on both toolchains: three Renode suites green, and with the _sbrk() bound deliberately widened the self-tests still fail - two checks on the NUCLEO-F401RE, one on each PolarFire executable. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
The framework documented a portable application layer it did not have. Each target owned its own demo under
app/, so nothing checked that a demo could actually move between boards — which is howapps/stayed documented for weeks while not existing (#52 walked the claim back).This adds
apps/, moves the NUCLEO-F401RE demo intoapps/threadx_demo/main.c, and has the PolarFire SoC Icicle Kit build that same source as a second executable beside its LM75 monitor. CI runs it under Renode on 32-bit Cortex-M4 and 64-bit RISC-V and asserts on the same console output from both.The rule, now written into
docs/architecture.mdandtemplates/target/README.md:PolarFire's LM75 monitor stays a target app because it genuinely models a sensor.
A third BSP interface was unavoidable
polarfire_bsp'strap.ccarried a hard undefined reference toconsole_rx_isr_callback(), a symbol only its own demo defined. Any second executable linking that BSP failed to link, and a portable application would have had to define a PolarFire-specific ISR callback purely to say it wanted nothing. That is the same violation this effort exists to remove — the BSP reaching up into the application — expressed through the linker rather than a header.bsp/console.hgains a registration call in the shapebsp_self_test()already established:The board stores a nullable pointer and checks it before dispatching, so bytes arriving with no handler attached are dropped rather than faulting. The LM75 demo registers its existing handler in
main(); the NUCLEO-F401RE polls USART2 and so stores a handler it never invokes, keeping the contract uniform enough to register against unconditionally. A__attribute__((weak))default was rejected: it compiles, but keeps the up-call and is a GCC extension in a C99/MISRA codebase.This brought the usual obligations with it — an architecture entry, a
templates/target/stub, and README rows, as PR #56 did for the first two interfaces.Building for a second architecture found two real defects
Both were latent and neither was reachable with one target:
%luin the demo was wrong on one of the two boards. ThreadX definesULONGasunsigned longon the Cortex-M4 port andunsigned inton the RISC-V 64 port. Each value now casts tounsigned long, matching the idiom already used in the LM75 demo._sbrk()underflow self-test asked for a fixed-128, which is an underflow only when nothing has allocated yet. The NUCLEO's passed by accident — itsprintfbuffer malloc fails against a small reservation, leaving the break at base. The PolarFire's failed outright once an application reachedprintffirst. Both now hand back one byte more than has ever been taken, which underflows regardless of what ran before.Measurements
printfTHREAD_STACK_SIZEis written in machine words rather than bytes, since every saved register and the newlibprintf()call chain doubles in width on a 64-bit hart.BSP_HEAP_RESERVE_BYTESneeded no change: the whole run also completes with the reservation temporarily cut to 4 KB, which bounds peak heap use across the run rather than just at startup.Verification
Run locally on both toolchains (Arm GNU 14.3.Rel1, xPack RISC-V GCC 14.3.0-1, Renode 1.16.1) — the versions CI pins.
--app lm75, PolarFire--app threadx_demo.-Werror; both PolarFire ELFs build warning-free._sbrk()'s bound deliberately widened to the end of RAM, the checks still fail: two on the NUCLEO-F401RE, one on each PolarFire executable. The NUCLEO count is two rather than the three seen before this PR, because the underflow check now correctly stays green — its old failure was incidental to the fixed-128, not that assertion doing its job.The PolarFire runner is one script with an
--appargument rather than two scripts, since the ~150 lines of Renode process handling are what would drift if copied.Notes for review
app/starter/is gone, including the emptycloud_config.hplaceholder that Moved the startup self-tests and RAM sizing behind new BSP interfaces #56 stopped including.Eclipse ThreadX Device Monitor Demo); both Renode suites and the Robot suite assert on that one line.docs/architecture.mdandtemplates/target/README.md. That was scheduled for a follow-up, but its precondition — the shared demo green on both architectures — is met by this PR, and leaving those statements in place would have shipped documentation that this change makes false.