Repository navigation
feat(dot): ausklappbarer PWM-Signalweg von OC2REF über PE11 zur LED - #13
Conversation
Umsetzung und Abschlussprüfung zu Issue #5Die ursprünglich als Ideen-Issue erfasste Darstellung ist nach ausdrücklicher Zustimmung als kompakte, native Fachliche Abgrenzung
Testresultat des finalen Heads
Bitte den PR fachlich/didaktisch reviewen. PR und Issue bleiben offen; kein automatischer Merge. |
| <summary> | ||
| <span> | ||
| <strong>Signalweg verstehen</strong> | ||
| <small>Warum leuchtet DOT bei OC2REF = 1?</small> |
There was a problem hiding this comment.
[P1 · fachlich-didaktischer Widerspruch zu AK „TIM2 ohne DOT-Verbindung“] Der Text im standardmäßig geschlossenen Ausklappfeld lautet auch bei ausgewähltem TIM2 unverändert „Warum leuchtet DOT bei OC2REF = 1?“. Gerade für TIM2 ist die Antwort aber: gar nicht, da TIM2_CH2 hier keinen PE11-/DOT-Pfad hat. Der irreführende Satz erscheint, bevor man die korrekte Erklärung im geöffneten Feld lesen kann. Bitte die Kurzbeschreibung vom Timer abhängig machen, z. B. bei TIM1 „Warum leuchtet DOT bei OC2REF=1 (unter den genannten Freigabeannahmen)?“, bei TIM2 „Warum ist TIM2_CH2 nicht mit DOT/PE11 verbunden?“. Einen E2E-Test für beide geschlossenen Zustände ergänzen.
|
|
||
| <div className="dot-path-table-wrap"> | ||
| <table className="dot-path-table"> | ||
| <caption>Signalzustände für TIM1_CH2 mit Ausgangspolarität LOW und aktivierter DOT-Anzeigenstelle</caption> |
There was a problem hiding this comment.
[P2 · TIM2-Kontext der Zustandstabelle] Auch bei TIM2 werden hier die beiden theoretischen TIM1_CH2 → PE11 → DOT-Zeilen mit real wirkenden Pinpegeln LOW/HIGH und DOT AN/AUS dargestellt. Die Tabellenüberschrift nennt TIM1 und keine Zeile wird als aktuell markiert – das ist fachlich nicht falsch, aber direkt unter dem als getrennt gezeigten TIM2-Signalweg leicht misszuverstehen. Bitte den Referenzcharakter bei TIM2 zusätzlich unmittelbar sichtbar machen (z. B. „Nur Vergleichsbeispiel für TIM1, nicht der aktuelle TIM2-Pfad“) oder die Tabelle für TIM2 durch einen „keine PE11-Verbindung“-Zustand ersetzen. Dadurch würde die Trennung interner Logik/physischer Verbindung didaktisch klarer.
| // Die Ausgangspolarität LOW invertiert den logischen OC2REF-Zustand. | ||
| // PE11 und DOT werden nur bei TIM1 unter den dokumentierten Annahmen modelliert. | ||
| const pe11Level = !connected ? "nicht verbunden" : oc2refActive ? "LOW (0)" : "HIGH (1)"; | ||
| const dotState = !connected ? "nicht verbunden" : oc2refActive ? "AN" : "AUS"; |
There was a problem hiding this comment.
[P2 · inkonsistenter Status zum bestehenden LED-Widget] Hier wird bei TIM2 richtigerweise DOT: nicht verbunden ausgegeben. Das daneben weiterhin sichtbare LED-Widget in TimerPwmDemo.tsx meldet per aria-label jedoch „LED momentan ausgeschaltet“. Nicht verbunden und über einen aktiven PE11-Ausgang ausgeschaltet sind verschiedene Zustände; gerade diese Unterscheidung ist Gegenstand von Issue #5. Bitte die Beschriftung/den Accessible Name des übergeordneten virtuellen DOT bei TIM2 ebenfalls auf „nicht verbunden“ bzw. „kein DOT-Pfad über PE11“ ändern. Der neue E2E-Test auf aria-label = LED momentan ausgeschaltet zementiert zurzeit die Inkonsistenz.
| - Bei `TIM1_CH2` mit `OCPolarity LOW` entspricht OC2REF=1 einem **LOW-Pegel an PE11**; eine low-aktive DOT-Leitung lässt das gewählte Segment dann leuchten. | ||
| - Die physikalische Darstellung gilt nur bei **TIM1** und unter Annahme aktivierter Ausgangsfreigaben **CC2E/MOE**, AF1-Konfiguration sowie einer aktivierten Anzeigenstelle über **CNTL1…CNTL4**. Diese Voraussetzungen sind in der Website nicht dynamisch modelliert. **TIM2_CH2 ist nicht mit DOT/PE11 verbunden.** | ||
| - Das Blockschaltbild ist eine **didaktische Funktionsdarstellung, kein elektrischer Stromlaufplan** und behauptet keine zusätzlichen physischen Inverter. Der tatsächliche Treiber-/Multiplex-Strompfad des 8-Segment-Adapterboards ist nicht rekonstruiert. | ||
| - Materialgrundlage: Praktikumsanleitung, Kapitel 7.1.1 (low-aktive Segmente und CNTL-Leitungen) sowie Kapitel 7.6.3 (TIM1_CH2/PE11, AF1, DOT); das Open429Z-D-Basisschaltbild dokumentiert PE11, aber nicht die vollständige Adapterboard-Beschaltung. |
There was a problem hiding this comment.
[P2 · Rückverfolgbarkeit der Hardware-Annahmen] Die MCU-Pinzuordnung PE11 = TIM1_CH2/AF1 lässt sich in der offiziellen STM32F429-AF-Tabelle verifizieren. Die low-aktive DOT-Leitung, CNTL1…CNTL4 und der Adapterboard-Pfad werden hier hingegen nur über eine nicht verlinkte „Praktikumsanleitung“ begründet. Für das Hardware-Akzeptanzkriterium bitte Dokumenttitel, Version/Revision und möglichst eine überprüfbare Fundstelle/URL zur Praktikumsanleitung sowie zum verwendeten Board-Schaltplan nennen. Die ehrliche Einschränkung „kein verifizierter elektrischer Schaltplan“ ist ausdrücklich richtig und soll erhalten bleiben – die fehlende Primärquellenangabe ist ein Nachvollziehbarkeitsmangel, kein nachgewiesener Signalfehler.
| await expect(details.getByTestId("path-pe11")).toContainText("PE11: nicht verbunden"); | ||
| await expect(details.getByTestId("path-dot")).toContainText("DOT: nicht verbunden"); | ||
| await expect(details.getByTestId("path-current-state")).toContainText("keine Verbindung"); | ||
| await expect(page.locator(".led-visual").first().getByRole("img")).toHaveAttribute( |
There was a problem hiding this comment.
[P2 · Tests sichern aktuell einen widersprüchlichen TIM2-Zustand ab] Der TIM2-Test kontrolliert den neuen Pfad auf „nicht verbunden“, verlangt zugleich aber beim übergeordneten LED-Bild den Accessible Name „LED momentan ausgeschaltet“. Nach Harmonisierung beider Komponenten sollte die Erwartung auf „nicht verbunden“ wechseln. Zusätzlich prüfen: geschlossener Summarytext bei TIM2, keine „aktuell“-Zeile in der TIM1-Vergleichstabelle bei TIM2 und TIM2 auch mit OC2REF=0. Die bisherigen 36 bestandenen Tests decken das positive TIM1-Signal und die wesentlichen PWM-Grenzen bereits gut ab.
| </div> | ||
| </div> | ||
|
|
||
| <div className="dot-path-current" data-testid="path-current-state"> |
There was a problem hiding this comment.
[P3 · Accessibility/UX-Vorschlag] Die aktuelle Signalkette wird dynamisch geändert, ist aber nicht als zusammenhängender Status für assistive Technologien gekennzeichnet. Bei per Tastatur ausgelösten CNT-Schritten/CCR2-Änderungen müssen Screenreader-Nutzende unter Umständen erneut zum Block navigieren, um den neuen PE11-/DOT-Zustand zu erfahren. Eine sparsame Zustandsankündigung für manuelle Aktionen wäre hilfreich; bitte nicht die gesamte Tabelle als Live-Region verwenden und bei laufender Animation keine Ansageflut erzeugen. Kein fachlicher Merge-Blocker.
Unabhängiges Code-, Fach-, UX- und Didaktik-Review – PR #13 / Issue #5Empfehlung: REQUEST CHANGES (nur als PR-Konversationskommentar; kein formelles Approve/Request-Changes). Die neue Funktionsdarstellung ist insgesamt fachlich schlüssig. Es gibt jedoch einen gut reproduzierbaren Widerspruch bei TIM2, der das zentrale Lernziel von Issue #5 betrifft. 6 konkrete Befunde sind bereits inline kommentiert. Prüfbasis: PR-Head Akzeptanzkriterien aus Issue #5
Vor dem Merge beheben[P1] TIM2-korrekte Aussage bereits im geschlossenen Ausklappfeld. Der dauerhaft angezeigte Text „Warum leuchtet DOT bei OC2REF = 1?“ ist bei ausgewähltem TIM2 sachlich nicht zutreffend, weil dort kein PE11-/DOT-Signalpfad besteht. Bitte die Unterzeile von Weitere gezielte Hinweise
GesamturteilStärken: Die beiden logischen Umkehrungen sind klar erklärt, TIM1-Pegel und DOT-Zustand reagieren korrekt auf die modellierten PWM-Ereignisse, Freigabe- und Adapterannahmen werden offengelegt, das neue Blockbild ergänzt das bestehende Zeitdiagramm ohne Eingriff in das Hardwaremodell. Die neuen Screenshots sind stimmig. Es gibt keinen nachgewiesenen Fehler in der eigentlichen Timer-/PWM-Berechnung. Request Changes empfohlen, primär wegen der widersprüchlichen TIM2-Kernaussage im geschlossenen Einstieg und der Statusinkonsistenz zum LED-Widget. Nach diesen überschaubaren Text-/Statuskorrekturen halte ich den PR fachlich-didaktisch für mergefähig; die restlichen Punkte können im Rahmen dieses PRs oder als Folgeaufgaben behandelt werden. Keine Codeänderungen, kein Merge, keine Änderung des PR-Status durch mich. |
Unabhängiges Review zu PR #13: gezielte Auswertung und KorrekturDanke für die sechs Inline-Findings. Die fachlich relevanten TIM2-Widersprüche wurden korrigiert, ohne die gemeinsame Timer-/PWM-Berechnung oder den Scope des Issues zu ändern. Übernommen
Bewusst nicht umgesetzt[P3] Permanente Screenreader-Live-Ansagen bei jedem CNT-/CCR-Schritt: Kann bei der laufenden Animation unnötige Ansagefluten erzeugen und würde zusätzliche Unterscheidung von manuell/automatisch ausgelösten Zustandsänderungen erfordern. Die vorhandenen Statuswerte und semantischen Labels bleiben regulär auslesbar. Eine gezielte, debouncte Ankündigung nach manuellen Aktionen könnte bei Bedarf in einem eigenen Accessibility-Schritt ergänzt werden; kein fachlicher Merge-Blocker. Abschlussprüfung
PR #13 bleibt offen, Issue #5 bleibt offen; kein Merge ohne explizite Nutzerfreigabe. |
Bezug
Closes #5 (bei Merge; erst nach Review). Dieses bereits vorhandene Issue dokumentiert die didaktisch wichtige Trennung von internem Referenzsignal, Ausgangspegel und low-aktiver DOT-Anzeige.
Lösung
Im Abschnitt 05 / HARDWAREBEZUG · Virtuelles DOT · PE11 gibt es ein standardmäßig eingeklapptes natives
<details>-Feld, das per Tastatur und Touch bedient wird.pwmActive-Zustand wird aus dem Parent übergeben; es entsteht keine zweite Timer-/PWM-Engine. BeiCNT=CCR2, periodischem Rücksprung, 0 % und 100 % bleibt die Darstellung konsistent.Hardwaregrundlage / Abgrenzung
Die beigefügte Praktikumsanleitung beschreibt in Kap. 7.1.1 die low-aktiven Segmentleitungen und CNTL-Anzeigenstellen sowie in Kap. 7.6.3 TIM1_CH2 → GPIO_AF1_TIM1 → PE11 (DOT). Der Open429Z-D-Basisschaltplan allein zeigt nicht den vollständigen Strompfad im 8-Segment-Adapterboard. Deshalb zeigt dieser PR bewusst keine behauptete reale LED-/Treiber-Verdrahtung.
Review-Checkliste
Bitte Findings inline am PR kommentieren. Kein Merge ohne ausdrückliche Freigabe.