Conversation
pstop_c is already OS-agnostic C99, but a Zephyr application currently has to reach into pstop_c/ with hand-written CMake to use it -- duplicating the source list in every consumer and re-solving the same problems each time. This adds the standard Zephyr module plumbing so an application can add this repository to its west.yml and select CONFIG_PSTOP instead. CONFIG_PSTOP defaults to n, so merely having the repository in a manifest adds nothing to an image that does not ask for it, and an existing consumer compiling pstop_c by hand keeps working unchanged rather than hitting duplicate symbols. The sources are split by role. A remote -- the unit carrying the stop switch -- needs only six of the eleven core files; machine.c, protocol.c, pstop_application.c and pstop_remote_data.c sit behind CONFIG_PSTOP_MACHINE so a remote image does not carry them. The optional UDP transport is behind CONFIG_PSTOP_TRANSPORT_UDP for applications that manage their own sockets. CONFIG_PSTOP_ZEPHYR_TIME (default y) is the option that matters most. pstop/src/pstop/time.c implements time_get_now() only under __linux__ and returns 0 on every other target, so a Zephyr build that linked it would get a clock that never advances -- disabling every heartbeat and timeout in the protocol while the build succeeds and the code reads correctly. The module compiles a k_uptime_get()-backed implementation instead and excludes upstream's time.c. This is the same trap components/pstop/port/ pstop_time_esp.c works around for ESP-IDF; a module is the place to solve it once for every Zephyr consumer. protocol.c prints uint64 stamps with %ld, a hard -Werror=format wherever long is 32-bit, so -Wno-format is applied to this library only when the machine half is enabled. Consuming applications keep their own warning set. Verified against a Zephyr 4.4 application on native_sim: with CONFIG_PSTOP unset the module registers and contributes nothing; with CONFIG_PSTOP=y a remote application builds and its 50-case test suite passes, including golden-frame tests that check encoded bytes against this library's own output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Graphify review — findings
Packages the repository as a Zephyr module, gated behind CONFIG_PSTOP, so applications can consume pstop_c via west and Kconfig instead of hand-written CMake. Splits the build into always-on core sources, machine-only sources behind CONFIG_PSTOP_MACHINE, and optional UDP transport behind CONFIG_PSTOP_TRANSPORT_UDP, and relaxes -Wformat for the machine library to accommodate protocol.c's uint64 logging. Adds time_get_now() backed by k_uptime_get() and selects it by default via CONFIG_PSTOP_ZEPHYR_TIME, since upstream's time.c returns 0 off Linux and would leave every heartbeat and timeout frozen.
Worth a look
- Disabling Zephyr time still compiles the module's time_get_now —
zephyr/CMakeLists.txt:45· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 2 functions depend on the 2 functions this change touches.
Health — grade A; no new coupling hotspots.
Verification — 2 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 2 function(s) in the blast radius were not formally verified this run
CONFIG_PSTOP_ZEPHYR_TIME conflated two different intentions. Its help text
said to turn it off if the application defines time_get_now() itself -- but
turning it off compiled pstop_c's own time.c, whose time_get_now() is a
strong symbol (T in the object). An application following that advice got a
duplicate definition at link.
A plain bool cannot express the third state. Replaces it with a choice:
PSTOP_TIME_ZEPHYR k_uptime_get()-backed, supplied by this module.
Default, and the right answer on any Zephyr target.
PSTOP_TIME_UPSTREAM pstop_c's time.c unchanged. Real clock on a POSIX
build; returns 0 on anything else, which silently
freezes every heartbeat and timeout.
PSTOP_TIME_APPLICATION Compile neither. The application supplies it.
Verified all three on native_sim by inspecting which object the module
compiled: zephyr only, upstream only, and neither, respectively. The default
is unchanged, so no consumer needs to do anything.
Found by review on polymathrobotics#121.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks — this was a good catch, and it found a real bug, though not quite the one the title describes. Pushed a fix in What I checked firstThe literal claim didn't reproduce.
So disabling it did not compile the module's But the underlying problem is realDisabling it compiled a My own help text said "turn it off only if the application defines The root cause is that a bool can't express three states. "Use the Zephyr clock", "use upstream's time.c", and "I'll supply my own" are distinct intentions, and the switch collapsed the last two into one. The fixReplaced it with a choice:
Verified all three on One note on the finding's framingThe report says this was "agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict". That caveat was worth having: the stated claim was false, and I'd have dismissed it on that basis if the note hadn't prompted me to work out what the finding was actually gesturing at. Flagging unverified findings as unverified, rather than asserting them, is what made this useful. 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
Packages the repository as a Zephyr module: module.yml exposes it via west with a zephyr/Kconfig guarded by CONFIG_PSTOP, and zephyr/CMakeLists.txt compiles the pstop_c core plus optional machine-side, UDP-transport, and time sources. Adds time_get_now() backed by k_uptime_get() so on-target builds get a monotonic millisecond clock instead of upstream's time.c, which only advances under __linux__ and otherwise returns 0, silently killing every heartbeat and timeout. A PSTOP_TIME_SOURCE choice picks between this Zephyr clock (default), upstream's file, or an application-supplied symbol, since only one strong time_get_now() may link; the machine-side library also compiles with -Wno-format to tolerate %ld on uint64 stamps.
No blocking issues surfaced. 4 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 2 functions depend on the 2 functions this change touches.
Health — grade A; no new coupling hotspots.
Verification — 2 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 2 function(s) in the blast radius were not formally verified this run
|
@rosterloh Appreciate your interest in the project. Before throwing a PR, can you help me understand why Zephyr is important / relevant here? |
|
Hi @iliabaranov, thanks for an awesom project. I'm really interested in using it in my robotics work. This PR grive people the ability to use you pstop_c library in their zephyr projects by adding it as a dependency in a way that makes it visible to the zephyr CMake build system. I have an example of a port of your firmware here. This PR is not strictly needed for your project but I thought it would provide some visibility and maybe even generate to contributions like the other PR I submitted |
…ss fault) (#129) The six little-endian accessors in `endian.c` cast a `uint8_t*` at an arbitrary offset to `uint16_t*` / `uint32_t*` / `uint64_t*` and dereference it: ```c const uint32_t *bytes = (const uint32_t *)(data + *pos); return *bytes; ``` A pstop frame begins with two `uint8_t` fields, so **every multi-byte field lands on an unaligned offset**: `stamp` at 2, `received_stamp` at 10, `id` at 18, `receiver_id` at 22, and so on. Not one of the 32- or 64-bit fields is naturally aligned; the u16 checksum at 46 is only 2-aligned. ## Impact On x86 and Xtensa the hardware absorbs the unaligned access, which is why this has not surfaced — it works fine on the ESP32 targets. On a target that traps it, decoding a frame faults immediately. Cortex-M3 under QEMU: ``` E: ***** USAGE FAULT ***** E: Unaligned memory access E: Faulting instruction address (r15/pc): 0x0000435c E: >>> ZEPHYR FATAL ERROR 0: CPU exception on CPU 0 ``` It is also undefined behaviour independently of the hardware: the cast violates both the alignment requirement and strict aliasing, so a compiler is entitled to assume the pointer is aligned and emit a wide load on any target. ## The change Rewrites all six to build and split values byte by byte — exactly as the **big-endian half already does**. Those were always correct, which is why only the `_le` variants are touched. No behavioural change on a little-endian host. The encoded bytes are identical, which I verified against golden frames captured from the *previous* implementation rather than regenerated from the new one. ## Testing | Target | Before | After | |---|---|---| | x86-64 (`native_sim`) | 50/50 | 50/50, byte-identical frames | | Cortex-M3 (QEMU) | USAGE FAULT on first decode | 14/14 | The x86 run includes golden-frame tests that assert encoded output against byte arrays captured from this library before the change, so a silent shift in the wire format would have failed them. ## Context Found while adding `qemu_cortex_m3` coverage to a Zephyr application built on `pstop_c`, so that its unit suites can run on macOS — `native_sim` is Linux-only. Independent of #121 (that touches only `zephyr/`, this only `pstop_c/`); the two apply cleanly together. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: johnpolymath <johnh@polymathrobotics.com>
|
@rosterloh As in this Zephyr? Also, consider joining the OSSC: https://opensourcesafe.com/ |
|
Yes, that Zephyr. |
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
No blocking issues surfaced.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 2 functions depend on the 2 functions this change touches.
Health — grade A; no new coupling hotspots.
Verification — 2 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 2 function(s) in the blast radius were not formally verified this run
2 participants
What
Adds the standard Zephyr module plumbing (
zephyr/module.yml,zephyr/Kconfig,zephyr/CMakeLists.txt) so a Zephyr application can add this repository to itswest.ymland selectCONFIG_PSTOP, instead of reaching intopstop_c/with hand-written CMake.Why
pstop_cis already OS-agnostic C99 and ports cleanly — but every Zephyr consumer currently has to duplicate the source list and re-solve the same problems. This is the same shape as the existing ESP-IDF wrapper incomponents/pstop/, expressed the way Zephyr expects.Design notes
CONFIG_PSTOPdefaults ton. Merely having this repository in a manifest adds nothing to an image that did not ask for it, and an existing consumer that compilespstop_cby hand keeps working unchanged rather than hitting duplicate symbols.Sources are split by role. A remote — the unit carrying the stop switch — needs only six of the eleven core files.
machine.c,protocol.c,pstop_application.candpstop_remote_data.csit behindCONFIG_PSTOP_MACHINE, so a remote image does not carry the machine half. The optional UDP transport is behindCONFIG_PSTOP_TRANSPORT_UDP.CONFIG_PSTOP_ZEPHYR_TIME(defaulty) is the option that matters most.pstop/src/pstop/time.cimplementstime_get_now()only under__linux__and returns0on every other target. A Zephyr build that linked it would get a clock that never advances — disabling every heartbeat and timeout in the protocol, while the build succeeds and the code reads correctly. The module compiles ak_uptime_get()-backed implementation instead and excludes upstream'stime.c.This is the same trap
components/pstop/port/pstop_time_esp.cworks around for ESP-IDF. A module seems the right place to solve it once for every Zephyr consumer rather than in each application.-Wno-formatis scoped to the machine half only.protocol.cprintsuint64stamps with%ld, a hard-Werror=formatwhereverlongis 32-bit — the same reason the ESP-IDF component relaxes it. Consuming applications keep their own warning set.Testing
Verified against a Zephyr 4.4 application on
native_sim:CONFIG_PSTOPunset: the module registers (CONFIG_ZEPHYR_PSTOP_MODULE=y) and contributes no code; the consuming build is unaffected.CONFIG_PSTOP=y: a remote application builds and its 50-case suite passes, including golden-frame tests that assert encoded bytes against output captured from this library's own encoder, and a live interop run againstpstop_c/examples/machine/machine_appover loopback — BOND accepted, heartbeat window adopted, fail-safe STOP sustained.No files outside
zephyr/are touched;pstop_c/is unchanged.Note for maintainers
Happy to adjust the Kconfig symbol names or the role split if you would prefer a different granularity — the structure is the proposal, not the detail.
🤖 Generated with Claude Code