Make the driver's atomic flag helpers actually atomic - #25
Merged
Merged
Conversation
bitsSetAtomic/bitsClearAtomic/valueSetAtomic were plain read-modify-writes (the interrupt masking a real MCU would use is commented out). In the simulator the "ISRs" - e.g. the serial RX handler that sets EXEC_STATUS_REPORT for a '?' - run on the hardware thread while grblHAL's main loop runs on the grbl thread, so the two threads' updates to the same flags word could interleave and one would be lost. Visible as '?' status requests that never get a reply: 50 of 3200 probes during motion at -t 1 (about 1.6%), with every answered one back in 30 ms or less - loss, not latency. With __atomic_fetch_or / __atomic_fetch_and / __atomic_exchange_n: 0 of 1600. Builds with gcc and clang (the compilers this repo's CI matrix uses); MinGW gcc on Windows supports the same builtins. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
terjeio
reviewed
Sep 25, 2026
| } | ||
|
|
||
| // Helper functions for setting/clearing/inverting individual bits atomically (uninterruptable) | ||
| // |
Contributor
There was a problem hiding this comment.
Please tell the Claude thing that I do not want (parts of) the PR summary in the code, IMO it is not needed.
Contributor
|
Thanks. Re: Caveat |
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Problem
bitsSetAtomic,bitsClearAtomicandvalueSetAtomicinsrc/driver.care plain read-modify-writes; the interrupt masking a real MCU relies on is commented out. In the simulator, the "ISRs" run on the hardware thread while grblHAL's main loop runs on the grbl thread. One example is the serial RX handler that setsEXEC_STATUS_REPORTwhen a?arrives. Both threads update the same flags word, so their updates can interleave and one gets lost.The visible symptom is that some
?status requests never get a reply, most noticeably during motion.Fix
Use real atomic read-modify-writes:
__atomic_fetch_or,__atomic_fetch_andand__atomic_exchange_n(__ATOMIC_SEQ_CST). This is a single-file change tosrc/driver.c.Evidence
Probe: 400
?requests per trial, about 50 ms apart, during a longG91 G1 X1000 F200move at-t 1, over the-ptelnet connection. A request counts as lost if no status report arrives within 2 s.master(91eda77), two runs of 4 trialsSo it's loss, not latency: every answered request is fast, and the rest never get a reply. I found this downstream, in an MCP server that drives grblHAL and runs its hardware test scripts against the simulator in CI. A status-polling test there failed 4 of 14 runs on
masterand passed 14 of 14 with this change.Caveat
The
__atomic_*builtins are GCC/Clang-specific. That covers this repo's CI matrix (gcc, clang, MinGW gcc on Windows), but an MSVC build would need_Interlocked*equivalents. I've only built and tested with gcc 14 on Linux. Happy to adjust if you'd prefer a different primitive, for example C11<stdatomic.h>(already included indriver.c) with the pointed-to fields declared_Atomic.🤖 Generated with Claude Code