docs(reviews): settle the CANopen scope questions against the CiA 301 text - #127
Conversation
… text The gap review marked its CANopen rows as recollection rather than fact, because the standard was not available. It is now, so they are read against the document and cited by clause. Two moved, one in each direction. **1018h Identity object is mandatory** (§7.5.2.21 "Category: Mandatory", and the object overview lists it `ro M`), alongside 1000h and 1001h. All three have no presence in the package, so a conformance tool's first request -- 1000h:00 -- is answered with abort 0602 0000h. **The PDO and SDO records are mandatory once the device supports them.** The overview's footnotes say so outright: PDO communication *and* mapping entries are mandatory if a device supports PDOs, SDO parameters if it supports SDOs. This package does both, and 1400h, 1800h and 1200h have no hits at all, while 1600h and 1A00h do. That asymmetry is a conformance defect, not a missing convenience. **The transmission type is mis-encoded, not merely coarse.** The normative table reads 00h synchronous acyclic, 01h-F0h every n-th SYNC, F1h-FBh reserved, FCh/FDh RTR-only, FEh/FFh event-driven. The three-value enum therefore has every one of its values mean something else on the wire than its name says -- EventDriven = 0x00 is "synchronous acyclic" there. It is invisible today only because 1800h does not exist, which is what ties the two items together: implementing 1800h turns the numbering into a wire defect, so the enum has to become a byte first. **Abort codes: 17 of Table 22's 31**, all seventeen correct, none invented. The fourteen missing include 0504 0003h invalid sequence number, which the block transfer needs. In the code's favour: the block-transfer CRC is right -- §7.2.4.3.16 wants x16+x12+x5+1, init 0000h, and "123456789" -> 31C3h, which the implementation produces. No test pins that value though the standard hands it over as a vector, and the XML comment cites §7.2.4.3.15 where the algorithm is in §7.2.4.3.16. The TIME object is `rw O`, optional, so listing it as a gap was too strict; it belongs in the exclusions. Two open defects are now confirmed against the text rather than argued: #38 (§7.2.4.3.3 -- e=0 means normal transfer whatever s says, so cs 0x20 is a legal segmented download) and the first half of #39 (§7.2.4.3.10 -- one confirm per sub-block, retransmission from ackseq + 1). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
PR SummaryLow Risk Overview The new material narrows earlier gap conclusions: mandatory Reviewed by Cursor Bugbot for commit bf9246f. 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: 356c5d9ffd
ℹ️ 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! |
Codex on #127, three P2 findings, all correct and all the same mistake: the first draft equated "the standard defines X" with "the implementation must have X". Having the normative text does not protect against over-reading it -- which is worth recording, since this document exists to replace recollection with evidence. **The empty object dictionary.** CiA 301 binds a finished *device*; these packages are a *library*, and a stack cannot know a vendor ID or serial number -- inventing one would be worse than omitting it. The application can populate the dictionary and ObjectDictionary.AddU32 is public for exactly that, so an empty `_od` is not a conformance defect of the stack. What survives is narrower and still real: the default path yields a nonconformant node with nothing warning about it, and the package README's own example populates 1000h and 2000h only, so following the documented path still leaves 1001h and 1018h missing. That is a question for the scope decision, not a finding against the code. **The transmission type.** The enum is already declared `: byte`, its values are internal discriminants, and no path serializes them, so implementing 1800h does not automatically make the numbering a wire defect -- the OD encoding can map Synchronous to 01h and the event-driven modes to FEh/FFh explicitly. My "make the enum a byte first" was wrong on its face; it already is one. It is also public API, used as ConfigureTpdo's default parameter, so renumbering would break consumers relying on the values. What survives: three values cannot express 02h-F0h or FCh/FDh, so something has to be added -- as an explicit OD encoding or a byte-valued API, not as a renumbering. The claimed ordering prerequisite is dropped with the premise it rested on. **The abort codes.** Table 22 is a protocol reference, not an implementation checklist. Nothing is lost on receive, because SdoAbortException.AbortCode is a raw uint and an unknown peer code reaches the application unaltered; the server needs named values only for states it detects and emits. The finding therefore shrinks to 0504 0003h, which the block transfer does detect and has no code for. The hardware, device-state and value-range codes become scope when some behavior emits them, and not before. The proposal table is re-cut accordingly: 1400h/1800h/1200h is the real conformance item of this round, not the object dictionary and not the abort table. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1045ccb3f7
ℹ️ 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".
…tself Codex on #127 again, two P2 findings on the revised text. Both correct, and both the mistake the previous commit was written to fix -- so the lesson is sharper than "have the standard": being told once was not enough either. **The communication records.** Same finished-device-versus-library confusion as the object dictionary. An application can register 1200h, 1400h and 1800h through the public AddU* methods, and the quoted footnote even allows them read-only, so zero source matches do not show the stack prevents a conformant device. What the stack does prevent is a *consistent* one, and that the application cannot supply: a write to 1600h/1A00h reaches the running PDO slot through ApplyTpdoMappingFromSdo, while nothing reads 1400h/1800h at all -- transmission type, inhibit time and event timer live only in ConfigureTpdo. An application-registered 1800h:02 would be a value the SDO server serves and accepts while the node keeps sending as before, so the master reads a promise the device does not keep. That is worse than the object being absent, and the missing wiring is internal. The item is therefore not "add the objects" but "connect the communication parameters to the PDO engine, as the mapping records already are". **The transmission modes.** That the table defines 02h-F0h and FCh/FDh does not oblige any device to support them; a value table fixes meanings, not obligations. A node that only does every-SYNC and event-driven is legitimate as long as it says so in a read-only record. Without a conformance clause requiring them they are scope candidates, not a Must, and what remains normative -- that the record matches what the device does -- folds into the item above rather than standing beside it. The question to ask of each line is not whether the standard says it, but what exactly the application cannot do for itself. Only the second one separates a library defect from a table row. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 139a7b4542
ℹ️ 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".
Codex on #127, third round. 1200h is the SDO server parameter record and has nothing to do with the PDO engine: CanOpenNode.cs:774 recognises a request by comparing cobId against CanOpenCobId.SdoRx(_nodeId) -- a fixed 0x600 + node id -- and answers on 0x580 + node id, never through an object dictionary entry. Folding it into one row with 1400h/1800h would have allowed exactly what the finding warns about: the PDO work counted as done while 1200h stays inert. Split into 2a and 2b. For 1200h the smaller of the two paths is a read-only record over the fixed default ids, which the footnote's "These may be ro" permits: the stack supports only those, so an rw record would promise a flexibility that does not exist -- the same defect as an application-registered 1800h:02, on the SDO path instead of the PDO one. Reading the COB-IDs out of the record is the larger option and is a separate decision. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 476b7cf9c6
ℹ️ 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".
Codex on #127, fourth round. The scope document confirmed the 0x20 server path against the standard and silently dropped the client half, which #38 and this repository's own priority review both record: a peer answering an upload initiate with 0x40 -- normal transfer, size not indicated, legal under §7.2.4.3.6 exactly as e and s are separate on the download side -- is not recognised, because CanOpenNode.cs:1564 compares for 0x41 exactly. The client ignores the response and the transfer runs into its timeout. Requirements derived from the document as written could therefore have closed #38 while leaving uploads from conformant peers broken. Both halves now sit in one item. The omission is the more annoying for being self-inflicted: 2026-09-15-open-issue-priority.md names the client path and the line number, so this was carried forward incompletely rather than missed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac061aba98
ℹ️ 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".
…s not wiring Codex on #127, fifth round, two findings, both correct. **A static read-only record is coherent without any wiring.** I claimed an application-registered 1800h:02 is necessarily a promise the device does not keep. Not in the static case: AddU8/AddU32 take OdAccess.ReadOnly, the SDO server refuses writes to such an entry (CanOpenNode.cs:1063), and the same application can pass the same values to ConfigureTpdo. The master then reads an accurate record with no OD-to-engine connection at all, so a coherent device is buildable today and 2a is not a compulsory requirement. What the application cannot resolve is narrower: a writable record, whose master-side write the engine never sees, and drift, where a later ConfigureTpdo call silently outdates the entry because nothing keeps the two together. The item is now a choice between those two shapes rather than a wiring mandate. **Inhibit time does not belong in a wiring item at all.** ConfigureTpdo takes transmission type, COB-ID and event-timer interval and no inhibit time (ICanOpenNode.cs:184-187), and the package README says so itself -- "a change-of-state TPDO is not rate-limited" (README.md:51). So it does not "live exclusively in ConfigureTpdo" as I wrote; it lives nowhere, and an effective 1800h:03 needs new scheduling behavior rather than connecting existing state. Folded into "wiring" it would look finished while staying inert. Split out as its own row. Eight over-readings across five rounds, three of them inside the correction of the round before. The count is now in the document's introduction, because the pattern is the most useful thing in it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd936bfc95
ℹ️ 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".
Codex on #127, sixth round, two findings, both correct. **The gap document still carried the conclusion the scope review had already withdrawn.** I added an "Aufgelöst" banner above the paragraph and left the paragraph itself saying that every node built with this stack is nonconformant once the premise is confirmed -- so a reader of the gap document got the banner and then the invalid inference anyway. The premise did turn out to be right; the conclusion never followed from it, because the standard binds a device and this is a library. The paragraph is rewritten rather than annotated. Same for the TIME line, which is now stated as checked and optional instead of a candidate. **Drift is resolvable by the application after all.** ObjectDictionary.WriteRaw and WriteUnsigned check only that the entry exists, never its access flags (ObjectDictionary.cs:131-141), so an application that calls ConfigureTpdo again can update the matching OD entry itself -- including a read-only communication record. Listing drift among the things the application "cannot resolve" would have promoted drift prevention into the proposed scope against this document's own criterion. The real limit is narrower and now says so: the reconciliation is neither automatic nor atomic, but it is possible. That leaves exactly one unresolvable case behind item 2a: a writable record whose master-side write the engine never sees. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67d87fbeb0
ℹ️ 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".
Codex on #127, seventh round, two findings, and both are the same shape as the banner one two rounds ago: the correction went into the body while the sentence introducing it, and the heading above it, kept asserting what the body withdraws. - "Was er verhindert, ist ein stimmiges Gerät, und das kann die Anwendung nicht ausgleichen" still said the opposite of the correction ten lines below, which establishes that a static read-only record matching the ConfigureTpdo values is coherent without any wiring. Rewritten to say so directly and to name the one case that stays open. - The section heading "Die Übertragungsart ist falsch kodiert" preserved exactly the wire-defect conclusion its own section retracts, and a reader skimming headings would have taken the retracted claim into the scope decision. Renamed to what survives: the enum cannot express the value table. The opening sentence now states the name/byte mismatch as a fact and immediately says why "mis-encoded" is the wrong word -- nothing encodes it. Three times now a correction has been added while the claim it corrects stayed in place, twice in a heading or lead sentence. A retraction that sits below the thing it retracts does not retract anything: whoever reads top-down, or skims, gets the original. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
2 participants
What does this change?
Die Gap-Analyse hat ihre CANopen-Zeilen ausdrücklich als Erinnerung statt Tatsache markiert, weil der Normtext fehlte. Er liegt jetzt vor (CiA 301 v4.2.0, Volltext, vom Maintainer bereitgestellt). Diese PR liest sie am Dokument nach und zitiert nach Abschnitt.
Die eigentliche Lehre steht nicht im Normtext
Dieses Dokument hat „die Norm definiert X" mit „die Implementierung muss X haben" gleichgesetzt — zwölf Mal über sieben Review-Runden, und mehrfach in der Korrektur der Runde davor. Codex hat jedes einzelne auseinandergenommen und in jedem recht behalten.
Den Normtext zu haben schützt also nicht davor, ihn zu überdehnen, und einmal darauf hingewiesen zu werden auch nicht. Zwei Fragen, die jetzt in der Einleitung des Dokuments stehen:
Was normativ feststeht
Pflichtobjekte sind drei.
1018hträgt in §7.5.2.21Category: Mandatory, die Objektübersicht führt esro M— neben1000hund1001h.Die PDO- und SDO-Records sind Pflicht, sobald das Gerät sie unterstützt. Fußnoten der Objektübersicht, wörtlich:
Wertetabelle der Übertragungsart:
00hsync acyclic ·01h–F0hjeder n-te SYNC ·F1h–FBhreserved ·FCh/FDhRTR-only ·FEh/FFhevent-driven.Was daraus nicht folgt
CiA 301 bindet ein Gerät; diese Pakete sind eine Bibliothek. Zurückgenommen:
ObjectDictionary.AddU32ist öffentlich,README.md:104zeigt den Weg. Was bleibt: der Standardweg führt still zum nicht konformen Knoten, und das README-Beispiel belegt selbst nur0x1000und0x2000— ein Pflichtobjekt von dreien.1800hmacht die Enum-Nummerierung nicht zum Draht-Defekt. Das Enum ist schon: byte, kein Pfad serialisiert seine Werte, und es steht in der API-Baseline — Umnummerieren wäre ein Bruch. Übrig: drei Werte können02h–F0hundFCh/FDhnicht ausdrücken.SdoAbortException.AbortCodeist ein roheruint; empfangsseitig geht nichts verloren. Übrig:0504 0003h, den der Blocktransfer erkennt und nicht melden kann.1400h/1800h/1200hbelegen keine Nichtkonformität — die Anwendung kann sie anlegen,roist erlaubt.Must.ro-Record ist stimmig — ohne jede Verdrahtung.OdAccess.ReadOnlywird vom SDO-Server geprüft (CanOpenNode.cs:1063), und dieselbe Anwendung kannConfigureTpdodieselben Werte geben. Ein kohärentes Gerät ist heute baubar.ObjectDictionary.WriteRawprüft keine Zugriffsflags (ObjectDictionary.cs:131-141), der Eintrag lässt sich also nachziehen — auch beiro. Die Grenze ist nur, dass der Abgleich weder automatisch noch atomar ist.ConfigureTpdo— die Signatur hat sie nicht (ICanOpenNode.cs:184-187), und die README sagt es selbst. Sie ist kein Verdrahtungs-, sondern ein Scheduling-Thema.Was als Zuschnitt übrig bleibt
1000h/1001hdurch den Stack,1018hper benanntem Helfer; README-Beispiel auf alle drei1400h/1800h: entweder statischround dokumentiert, oder beschreibbar und dann verdrahtetroerfüllbar1200halsro-Record über die festen0x600/0x580 + Node-ID1800h:030504 0003h31C3h, Zitat auf §7.2.4.3.161012hAus „sechs bis acht neue
Must" sind damit drei Entscheidungen und vier Kleinigkeiten geworden, jede mit benannter Belegstelle.Zugunsten des Codes
0000h,"123456789"→31C3h—ComputeCrc16Xmodemliefert exakt das. Kein Test pinnt den Wert, obwohl die Norm ihn als Vektor mitliefert.pst = 0erzwingen ist normkonform (§7.2.4.3.13).Zwei Defekte, am Text bestätigt statt begründet
eundsauf beiden Wegen. Server: cs0x20ist legal,CanOpenNode.cs:1037fängt ihn über die Expedited-Maske ab. Client:CanOpenNode.cs:1564vergleicht exakt auf0x41, lässt ein konformes0x40also liegen.ackseq + 1.Was diese PR nicht tut
Keine Anforderung, kein Code. Der Zuschnitt selbst ist die nächste Runde. Offen bleibt, was CiA 301 nicht entscheidet: bit-granulares PDO-Mapping und CiA 302/304/305.
Type of change
feat— new behaviour (minor release)fix/perf— bug or performance fix (patch release)docs/test/refactor/chore/ci— no releaseAcht
docs(reviews):-Commits. Kein Release.Checklist
dotnet build CanKit.Pro.sln -c Releasesucceeds — nicht gelaufen, kein C# geändertdotnet test CanKit.Pro.sln -c Releasepasses — nicht gelaufen, kein C# geändert31C3hnachgerechnetGeprüft:
eng/verify-requirements-traceability.pyexit 0,mkdocs build --strictmit denselben zwei Warnungen wie aufmain.Hinweis zum Merge: Codex' letzter Review lief auf
67d87fb(21:35). Für den finalen Headbf9246fist er nicht mehr angelaufen — 27 Minuten nach dem Push stand die Summary unverändert. Die Differenz zwischen beiden Commits sind genau die zwei Formulierungen, die Codex selbst verlangt hatte (Abschnittsüberschrift und Einleitungssatz). Checks und Bugbot sind aufbf9246fgrün.🤖 Generated with Claude Code
https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj