docs(reviews): record the CANopen scope decision and what it changed - #129
Conversation
Everything below existed only in conversation. The scope document now carries the decision it was written to support, and the gap review carries the provenance rule the remaining three products will need. **The decision (maintainer, 16.09.).** The EDS feeds the object dictionary *and* the PDO configuration -- one source for both. A coded minimum applies only when no EDS is present, with static read-only records. What an EDS states and the stack cannot honor is degraded and reported: not refused, which would leave the path unusable until everything is built, and above all not silently ignored, which is the promise-without-cover the whole review round was about. **What it dissolves.** Item 2a stops being a question: the choice between a static record and a wired one existed only while record and behavior came from two hands. Item 1 largely folds into the EDS path, since an EDS file has a MandatoryObjects section. **What it adds, and this is the interesting direction.** Transmission types 02h-F0h and the inhibit time become necessary although CiA 301 requires neither -- a real EDS carries values ConfigureTpdo cannot accept (the enum has no n-th-SYNC, the signature has no inhibit time). The table now records for each item whether its necessity comes from the standard or from the architecture, so nobody later reads an obligation into CiA 301 that is not there. **Three findings that were only spoken.** - The fallback *can* fill 1018h truthfully. The standard defines 0000 0000h at sub 01h as "invalid vendor-ID" -- a defined value, not an invention -- while 02h-04h are reserved at zero, and sub 00h ranges over 01h-04h. So sub0 = 01h with vendor-ID 0 is structurally conformant and says what is true. The remaining sub-indices are omitted, not zeroed. - The inhibit-time sub-entry is `Entry category: Optional`. The first draft never said so, which left it looking like an obligation. - FCh/FDh is not the same item as 02h-F0h: n-th SYNC is a value the API cannot express, RTR-only is behavior the stack does not have. The plumbing is there though -- IsRemoteFrame reaches the dispatcher as isRtr (CanOpenNode.cs:645, :703), used so far only by node guarding. And the value table is reachable with no API break at all, because the EDS path takes the raw byte and TpdoTransmission stays the convenience API. **EdsDcfNet is a dependency, not ours to fix.** The 1018h finding in its XDD classifier is out of this scope; CLAUDE.md already fixes that posture for the upstream package. What remains for us is a constraint, not a ticket: seeding from an XDD must not trust its MandatoryObjects, while reading from EDS/DCF is unaffected because the library parses the file's own section. **Provenance gets a third tier** -- measured, standard, secondary source, each requirement carrying its own. For CANopen the standard was available; for the other three it may not be, and a requirement resting on a poster must not look like one resting on a clause. That was the fault the twelve findings on #127 came from. With it, an assessment of how much normative text each remaining product actually needs, and the note that the ISOBUS data base answers a different layer than the open J1939-21 transport issues. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
PR SummaryLow Risk Overview The CANopen scope doc now records the 16.09 architecture decision for the device role: EDS drives both the object dictionary and PDO configuration; a coded fallback applies only without EDS; unsupported EDS values are degraded with reporting (including OD correction); runtime SDO writes to communication records pass through to the engine (not forced The norm-gap doc adds a three-tier provenance model (measured / norm / secondary source), marks CiA 301 as resolved, states CANopen scope step 2 done for the device role, and adds guidance on how much norm text other products need and that ISOBUS data is not the same layer as open J1939-21 transport issues. Reviewed by Cursor Bugbot for commit 9c94273. 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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: 50bc836fa5
ℹ️ 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".
…ahl korrigieren Der vorige Commit hat die alte Vorschlagstabelle (1, 2a, 2b, 2c, 3 …) durch eine neue mit zehn anders nummerierten Posten ersetzt, aber fünf Sätze zeigten weiter auf die alten Nummern. „Posten 3" kollidierte dabei aktiv: in der neuen Tabelle ist 3 das Fallback für 1018h und damit eine Normpflicht — gemeint war die Übertragungsart, die die Norm gerade nicht verlangt. Die Verweise benennen die Sache jetzt, statt zu nummerieren. Dieselbe Ursache in 2026-09-15-norm-gap.md, dort als Zahl: „zwei der Posten verlangt die Norm ausdrücklich nicht" — die Tabelle führt drei, seit FCh/FDh im selben Commit ein eigener Posten wurde. Also derselbe Fehler, den CLAUDE.md unter „Eine Zahl gehört an eine Stelle" beschreibt, und wieder still: beide Dokumente blieben für sich stimmig. Die Zahl ist ersetzt durch die Benennung, und die zweite Wiederholung („zehn Posten") zeigt jetzt auf das Scope-Dokument, statt den Wert zu kopieren. Dazu vier Zeilen auf die 100-Spalten-Konvention der Dateien umgebrochen. Geprüft: eng/verify-requirements-traceability.py exit 0; mkdocs build --strict mit denselben zwei Warnungen wie auf main (nav-Ziel 'api', mermaid-CDN durch den Proxy blockiert). 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: ae963a57dc
ℹ️ 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".
Alle fünf Befunde sagen dieselbe Sache über die Struktur: die Liste ist der Text, aus dem die SRS geschrieben wird, also ist eine Lücke in ihr keine Ungenauigkeit, sondern eine Anforderung, die es nie geben wird. Alle fünf am Code nachgeprüft, alle fünf zutreffend. - Übertragungsart 00h (synchron-azyklisch) fehlte. Das ist eine reine Abschreibfehler-Lücke: die README des Pakets führt "0 (synchronous-acyclic), 2-240 ... 252/253" als nicht unterstützt, und ich habe die Liste ohne den ersten Eintrag übernommen. 00h ist eigenes Verhalten - auf SYNC senden, aber nur bei Änderung -, das weder EventDriven noch Synchronous leistet. - Synchrone RPDO fehlte ganz. Der Posten war über TpdoTransmission formuliert und damit sendeseitig; ConfigureRpdo hat gar kein Übertragungsart-Argument (ICanOpenNode.cs:192) und HandleRpdo schreibt sofort ins OD, statt bis SYNC zu halten (CanOpenNode.cs:1697). - EDS-Zugriffsrechte auf 1600h/1A00h wären wirkungslos. Der Mapping-Pfad wird bei CanOpenNode.cs:1022 vor der generischen OD-Prüfung abgezweigt, und HandlePdoMappingSdoRequest bricht nur bei ungültigem CS und fehlendem Subindex ab - OdAccess fragt es nie. Geladene ro-Flags blieben Dekoration. - Degradieren war zu schwach formuliert: den Originalwert ins OD zu kopieren und daneben anders zu laufen ist wieder die Zusage ohne Deckung. Der degradierte Wert gehört ins OD, oder der Eintrag weg, oder das PDO aus. Dazu der offene Punkt "schreibbare Kommunikationsrecords" als Posten 14 in die Tabelle, statt nur unter "Was offen bleibt" zu stehen - die Ratsche deckt ab, was in der Tabelle steht. Die Entscheidung selbst bleibt offen und ist die des Maintainers: durchreichen oder beim Laden auf ro zwingen. Geprüft: eng/verify-requirements-traceability.py exit 0; mkdocs build --strict mit denselben zwei Warnungen wie auf main. 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: f21172f3f7
ℹ️ 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".
…#129) Zwei weitere Befunde, beide am Code nachgeprüft, beide zutreffend. Posten 5 war als reine API-Erweiterung beschrieben - "EDS-Pfad nimmt das rohe Byte". Das ist die halbe Arbeit. HandleSync sendet jedes synchrone TPDO bei jedem SYNC (CanOpenNode.cs:868-880); ohne Zähler je TPDO würde der Stack den Wert 02h annehmen, im Record anzeigen und weiter bei jedem SYNC senden - also genau die OD/Laufzeit-Inkonsistenz, gegen die die Entscheidung geschrieben ist. Die Zeile nennt jetzt beide Hälften. Das ist dreimal auf dieser PR dieselbe Ursache: die Spalte "Art der Arbeit" beschrieb die API-Oberfläche statt des Verhaltens (vorher schon bei RPDO und bei FCh/FDh). Wo eine Zeile eine API nennt, ist die Frage, was die Engine zusätzlich tun muss. Posten 15 neu: Reset Communication. Der Handler wechselt nur den Zustand und sendet Bootup (CanOpenNode.cs:848-857). Interessant ist die Begründung in der README (src/CanKit.Pro.CANopen/README.md:56-57): nicht wiederhergestellt, "because they do not live in the OD". Diese Begründung trägt nur, solange es keine Quelle gibt - mit der EDS gibt es sie, und damit wird aus einer dokumentierten Einschränkung eine offene Anforderung. Die README bleibt unverändert richtig, weil sie das heutige Verhalten beschreibt; sie wird mit der Umsetzung nachzuführen sein, nicht vorher. Geprüft: eng/verify-requirements-traceability.py exit 0; mkdocs build --strict mit denselben zwei Warnungen wie auf main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
macOS-Leg auf
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 899d183ef0
ℹ️ 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-Runde auf 899d183, alle vier zutreffend. Diesmal am CiA-301-Text statt am Code nachgeschlagen - und der Text ist präziser als meine Formulierungen. Posten 11 (00h): "nur bei Änderung" war zu eng. §7.2.2.2 sagt "only if an event occurred before the SYNC", Tabelle 72 "the CANopen device internal event is given", und §7.2.2.3 zählt Ereignisse als anwendungsspezifisch auf. Ein expliziter TriggerTpdoAsync ist ein solches Ereignis. Also ein Latch, gesetzt von Zustandsänderung UND explizitem Trigger, verbraucht beim SYNC. Posten 6 gespalten in 6a/6b. Tabelle 72 unterscheidet FDh (RTR-only, event-driven: Sampling bei RTR, sofort senden) von FCh (RTR-only, synchron: Sampling bei jedem SYNC, puffern, den gepufferten Wert auf RTR senden). Ein gemeinsamer "OD lesen und antworten"-Handler erfüllt FDh und verletzt FCh. Posten 16 neu: auch die Fallback-Mappingrecords 1600h/1A00h gehören auf ro. Posten 4 deckte nur 1400h/1800h/1200h, Posten 13 nur EDS-Flags - das Fallback wäre also remappbar geblieben, obwohl die Entscheidung ihm ro zusagt. Posten 15 erweitert: Reset Communication stellt auch das codierte Fallback wieder her. Ohne EDS gäbe es sonst gar keine Quelle, aus der zurückzuholen wäre, während ConfigureTpdo und SDO-Remapping den Zustand weiter ändern. Zwei eigene Befunde beim Lesen von Tabelle 72, beide normativ: - Posten 17: "An attempt to change the value of the transmission type to any not supported value shall be responded with ... abort code 0609 0030h". Die Kehrseite des Degradierens. Der Code fehlt im Enum - von den 0609h-Codes führt SdoAbortCode.cs nur 0609 0011h -, gehört also zu Posten 8. - Posten 18: 1800h:04 ist reserved und "shall not be implemented", Zugriff führt auf 0609 0011h. Dieser Code ist vorhanden, nur das Verhalten fehlt. Posten 7 geschärft: die Inhibit Time ist normativ das Mindestintervall "if the transmission type is set to FEh and FFh", gilt also nicht für die synchronen Arten. Dazu sechs Anführungszeichen auf die Dateikonvention normalisiert. Geprüft: eng/verify-requirements-traceability.py exit 0; mkdocs build --strict mit denselben zwei Warnungen wie auf main. 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: 9ea6b5c02b
ℹ️ 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".
…n Einträgen Drei Codex-Befunde auf 9ea6b5c, alle am Normtext bzw. am Code nachgeprüft. Posten 19 neu, und er ist normativ statt architekturgetrieben - dieselbe Fußnotenlogik wie Posten 4, nur auf Dienste angewandt, die der Fallback ebenfalls anbietet: 1005h "Mandatory, if PDO communication on a synchronous base is supported" 1006h "Mandatory for SYNC producers" 1014h "Mandatory, if Emergency is supported" Der Stack kann alle drei (StartSyncProducer, SendEmcyAsync, synchrone TPDOs; FR-CO-010/011 sind Must). Ohne die Records hätte auch das Fallback Verhalten, das sein OD nicht beschreibt. 1006h stand nicht im Befund - es ergibt sich, wenn man dieselbe Logik einen Schritt weiterführt, weil der Stack ein SYNC-Produzent ist. Posten 15 auf ResetNode erweitert. Beide Kommandos teilen sich heute denselben Zweig (CanOpenNode.cs:848-857), und ResetNode ist der weitergehende - "full application reset (implies reset communication)" (Nmt/NmtState.cs:39). Nur ResetCommunication zu fordern hätte 0x81 den Zustand behalten lassen. Posten 17 eingeschränkt auf beschreibbare Einträge. Ist der Record ro - über Posten 4 oder je nach Entscheidung über Posten 14 -, scheitert der Download vorher an der Zugriffsprüfung mit 0601 0002h (AttemptWriteReadOnly, im Enum vorhanden). 0609 0030h unbedingt zu fordern hätte der Zugriffspolitik widersprochen und die falsche Reihenfolge in die SRS geschrieben. Geprüft: eng/verify-requirements-traceability.py exit 0; mkdocs build --strict mit denselben zwei Warnungen wie auf main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
…rgibt Beim Gegenlesen des eigenen Diffs gefunden, nicht von einem Bot. Die Entscheidung sagte "Dessen Records sind statisch ro" - pauschal für alle Fallback-Records. Die Norm trägt das nur für einen Teil davon. Fußnote * der Objektübersicht erlaubt ro ausdrücklich für "PDO communication parameter and PDO mapping object entries", und für sonst nichts; Fußnote ** für die SDO-Parameter nennt kein ro. Wo ein Objekt selbst rw vorschreibt, ist ro also keine zulässige Vereinfachung, sondern eine Abweichung. Zwei Posten waren dadurch falsch: - Posten 4 führte 1200h zusammen mit 1400h/1800h als "Pflicht bei PDO-/SDO- Support, statisch ro". Die Objektdefinition sagt aber "Category Conditional / Index 1200h: Optional" - Pflicht sind erst 1201h-127Fh je weiterem SDO-Server. Als eigener Posten 4b abgetrennt, mit dem Hinweis, dass sub0 und sub01 ohnehin const sind, wenn er gebaut wird - aus der Objektdefinition, nicht aus der PDO-Fußnote. - Posten 19 (1005h/1006h/1014h) erbte die pauschale ro-Zusage. Alle drei sind laut Objektdefinition rw. Sie fallen damit unter dieselbe Frage wie Posten 14, und das steht jetzt dort. Dazu zwei Aufräumungen: Posten 8 nennt jetzt beide fehlenden Abort-Codes, damit der Verweis aus Posten 17 nicht einseitig bleibt, und Posten 15 zitierte CanOpenNode.cs:848-857 zweimal in derselben Zelle. Geprüft: eng/verify-requirements-traceability.py exit 0; mkdocs build --strict mit denselben zwei Warnungen wie auf main. 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: 7d2593259e
ℹ️ 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".
…osten 20 Zwei Codex-Befunde auf 7d25932, beide gegen Posten 19 - also gegen den Posten, den ich eine Runde zuvor selbst angelegt hatte. Der erste ist ein Widerspruch im eigenen Text. Posten 19 schickte 1005h/1006h/ 1014h "unter dieselbe Frage wie Posten 14", und Posten 14 erlaubt als eine von zwei Varianten, Records beim Laden auf ro zu zwingen. Zwei Absätze weiter oben steht aber, dass genau das bei einem normativ rw-Objekt eine Abweichung wäre. Die Zeile hätte also erlaubt, was derselbe Commit ausgeschlossen hat. Jetzt steht dort, dass die ro-Variante für diese drei nicht offensteht und ihre Schreibzugriffe den Dienst erreichen müssen - mit der einen Ausnahme, die die Norm selbst nennt: 1005h darf const sein, "if the COB-ID is not changeable". Der zweite ist die Gegenrichtung zu Posten 14 und deshalb ein eigener Posten 20. StartSyncProducer setzt nur _syncProducerInterval und plant den Tick (CanOpenNode.cs:320-330), StopSyncProducer ebenso (:333-341); das OD erfährt nichts. 1006h bliebe auf seinem Anfangswert, und 0000 0000h heißt normativ "transmission of SYNC messages shall be disabled" - das OD sagte also SYNC aus, während der Knoten sendet. Bisher deckte die Liste nur OD -> Laufzeit ab. Geprüft: eng/verify-requirements-traceability.py exit 0; mkdocs build --strict mit denselben zwei Warnungen wie auf main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
…sten Instanz Das Review dieser PR hat die Postenliste auf ein Vielfaches ihres ersten Entwurfs gebracht, und beim Durchsehen der Befunde fallen sie in vier Muster, nicht in sechzehn Einzelfälle: 1. Die Zeile beschrieb eine API statt des Verhaltens. Dreimal: 02h-F0h braucht einen SYNC-Zähler, die synchrone RPDO die Empfangsseite, FCh einen gepufferten Wert. 2. Der Posten galt nur für einen der beiden Pfade. Dreimal, und immer fiel der Fallback durch, weil er der jüngere Pfad ist. 3. Der Posten galt nur in Richtung OD -> Laufzeit. Die Gegenrichtung fehlte, bis StartSyncProducer sie sichtbar machte. 4. Die Zusage im Text war breiter als ihr Beleg. Aus der ro-Fußnote für PDO-Records wurde kurzzeitig ein ro für alle Fallback-Records. Als Abschnitt vor "Was offen bleibt" aufgenommen, weil der nächste Schritt die Liste in Anforderungen übersetzt und J1939, UDS und ISO-TP dieselbe Übung noch vor sich haben. Keine der vier Fragen ist CANopen-spezifisch. Ohne Zahlenangabe zum Umfang: die Postenzahl steht in der Tabelle darüber, und eine zweite Stelle, die sie nacherzählt, geht beim nächsten Zuwachs still falsch - derselbe Fehler, den ae963a5 in diesem Branch behoben hat. Die beiden "Dreimal" bleiben, weil sie ihre Fälle im selben Satz aufzählen. Geprüft: eng/verify-requirements-traceability.py exit 0; mkdocs build --strict mit denselben zwei Warnungen wie auf main. 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: a886ebfdf8
ℹ️ 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".
…kunft Zwei Codex-Befunde auf a886ebf, beide zutreffend - und beide sind Instanzen der Prüffragen, die derselbe Commit hinzugefügt hat: Frage 2 (gilt der Posten für beide Pfade?) und Frage 3 (gilt er in beide Richtungen?). Posten 21, Heartbeat. Der Befund stimmt in der Sache: StartHeartbeatProducer (ICanOpenNode.cs:96) und AddHeartbeatConsumer (:104) ändern nur internen Zustand, und die README führt 0x1016/0x1017 unter den fehlenden Kommunikationsobjekten. Seine normative Einordnung stimmt aber nicht, und das ist wichtig genug, um es in die Zeile zu schreiben: 1016h ist Category Optional, und 1017h ist Conditional - "Mandatory, if guarding not supported". Dieser Knoten unterstützt Guarding, er beantwortet Guarding-RTRs. Die Klausel greift also nicht. Herkunft daher Architektur, nicht Norm - genau die Unterscheidung, für die die Spalte existiert. Posten 22, 1001h und EMCY. SendEmcyAsync (CanOpenNode.cs:352-358) baut die Nachricht und sendet, ohne das OD zu berühren, während EmcyMessage.cs:13 Byte 2 ausdrücklich als "mirror of OD 0x1001" dokumentiert. Das Paket hält also eine Invariante nicht, die es selbst aufschreibt. Gegenrichtung wie Posten 20, und diesmal betrifft es beide Pfade, nicht nur das Fallback. Dazu 1007h geprüft und zu Posten 10 gelegt: Category Optional, wird nicht gebaut. Es steht in derselben README-Zeile wie 1005h/1006h, und ohne diesen Vermerk käme es beim nächsten Durchsehen als vermeintliches Übersehen wieder. Geprüft: eng/verify-requirements-traceability.py exit 0; mkdocs build --strict mit denselben zwei Warnungen wie auf main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
|
@codex review Anlass, damit der Kommentar nachvollziehbar bleibt: alle Checks auf Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7664f52e2
ℹ️ 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".
…erter Fall Codex-Befund auf b7664f5, zutreffend: Posten 21 war ausdrücklich fallback-only und sagte nichts über Richtungen. Eine EDS mit 1016h-Konsumenten oder 1017h ungleich 0 erreicht die Engine nicht, weil Posten 1 nur die PDO-Konfiguration befüllt und Heartbeat keine ist; und StartHeartbeatProducer (CanOpenNode.cs:265-275) wie AddHeartbeatConsumer (:290) fassen das OD nicht an. Posten 21 gilt jetzt für beide Pfade, mit den gemessenen Fundstellen. Der eigentliche Punkt ist aber, dass dies die dritte Runde in Folge war, in der derselbe Zusammenhang je Dienst nachgezogen wurde - SYNC in Posten 20, EMCY in 22, jetzt Heartbeat in 21. Drei Instanzen sind eine Regel, kein Zufall, und ein vierter Dienst würde als vierter Befund auffallen. Posten 23 hält sie deshalb allgemein fest: für jedes Kommunikationsobjekt, das der Knoten anbietet, stimmen OD und Laufzeit in beiden Richtungen und in beiden Pfaden überein. Die bekannten Instanzen stehen als Verweis darin, damit die Regel belegt bleibt und nicht als Absichtserklärung dasteht. Das ist derselbe Schritt, den a886ebf für die Prüffragen gemacht hat, eine Ebene tiefer: nicht die nächste Instanz, sondern das Muster. Geprüft: eng/verify-requirements-traceability.py exit 0; mkdocs build --strict mit denselben zwei Warnungen wie auf main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
|
@codex review Letzter Head ist Neu seit Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1325a77623
ℹ️ 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-Befund auf 1325a77, zutreffend - und der Befund, um den ich beim Anstoßen des Reviews ausdrücklich gebeten hatte: die Verallgemeinerung greift zu weit. "Ein SDO-Schreibzugriff erreicht den Dienst" gilt unqualifiziert auch für die Objekte, die dieselbe Tabelle absichtlich ro setzt - 1000h/1001h/1018h in Posten 3, die Fallback-PDO-Records in 4 und 16. Wörtlich in Anforderungen übersetzt, machte die Regel entweder diese Records beschreibbar oder erzeugte einen Widerspruch zu Posten 17, der selbst sagt, dass ein Download auf einen ro-Eintrag an der Zugriffsprüfung mit 0601 0002h endet. Die Zeile sagt jetzt "angenommener" Schreibzugriff und benennt die zweite Hälfte ausdrücklich: auf einem ro-Eintrag ist die Ablehnung das richtige Verhalten, keine Lücke. Das ist derselbe Fehler wie beim ro-Freibrief in 7d25932 - eine Zusage breiter formuliert als ihr Beleg, also Prüffrage 4 -, diesmal auf eine Regel statt auf einen Einzelposten angewandt. Eine Verallgemeinerung ist dafür anfälliger als die Instanzen, aus denen sie stammt, und das war der Grund, sie vor der SRS-Übersetzung gegenlesen zu lassen. Geprüft: eng/verify-requirements-traceability.py exit 0; mkdocs build --strict mit denselben zwei Warnungen wie auf main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
macOS auf
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac61057ec2
ℹ️ 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".
… Zugriffsweg Codex-Befund auf ac61057, zutreffend: die Einschränkung aus ac61057 war zu eng. Sie sprach von "angenommenen SDO-Schreibzugriffen" und deckte damit nur einen der beiden Wege ins Objektverzeichnis ab. Gemessen: ObjectDictionary ist öffentlich auf ICanOpenNode (:33), und WriteRaw prüft nur die Existenz des Eintrags, nie OdAccess (ObjectDictionary.cs:131-142). Die Anwendung kann also 1006h direkt setzen, das OD kündigt eine neue SYNC-Periode an, und der Produzent behält seinen Zeitplan - genau der Fall, den die Zeile ausschließen soll, nur am SDO-Server vorbei. Statt einer dritten Einschränkung jetzt ein Kriterium, das beide Wege trägt: maßgeblich ist die tatsächlich erfolgte Änderung, nicht der versuchte Zugriff. Was das OD übernimmt, erreicht den Dienst - gleich ob über SDO oder direkt. Ein mit 0601 0002h abgewiesener Download fällt nicht darunter, weil er nichts ändert, und seine Ablehnung bleibt richtig. Dazu EntryWritten (ObjectDictionary.cs:141) als vorhandene Naht benannt, damit die Anforderung nicht nach einem neuen Mechanismus klingt. Die Zeile ist damit dreimal angefasst worden: zuerst zu weit, dann zu eng, nun am Kriterium statt am Weg. Das ist der Verlauf, den CLAUDE.md unter "Remove the broken half, not the whole assertion" beschreibt - und der Grund, warum diese Verallgemeinerung vor der SRS-Übersetzung gegengelesen gehörte. Geprüft: eng/verify-requirements-traceability.py exit 0; mkdocs build --strict mit denselben zwei Warnungen wie auf main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
|
@codex review Head ist Die Formulierung ist bewusst so gewählt, dass sie keinen weiteren Zugriffsweg aufzählen muss. Falls sie dadurch etwas einschließt, das sie nicht soll, oder ein Weg existiert, der das OD ändert ohne unter „erfolgte Änderung" zu fallen, ist das der Befund, den ich suche. Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfb60aae3e
ℹ️ 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-Befund auf bfb60aa, zutreffend - und er trennt sauber, was diesmal richtig und was falsch war: das Kriterium aus bfb60aa hält, die Naht, die ich daneben benannt habe, deckt es nicht ab. Gemessen, und das Paket dokumentiert es selbst (ObjectDictionary.cs:33): "raised after a value-mutating write (WriteRaw / WriteUnsigned) completes successfully. Registration via the Add* methods does not raise it." Und Add (:210-218) ersetzt einen bestehenden Eintrag per Indexer-Zuweisung, ohne etwas auszulösen. Ein AddU32(0x1006, ...) ändert das OD also tatsächlich, fällt unter "erfolgte Änderung" - und die als Mechanismus genannte Naht schweigt. Korrigiert wurde deshalb nur diese Hälfte: EntryWritten steht jetzt als halbe Naht da, mit dem fehlenden Fall daneben, und die Zeile verlangt ausdrücklich das Ergebnis statt des Weges - Naht erweitern oder das Ersetzen verwalteter Records unterbinden. Das ist der Punkt, den CLAUDE.md unter "Remove the broken half" macht: die Versuchung wäre gewesen, die ganze Zeile noch einmal umzuschreiben, nachdem sie schon dreimal dran war. Falsch war aber nur die Mechanismusangabe. Geprüft: eng/verify-requirements-traceability.py exit 0; mkdocs build --strict mit denselben zwei Warnungen wie auf main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
Windows/net48 auf
|
| Bedingung | Läufe | Ergebnis |
|---|---|---|
| net10.0, unbelastet | 12 | 12 bestanden |
| net10.0, 3× CPU-Überlast | 8 | 8 bestanden |
20 von 20 unter Bedingungen, die härter sind als ein geteilter CI-Runner. Die Erklärung „ausgelasteter Runner" trägt hier also nicht — der Unterschied ist das Zielframework, und die Arithmetik des Tests passt ohnehin nicht dazu: der neu gesetzte Timer läge bei 2000 ms, gefeuert wurde innerhalb von 850 ms. Die naheliegendere Lesart ist die, gegen die der Test geschrieben wurde — der ersetzte Timer feuert trotzdem.
Ausführlich mit der Begründung auf #92, inklusive des Hinweises, dass es dort anders geformt ist als die drei gelisteten Fälle und ein eigenes Ticket verdienen könnte — das ist deine Entscheidung, nicht meine.
Keinen zweiten Wiederholungslauf. CLAUDE.md lässt einen zu, und den habe ich auf ac61057 verbraucht. Ein zweiter wäre hier ohnehin das Falsche: ich behaupte gerade nicht, dass der Fehlschlag zufällig ist.
Damit steht die PR so da: alles grün außer diesem einen net48-Leg, dessen Ursache belegt außerhalb dieses Branches liegt und auf #92 dokumentiert ist. Die Merge-Entscheidung gehört damit auf diese Tatsachen gestellt, nicht auf einen grünen Haken.
@codex review
Head ist 47b57df. Neu ist nur die Korrektur an Posten 23: EntryWritten steht jetzt als halbe Naht da (feuert bei WriteRaw/WriteUnsigned, nicht bei Add*), und die Zeile verlangt das Ergebnis statt des Weges. Das war dein letzter Befund.
Generated by Claude Code
|
Codex Review: Didn't find any major issues. Another round soon, please! 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". |
FERTIG — mergebarHead ist Alle Threads geschlossen: 22 von 22 aufgelöst, keiner offen. Codex hat auf Checks: alle grün außer dem net48-Leg auf Windows, und das sage ich hier statt es einen grünen Haken vortäuschen zu lassen:
Die PR-Beschreibung ist auf den aktuellen Stand gebracht; sie stammte von Zwei Dinge, die nach dem Merge offen bleiben — beide deine Entscheidung, keine von beiden blockiert diese PR:
Ab jetzt ist der Branch eingefroren. Kommt ein Befund herein, ist dieses Signal in dem Moment widerrufen, und ich sage das, bevor ich mit der Untersuchung anfange. Generated by Claude Code |
„FERTIG — mergebar" ist hiermit widerrufenDer Maintainer hat die offene Frage entschieden: Schreibzugriffe auf die PDO-Kommunikationsrecords werden bis zur Engine durchgereicht, volle Interop. Nicht die Damit ist eine Aussage im Dokument falsch geworden — dort steht bei diesem Posten „offene Entscheidung des Maintainers", und der Absatz darunter lässt beide Wege offen. Das ist lebender Text, der als aktuell gelesen wird, kein datierter Vermerk. Er wird nachgeführt. Ich sage den Widerruf vor der Änderung, nicht danach: ein Signal, das seinen Zustand überlebt, ist genau das Rennen, das die Regel verhindern soll. Nächster Stand folgt, wenn Checks und Review auf dem neuen Head durch sind. Generated by Claude Code |
…reicht Der Maintainer hat die letzte offene Frage dieses Dokuments beantwortet: Schreibzugriffe auf die PDO-Kommunikationsrecords werden bis zur Engine durchgereicht und wirken wirklich. Die billigere Gegenvariante - die Records beim Laden auf ro zwingen - ist nicht gewählt. Damit schließt der Zuschnitt die Interop-Lücke, die die README des Pakets bisher als bekannte Einschränkung führt (README.md:47, "a master cannot configure this node purely over the OD"), statt sie durch einen Schreibschutz zu zementieren. Nachgeführt an vier Stellen, weil die Entscheidung überall durchschlägt, wo das Dokument die Frage noch offen führte: - Die Entscheidungsliste oben bekommt den Punkt als fünften. - Posten 14 ist nicht mehr "offene Entscheidung des Maintainers", sondern eine Architekturentscheidung mit dem zu bauenden Weg: 1800h:02, 1800h:03, 1400h:02 und die COB-ID-Subs. - Posten 17 verwies auf "Posten 14 je nach Entscheidung" - diese Wahl gibt es nicht mehr, der ro-Fall ist jetzt ausschließlich das Fallback (4 und 16). - Posten 19 verwies auf "die ro-Variante aus Posten 14" - dito; die Entscheidung zeigt ohnehin in dieselbe Richtung wie das, was dort für 1005h/1006h/1014h ohnehin gilt. Der Abschnitt "Was offen bleibt" trug den Punkt als offene Wahl. Er steht jetzt davor, als Begründung der getroffenen Entscheidung, und unter der Überschrift bleibt nur noch, was CiA 301 tatsächlich nicht entscheidet. Ein Dokument, das eine beantwortete Frage als offen führt, ist die Art stale Aussage, die CLAUDE.md unter lebendem Text beschreibt. Geprüft: eng/verify-requirements-traceability.py exit 0; mkdocs build --strict mit denselben zwei Warnungen wie auf main. 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: e586fe9bf0
ℹ️ 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".
Vom Maintainer gefunden: das Dokument ist durchgehend aus der Geräteperspektive geschrieben - eigenes Objektverzeichnis, eigenes 1018h, eigene PDO-Records, die ein fremder Master beschreibt -, sagt das aber nirgends. Daneben behauptet das Gap-Review "CANopen ist die erste Runde, die durch ist". Die SRS kennt beide Rollen ausdrücklich: die Stakeholder-Tabelle nennt "CANopen-Master/-Node-Anwendung", FR-CO-007 verlangt NMT-Master und NMT-Node, FR-CO-002/003 sprechen von entfernten OD-Einträgen. Die API trägt sie ebenso - SendNmtCommandAsync(..., targetNodeId), SdoUploadAsync(serverNodeId, ...), AddHeartbeatConsumer, StartSyncProducer. Die SRS muss dafür also nicht geändert werden; das Scope-Dokument schon. Es ist Prüffrage 4 aus dem Dokument selbst, auf das Dokument als Ganzes angewandt: die Zusage ist breiter als ihr Beleg. Nicht weil etwas Falsches darin steht, sondern weil die Reichweite fehlt. Korrigiert an drei Stellen, ohne die zweite Rolle zuzuschneiden: - Der Entscheidungsabschnitt beginnt jetzt damit, was er NICHT ist, samt der absehbar anders gelagerten offenen Fragen der Master-/Tool-Rolle: die EDS des fremden Knotens statt der eigenen, Dekodieren fremder PDOs beim Beobachten, Knoten-Scan über 1000h/1018h, und ob Flying Master noch der Ausnahmekandidat ist, als den die GAP-Analyse ihn führt - jenes Urteil ist unter der Geräteannahme gefällt worden. - Das Gap-Review sagt "die Geräterolle als erste durch" statt "CANopen durch". - Und "Schritt 2 abgeschlossen" trägt dieselbe Einschränkung. Der Zuschnitt der zweiten Rolle bekommt eine eigene Runde nach der Übersetzung dieser Liste in Anforderungen (Maintainer, 16.09.). Geprüft: eng/verify-requirements-traceability.py exit 0; mkdocs build --strict mit denselben zwei Warnungen wie auf main. 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: 81dc666e2a
ℹ️ 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 review Head ist Seit deinem letzten Review (
Worauf ich besonders gegengelesen haben möchte: ob die Einschränkung weit genug reicht. Wenn noch eine Stelle im Dokument implizit für beide Rollen spricht, während sie nur die Geräterolle belegt, ist das derselbe Fehler nur an anderer Stelle. Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81dc666e2a
ℹ️ 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".
…re Antwort Posten 15, Reset. Die beiden Kommandos wurden gleich behandelt. Die Norm trennt sie: "Reset communication: In this NMT sub-state the parameters of the communication profile area are set to their power-on values" - also 1000h-1FFFh und gerade nicht die Anwendungsobjekte ab 2000h, die eine reale EDS mitbringt. 1011h trennt es zusätzlich über die Sub-Indizes (Reset Node für 01h-7Fh, Reset Communication nur für 02h). Eine Anforderung, die beide gleich behandelt, wirft in einem Fall Anwendungszustand weg und ist im anderen zu kurz. Posten 24 neu, Guard Time 100Ch und Life Time Factor 100Dh. Der Befund bot zwei Auswege an: Records samt Verdrahtung bauen, oder den RTR-Responder nicht mehr als Beleg für Guarding-Unterstützung nehmen. Die Norm hat einen dritten und genaueren: beide Objekte sind "Mandatory, if node guarding is supported" - Posten 21 beruft sich genau darauf, kann also nicht die eine Folge nehmen und die andere lassen. Die Zugriffsart fällt dabei von selbst, "rw; ro, if life guarding is not supported", und produzentenseitiges Life Guarding hat der Stack nicht (README.md:54-55). Also beide anlegen, beide ro, kein neues Verhalten. Posten 21 verweist jetzt auf diesen Preis. Die Rollengrenze aus 81dc666 war zu absolut formuliert. Die Befundabschnitte oben führen #38 ausdrücklich mit beiden Hälften, und die Client-Seite (CanOpenNode.cs:1564) gehört zur Master-Rolle - sie ist hier belegt und fällt nicht in die zweite Runde zurück. Die Grenze gilt für Entscheidung und Postenliste, nicht rückwirkend für die Analyse darüber. Posten 14 verwies für die Gegenrichtung auf Posten 20. Der betrifft aber nur 1006h und den SYNC-Produzenten; für die PDO-Records trägt die allgemeine Regel aus Posten 23. Falscher Querverweis, korrigiert. Und die Kopfzeile sagte "Erste von vier Zuschnitt-Runden" - schon überholt, seit CANopen zwei bekommt. Die Zahl ist raus und zeigt auf das Gap-Review, statt neben ihm zu veralten. Derselbe Fehler wie in ae963a5, dritte Wiederholung. Geprüft: eng/verify-requirements-traceability.py exit 0; mkdocs build --strict mit denselben zwei Warnungen wie auf main. 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: f944820233
ℹ️ 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".
…d zurückgewiesen Drei Codex-Befunde, zwei zutreffend, einer sachlich falsch. Zurückgewiesen: der Befund behauptet, CiA 301 kehre die Reihenfolge um - 1200h sei Pflicht bei Default-Server, 1201h-127Fh optional. Der Normtext sagt wörtlich das Gegenteil: "Category Conditional / Index 1200h: Optional / Index 1201h to 127Fh: Mandatory for each additionally supported SDO server". Die Kategorie bleibt also stehen wie sie ist. Sein Kern trifft aber trotzdem, nur eine Ebene tiefer: der Knoten betreibt den Default-Server auf 0x600/0x580 + Node-ID (CanOpenNode.cs:774) immer, und ein Fallback-OD ohne 1200h beschreibt weniger, als der Knoten tut. Posten 4b sagt deshalb jetzt "wird gebaut, aber nicht wegen der Norm" - dieselbe Begründung wie bei den Posten 19 und 21, und die Herkunftsspalte hält beides getrennt. Posten 25 neu, COB-ID-Steuerbits. Tabelle 70: der Eintrag in 1400h:01/1800h:01 ist keine CAN-ID, sondern ein 32-Bit-Wort mit valid (1b = "PDO does not exist / is not valid"), RTR (1b = "no RTR allowed on this PDO") und frame (1b = 29-Bit). ConfigureTpdo/ConfigureRpdo nehmen ihr uint? als blanken Bezeichner. Eine EDS, die ein ungenutztes PDO über Bit 31 abschaltet, würde beim rohen Durchreichen eingeschaltet - mit dem Flag-Wort als CAN-ID. Hängt an Posten 6a/6b für RTR und an Posten 2 für das, was der Stack nicht kann. Posten 26 neu, EDS-Attribut PDOMapping. HandlePdoMappingEntryWrite prüft die Richtungs-Zugriffsart als Ersatz für Mappbarkeit (CanOpenNode.PdoMapping.cs:229-233): lesbar genügt für ein TPDO. Ein Eintrag mit PDOMapping=0 ginge durch. Der Abort-Code ist vorhanden und verdrahtet - es fehlt das Kriterium, nicht der Code. Ausdrücklich ein anderer Posten als 13/16: dort die Zugriffsart des Mapping-Records, hier die Mappbarkeit des Ziels. Geprüft: eng/verify-requirements-traceability.py exit 0; mkdocs build --strict mit denselben zwei Warnungen wie auf main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
Obergrenze: noch zwei Reviewrunden, dann wird eingefrorenMaintainer-Entscheidung, hier festgehalten, damit sie nachprüfbar ist und nicht stillschweigend gedehnt wird. Der Grund. Die Art der Befunde hat sich verschoben. Die frühen Runden fanden Fehler in diesem Text — tote Verweise auf gelöschte Postennummern, Zahlen, die an zweiter Stelle veralteten, Zusagen breiter als ihr Beleg. Die letzten Runden finden neue CANopen-Oberfläche, die vorher niemand gelistet hatte: die COB-ID-Steuerbits aus Tabelle 70, das Das ist kein Nachlassen der Qualität, sondern die Vollständigkeitsübung, die tut, was sie soll — und genau deshalb hat sie kein natürliches Ende. Die Frage lautet „ist die Liste vollständig?", CiA 301 hat gut zweihundert Seiten, und jeder Durchgang über eine gewachsene Liste findet mehr. Die Zählung:
Ein Befund, der nach Runde 2 noch eintrifft, wird beantwortet — aber er wird ein Ticket, kein weiterer Commit hier. @codex review Head ist Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c96e656a72
ℹ️ 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".
…, Bitlänge
Drei Befunde, alle zutreffend, alle am Code bzw. Normtext nachgeprüft.
Posten 5 auf 01h erweitert, und das ist der schärfste der drei: die Zahl war
nicht nur unabgedeckt, ein roher Cast wäre aktiv falsch. Die Norm meint mit
01h "jeder SYNC". Das Enum vergibt die 1 aber an EventTimer
(Pdo/PdoMapping.cs:70), also periodisches Senden per Timer; "jeder SYNC" ist
dort die 2. Ein EDS-Byte 01h durchzureichen ergäbe damit stilles
Timer-Senden statt SYNC-Senden - eine Verwechslung, die im Betrieb erst
auffällt, wenn die Zeitbezüge nicht stimmen. Die Zeile verlangt jetzt eine
vollständige Abbildung Rohbyte -> Engine statt einer Zuweisung.
Posten 27 neu. Posten 25 deckte nur die PDO-COB-IDs; 1005h ist ebenso ein Wort
und kein Bezeichner - Tabelle 55: Bit 30 gen. ("CANopen device generates SYNC
message"), Bit 29 frame, Bits 10-0 die CAN-ID. Im Code ist die SYNC-COB-ID
fest verdrahtet (CanOpenCobId.Sync = 0x080 als const, geprüft bei
CanOpenNode.cs:712, benutzt bei :348 und :891). Eine EDS mit abweichender
COB-ID oder gesetztem frame-Bit bliebe wirkungslos. Bit 30 hängt dabei an
Posten 20: es ist derselbe Ein-/Ausschalter des SYNC-Produzenten, den
StartSyncProducer setzt.
Posten 28 neu. HandlePdoMappingEntryWrite prüft die Bitlänge gegen sich selbst
(0 < n <= 64, Vielfaches von 8) und die Summe gegen die 8-Byte-Grenze, nie
gegen OdEntry.Size. 16 Bit aus einem 8-Bit-Eintrag werden also quittiert; der
TPDO-Pfad polstert oder schneidet still, der RPDO-Pfad fängt den Größenfehler
und ignoriert ihn. Ausdrücklich ein anderer Posten als 26: dort ob das Ziel
mappbar ist, hier wie breit.
Geprüft: eng/verify-requirements-traceability.py exit 0; mkdocs build --strict
mit denselben zwei Warnungen wie auf main.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
|
@codex review Runde 2 von 2 — die letzte. Head ist Neu seit Danach wird eingefroren. Was in dieser Runde nicht gefunden wird, wird beim Übersetzen der Liste in Anforderungen gemessen statt reviewt — die Traceability-Ratsche ist dafür der verlässlichere Detektor als ein weiterer Durchgang über eine Liste, die mit jedem Durchgang wächst. Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46cfee6b44
ℹ️ 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".
…t-31 festgenagelt
Letzte Runde der vereinbarten Obergrenze. Zwei Befunde, einer ganz zutreffend,
einer halb.
Posten 5 forderte "eine vollständige Abbildung Rohbyte -> Engine" und ließ
gerade FEh/FFh aus - die Anforderung sagte also vollständig und war es selbst
nicht. Jetzt steht die Abbildung ausgeschrieben da: 00h -> Posten 11 (neu),
01h -> vorhandenes Synchronous, 02h-F0h -> SYNC-Zähler (neu), FCh/FDh ->
Posten 6b/6a (neu), FEh/FFh -> vorhandenes EventDriven. Das letzte Paar ist
das einzige, das auf bestehendes Verhalten trifft, und stand nirgends.
Beim zweiten Befund liegt Codex bei 1005h falsch und bei 1014h richtig.
Falsch: Bit 31 sei dort das Abschalten der SYNC-Konsumption. Tabelle 55 führt
es als "x - do not care", anders als bei den PDO-Records. Das steht jetzt
ausdrücklich in der Zeile, damit es niemand als vergessenes valid-Bit
nachträgt - es ist der zweite Befund dieser PR, der eine Normaussage umkehrt.
Richtig: meine Zeile nannte 1014h im Titel und gab kein Layout an. Tabelle 59:
Bit 31 valid ("EMCY does not exist / is not valid"), Bit 30 reserviert und
immer 0b, Bit 29 frame. Auch dort ist der Bezeichner fest verdrahtet -
SendEmcyAsync sendet auf CanOpenCobId.Emcy(_nodeId) (CanOpenNode.cs:352-358),
eine abweichende EMCY-COB-ID aus der EDS bliebe wirkungslos.
Damit ist die Liste eingefroren. Was jetzt noch fehlt, wird beim Übersetzen in
Anforderungen gemessen statt reviewt.
Geprüft: eng/verify-requirements-traceability.py exit 0; mkdocs build --strict
mit denselben zwei Warnungen wie auf main.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
FERTIG — mergebarHead ist Alle Checks grün — 14 von 14, Alle Threads geschlossen — 35 von 35 aufgelöst, keiner offen. Die Obergrenze ist eingehalten. Runde 1 auf Kein weiterer Was offen bleibt, und wo
Innerhalb dieser PR ist keine Frage mehr offen: Posten 14 ist entschieden, und das Dokument führt nichts mehr als ausstehend. Der Branch ist ab jetzt eingefroren. Kommt ein Befund herein, ist dieses Signal in dem Moment widerrufen, und ich sage das, bevor ich mit der Untersuchung anfange. Generated by Claude Code |
|
To use Codex here, create an environment for this repo. |
2 participants
What does this change?
Zwei Reviewdokumente. Das Scope-Dokument trägt jetzt die CANopen-Entscheidung, für die es geschrieben wurde, und das Gap-Review die Herkunftsregel, die J1939, UDS und ISO-TP brauchen werden. Kein Code, kein Release.
Die Entscheidung (16.09.)
ro— und nur für sie trägt das die Norm: Fußnote*der Objektübersicht erlaubtroausdrücklich für „PDO communication parameter and PDO mapping object entries", für sonst nichts.README.md:47, „a master cannot configure this node purely over the OD"). Die billigere Gegenvariante — beim Laden aufrozwingen — ist damit nicht gewählt.Die Postentabelle hält je Zeile fest, ob die Notwendigkeit aus CiA 301 oder aus der Architektur folgt — damit später niemand eine Normpflicht daraus liest, die dort nicht steht. Mehrere Posten sind ausdrücklich Architektur (Übertragungsart, Inhibit Time, Heartbeat-Records), mehrere ausdrücklich normativ (Abort-Codes,
1800h:04, die SYNC-/EMCY-Records).Mit Punkt 5 ist die letzte offene Frage dieses Dokuments beantwortet. Es führt keine Entscheidung mehr als ausstehend.
Was das Review beigetragen hat
Der Zuschnitt ist im Verlauf dieser PR auf ein Vielfaches seines ersten Entwurfs gewachsen — alle Zuwächse aus Codex-Befunden, jeder am Code oder am Normtext nachgeprüft. Die inhaltlich interessantesten:
1018hlässt sich wahrheitsgemäß füllen. Ich hatte das für unmöglich erklärt.01h= 0 ist „invalid vendor-ID", also ein definierter Wert;02h–04hsind bei 0 dagegen reserved. Also sub0 =01h, Vendor-ID = 0, die übrigen Subs weglassen, nicht nullen.1200histOptional, nicht Pflicht — die Objektdefinition sagt „Index 1200h: Optional"; Pflicht sind erst1201h–127Fhje weiterem SDO-Server.1017hist nur Pflicht, wenn Guarding nicht unterstützt wird — und dieser Knoten beantwortet Guarding-RTRs. Die Klausel greift also nicht; der Record wird aus Architekturgründen gebaut, nicht aus einer Normpflicht.FChundFDhsind zwei Verhalten, nicht eins:FDhsampelt bei Empfang des RTR,FChbei jedem SYNC und puffert. Ein gemeinsamer Handler erfülltFDhund verletztFChstill.EmcyMessage.cs:13nennt Byte 2 „mirror of OD 0x1001", undSendEmcyAsyncfasst das OD nicht an.Zwei Verallgemeinerungen statt weiterer Einzelfälle
Zweimal hat sich ein Befund als drittes Exemplar desselben Musters erwiesen. Beide Male steht jetzt die Regel im Dokument statt des vierten Einzelfalls:
Vier Prüffragen je Posten. Beschreibt die Zeile Verhalten oder nur eine API? Gilt sie für EDS und Fallback? Gilt sie in beide Richtungen? Deckt der Beleg die Zusage? Jede stammt aus mehreren tatsächlichen Fehlern in diesem Dokument, und keine ist CANopen-spezifisch — die drei übrigen Produkte haben dieselbe Übung vor sich.
Posten 23: OD und Laufzeit stimmen in beiden Richtungen und beiden Pfaden überein. Diese Zeile brauchte vier Anläufe: zu weit, zu eng, dann am Kriterium statt am Zugriffsweg (maßgeblich ist die tatsächlich erfolgte Änderung), zuletzt die Korrektur, dass
EntryWrittennur eine halbe Naht ist — es feuert beiWriteRaw/WriteUnsigned, nicht beiAdd*, was das Paket selbst dokumentiert. Genau dieser Verlauf ist der Grund, eine solche Verallgemeinerung vor der SRS-Übersetzung gegenlesen zu lassen und nicht danach.Herkunft bekommt eine dritte Stufe
Für CANopen lag der Normtext vor; für die anderen drei womöglich nicht. Eine Anforderung, die auf einem Poster ruht, darf nicht aussehen wie eine, die auf einer Klausel ruht — daraus kamen die zwölf Befunde auf #127. Dazu die Korrektur, dass die ISOBUS-Datenbasis auf einer anderen Ebene liegt als die offenen J1939-21-Transport-Issues #30–#33.
EdsDcfNet ist eine Abhängigkeit, kein Ticket von uns
Der
1018h-Befund im XDD-Klassifizierer gehört nicht in diesen Zuschnitt;CLAUDE.mdschreibt dieselbe Haltung für den Upstream fest. Für uns bleibt eine Randbedingung: wer aus einer XDD befüllt, darf derenMandatoryObjectsnicht trauen. EDS/DCF ist nicht betroffen.Type of change
feat— new behaviour (minor release)fix/perf— bug or performance fix (patch release)docs/test/refactor/chore/ci— no releaseAusschließlich
docs(reviews):-Commits, zwei geänderte Dateien. Kein Release.Checklist
dotnet build CanKit.Pro.sln -c Releasesucceeds — nicht gelaufen, kein C# geändertdotnet test CanKit.Pro.sln -c Releasepasses — nicht als Gate gelaufen, kein C# geändert. Ein einzelner Test wurde zur Diagnose eines CI-Fehlschlags wiederholt ausgeführt, siehe untenGeprüft auf jedem Head:
eng/verify-requirements-traceability.pyexit 0,mkdocs build --strictmit denselben zwei Warnungen wie aufmain(nav-Zielapifehlt, mermaid-CDN durch den Proxy blockiert).Offener CI-Punkt, nicht von dieser PR — jetzt #130
Das net48-Leg auf Windows war auf
47b57dfrot:DeadlineTests.Rearm_Before_Original_Expiry_Extends_The_Deadline. Der Diff enthält null.cs-Dateien, ubuntu und macOS waren auf demselben Commit grün, der net10.0-Lauf desselben Jobs ebenfalls.Ich nenne es nicht Flake: 20 von 20 lokalen Läufen bestanden, davon 8 unter dreifacher CPU-Überlast. „Ausgelasteter Runner" trägt also nicht. Auf Anweisung des Maintainers aus #92 herausgelöst in ein eigenes Ticket, #130 — samt zweier Hypothesen, die sich beim Nachsehen entkräften ließen (keine bedingte Kompilierung; kein Einheitenfehler in der Zeitrechnung trotz unterschiedlicher
Stopwatch.Frequency).Nächster Schritt, nicht in dieser PR: die Posten als Anforderungen in die SRS schreiben. Dann greift die Ratsche, und „CANopen vollständig" wird eine Zahl statt einer Einschätzung.
🤖 Generated with Claude Code
https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
Generated by Claude Code