Repository navigation
docs(canopen): say when a bound device description may be edited in place - #300
Conversation
…lace A bound CanOpenDeviceDescription is the mutable EdsDcfNet model, and every reader of it - the peer-SDO gate on each SDO call, the loader, the foreign PDO and SRDO observers, a safety transaction's reading at its start - enumerates it without synchronization. The README said a bound instance may be edited in place and that the edit is seen from the next call, but not that a structural edit during one of those readings fails that call. #296. Decision (option c of #296): the rule stands and is stated - edit a bound description only while no call that reads it is running; a structural edit during one fails that call with the dictionary's InvalidOperationException, reported as the call's own failure, never a silent wrong reading. README (SDO client, configuring a peer), the Objects and BindPeerDeviceDescription XML docs and FR-CO-029 say so. An immutable description, edit then bind again, is #299 for the next major. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
PR SummaryLow Risk Overview The docs spell out unsupported concurrent-edit behavior—enumerating readers may hit Reviewed by Cursor Bugbot for commit a83b33c. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf1816b7a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…upported, not loud bf1816b promised that a structural edit during a reading always fails that reading with the dictionary's InvalidOperationException. Codex: only an enumerating reader (the loader, a safety transaction's reading) can fail that way; the observers' file fallback and the gate's Contains look objects up one at a time around their own awaits, so an edit between two lookups gives a part-old, part-new reading or no match, with no error - and a Dictionary read concurrent with a write is unspecified anyway. README, the Objects XML doc and FR-CO-029 now say the edit is unsupported and what the call sees is unspecified, naming both outcomes. The rule itself is unchanged: edit only while no reading call runs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b017d76f50
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…e boot section says b017d76 gave "a boot halted at that slave" as the consequence of a verification that failed on a concurrent edit. Codex: that is the mandatory slave and the simultaneous start; an optional slave is skipped and the boot goes on (OnSlaveVerified, and the README's own boot-up section). The example now states the slave is not started and points at that section for the rest. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
What does this change?
Closes #296 by stating the rule as it stands (option c): a bound
CanOpenDeviceDescriptionis the mutable EdsDcfNet model and every reader — the peer-SDO gate on each SDO call, the loader, the foreign PDO and SRDO observers, a safety transaction's one reading at its start — enumerates it without synchronization. So a bound description is edited in place only while no call that reads it is running; an edit during one is unsupported and what that call sees is not specified: an enumerating reader (the loader, a safety transaction's reading) may fail with the dictionary'sInvalidOperationException, reported as that call's own failure; a reader with single lookups around its own awaits (the observers' file fallback, the gate'sContains) may answer from a part-old, part-new description or find no match, with no error. (The first commit promised a loud failure for every case; Codex's finding corrected that, second commit.) README (§SDO client and a peer's device description, §Configuring a peer), the XML docs ofCanOpenDeviceDescription.ObjectsandBindPeerDeviceDescription, and FR-CO-029 say so.Option (b), a library-owned lock with an
Editmethod, is rejected: it covers only the readers, a writer reachingObjectsbypasses it. Option (a), an immutable description (edit, then bind again), is the lasting fix but a breaking change: filed as #299 for the next major.No code change. The gate was run anyway because XML docs are compiled under
-p:CI=true.Type of change
feat— new behaviour (minor release)fix/perf— bug or performance fix (patch release)docs/test/refactor/chore/ci— no release!in the title, plus aBREAKING CHANGE:footer explaining the migration)Checklist
dotnet build CanKit.Pro.sln -c Releasesucceeds (-p:CI=true, 0 warnings)dotnet test CanKit.Pro.sln -c Releasepasses (net10.0: 1884 tests;dotnet format --verify-no-changesclean; pack +verify-packages.pyok; requirements traceability 0 new untraced)🤖 Generated with Claude Code