Conversation
…ng the title information.
…o support the 2026 REBUILT game, the old Reef structure removed, and automatic tower and fuel counting functions added.
- Refactor score_test.go to include new scoring logic and tests for Auto, Teleop, and Endgame points. - Introduce new scoring thresholds and ranking points in event_settings.go for Energized and Traversal bonuses. - Modify reports.go to reflect changes in scoring metrics, removing outdated Barge points. - Update scoring_panel.go to streamline scoring interface, removing Barge and Processor flags, and implementing new climb and fuel adjustments. - Ensure all changes are aligned with the 2026 game rules and scoring structure.
- Updated match review tests to reflect new scoring formats and removed obsolete match checks. - Modified scoring panel to remove deprecated fields and adjust to new scoring logic for 2026. - Enhanced websocket handling for scoring commands, including new commands for fuel and climbing. - Adjusted event settings to accommodate new thresholds for energized and supercharged fuel. - Improved test coverage for scoring panel websocket interactions and match state changes.
- Modified scoring_panel.html to enhance the real-time scoring interface, including new UI elements for autonomous and teleop periods, and improved button feedback. - Updated setup_settings.html to reflect new game-specific thresholds for energized and supercharged fuel, and removed obsolete fields. - Refactored referee_panel.go to streamline foul handling and card assignment logic, ensuring proper integration with the updated scoring system. - Adjusted websocket communication to support new game rules and real-time updates for match state and scoring.
…frontend requirements.
…ror logging, unpack score command data, and add debug logs for state changes.
…c message fields, and corrected the scoring check in automatic and manual phases.
…ecific messages, and added automatic stage victory determination logic.
This reverts commit 329645d.
… AJAX calls, enhanced WebSocket command handling, and improved error logging.
…dler, remove old foul_list routes, and ensure correct rendering of HTML fragments.
…sy-arena into my-test-branch
…case of a tie during the match and updates the relevant logic to reflect this change.
…gic for auto fuel wins, enhance hub light controls, and streamline stringer-generated code for registers and coils.
…d last fuel values and calculate increments during the match; updated testing to check fuel registration and Hub lights.
…d electronically controlled phases to improve the game flow.
…ure the end status option is displayed correctly.
…sy-arena into my-test-branch
… when the Hub is inactive and that the last value is updated correctly.
…and 2 outputs, and background thread management has been improved.
…tant definitions.
…dexes to reflect the new structure.
…vant logic to reflect the new structure.
…vant logic to reflect the new structure.
Refactor switch configuration methods for Fortinet support, including login and command execution adjustments.
…pdating command formats and test cases.
…atus judgment, and simplify the command processing logic of the Fortinet switch.
…g constant definition to reflect the correct address.
…ptions to improve readability.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces 2025 coral/barge scoring with a 2026 fuel/tower/hub model, adds Jetson GPIO Modbus PLC support and Arduino Mudbus sketch, migrates switch logic to Fortinet, and overhauls web UI/JS/CSS, templates, PLC API, many tests, and ranking/tiebreaker logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Arduino as Arduino (Mudbus Client)
participant Jetson as Jetson (Modbus TCP Server)
participant Background as Background Thread
participant GPIO as Jetson GPIO
Arduino->>Jetson: TCP connect / Modbus transactions
Jetson->>Background: provide/register Modbus data store
Background->>GPIO: poll input pins (BOARD 29,31,32,33,35,37)
GPIO-->>Background: return active-low states
Background->>Jetson: write discrete inputs 0..5 (1 if active)
Arduino->>Jetson: write/read coils (control outputs)
Background->>Jetson: read coil states
Background->>GPIO: drive outputs based on coils
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
templates/match_play.html (1)
158-166:⚠️ Potential issue | 🟡 MinorDuplicate HTML
id="fieldResetRadio"on two different elements.Line 159 and line 165 both use
id="fieldResetRadio". HTML IDs must be unique per page; the Signal Count radio button should have its own distinct ID (e.g.,signalCountRadio).Proposed fix
<label> <input type="radio" name="allianceStationDisplay" value="signalCount" - onclick="setAllianceStationDisplay();" id="fieldResetRadio"> Signal Count + onclick="setAllianceStationDisplay();" id="signalCountRadio"> Signal Count </label>web/reports.go (1)
61-108:⚠️ Potential issue | 🔴 CriticalPDF rankings report is broken: missing
"Tower"key incolWidthsmap and header/data column mismatch.Several issues in the rankings PDF report:
- Line 100 uses
colWidths["Tower"]but the map (lines 61–72) only has"Coop"and"Barge"— no"Tower"key. In Go, this returns0.0, resulting in a zero-width column.- The
"Barge"header is still rendered (line 88) but its data row was removed (line 104), creating a column count mismatch between header (10 cols) and data rows (9 cols).- The
"Coop"header (line 85) now displaysTowerPointsdata — the naming is inconsistent.The
colWidthsmap, headers, and data cells all need to be updated consistently for the Barge→Tower rename.Proposed fix
colWidths := map[string]float64{ "Rank": 13, "Team": 20, "RP": 20, - "Coop": 20, + "Tower": 20, "Match": 20, "Auto": 20, - "Barge": 20, "W-L-T": 22, "DQ": 20, "Played": 20, }And update headers accordingly:
pdf.CellFormat(colWidths["RP"], rowHeight, "RP", "1", 0, "C", true, 0, "") - pdf.CellFormat(colWidths["Coop"], rowHeight, "Coop", "1", 0, "C", true, 0, "") + pdf.CellFormat(colWidths["Tower"], rowHeight, "Tower", "1", 0, "C", true, 0, "") pdf.CellFormat(colWidths["Match"], rowHeight, "Match", "1", 0, "C", true, 0, "") pdf.CellFormat(colWidths["Auto"], rowHeight, "Auto", "1", 0, "C", true, 0, "") - pdf.CellFormat(colWidths["Barge"], rowHeight, "Barge", "1", 0, "C", true, 0, "")model/event_settings.go (1)
122-159:⚠️ Potential issue | 🟠 MajorExisting databases will get zero-valued thresholds instead of game defaults.
When
GetEventSettingsfinds an existing record (Line 127-129), it returns it as-is. For databases created before these new fields were added,EnergizedFuelThreshold,SuperchargedFuelThreshold, andTraversalPointThresholdwill deserialize as0(Go's zero value) instead of the intended defaults (100, 360, 50). This could silently break scoring logic that depends on these thresholds.Consider adding a migration or post-load fixup, e.g.:
🛠️ Proposed fix: apply defaults for zero-valued new fields
if len(allEventSettings) == 1 { - return &allEventSettings[0], nil + settings := &allEventSettings[0] + // Backfill 2026 defaults for databases created before these fields existed. + if settings.EnergizedFuelThreshold == 0 { + settings.EnergizedFuelThreshold = game.EnergizedFuelThreshold + } + if settings.SuperchargedFuelThreshold == 0 { + settings.SuperchargedFuelThreshold = game.SuperchargedFuelThreshold + } + if settings.TraversalPointThreshold == 0 { + settings.TraversalPointThreshold = game.TraversalPointThreshold + } + return settings, nil }Note: this approach assumes
0is never a valid user-configured value for these thresholds. If0is a legitimate value, a version/migration field would be needed instead.web/referee_panel.go (1)
138-176:⚠️ Potential issue | 🟡 MinorMissing
FoulsCommittedflag update for foul edit operations.The
addFoulcommand (Line 134-135) setsFoulsCommitted = truefor both alliances, but the foul edit commands (deleteFoul,toggleFoulType,updateFoulTeam,updateFoulRule) only callRealtimeScoreNotifier.Notify()without updatingFoulsCommitted. If this flag is used to track whether fouls have been modified since last commit, the edit operations should also set it.templates/setup_settings.html (1)
248-251:⚠️ Potential issue | 🟡 MinorStale description still references Cisco switch.
Line 249 says "Vivid-Hosting VH-113 access point and Cisco Catalyst 3500-series switch" but the codebase has been migrated to Fortinet switch support. Update this text to reflect the correct hardware.
plc/plc.go (1)
349-366:⚠️ Potential issue | 🔴 Critical
readInputspanics due to oversizedinputCount(pipeline failure).This is the downstream effect of the input constant misconfiguration above.
ReadDiscreteInputs(0, 41)requests 41 inputs. If the Modbus device (or fake test client) returns fewer bytes thanceil(41/8) = 6, the check at line 359 fails. Even if it returns enough bytes,byteToBool(inputs, 41)and the subsequentcopyintoplc.inputs[:](a 41-element array) will attempt to map hardware addresses that may not exist, causing the reported runtime panic.Fixing the input constants (above) to produce a contiguous, correctly-sized
inputCountwill resolve this.plc/plc_test.go (1)
61-88:⚠️ Potential issue | 🔴 CriticalThese name expectations confirm the input constants in
plc.gomust be contiguous.The test expects exactly 19 input names (
fieldEStopthroughblueConnected3), indexed 0–18. This is consistent with a contiguousiotablock starting at 0. The currentplc.goproducesinputCount=41with gaps, so this test (andTestPlcInputs) will fail until the input constants are fixed as noted in theplc.goreview.static/js/audience_display.js (1)
293-307:⚠️ Potential issue | 🟠 MajorRed-side
data-checkedattributes use stale field namesCoralBonusRankingPointandBargeBonusRankingPoint.Lines 300 and 306 reference
data.RedScoreSummary.CoralBonusRankingPointanddata.RedScoreSummary.BargeBonusRankingPoint, which no longer exist. While thehtml()calls on lines 297 and 303 correctly useSuperchargedRankingPointandTraversalRankingPoint, thedata-checkedattributes will always beundefined. If CSS or JS relies on these attributes for styling, this is a visual bug.Proposed fix
$(`#${redSide}FinalCoralBonusRankingPoint`).attr( - "data-checked", data.RedScoreSummary.CoralBonusRankingPoint + "data-checked", data.RedScoreSummary.SuperchargedRankingPoint ); ... $(`#${redSide}FinalBargeBonusRankingPoint`).attr( - "data-checked", data.RedScoreSummary.BargeBonusRankingPoint + "data-checked", data.RedScoreSummary.TraversalRankingPoint );
🤖 Fix all issues with AI agents
In `@arduino/driverStation.py`:
- Around line 1-2: The file driverStation.py contains an unintended heredoc
wrapper that references /home/jetson/modbus_plc.py and a dangling EOF (e.g., the
top line "#cat << 'EOF' > /home/jetson/modbus_plc.py" and the trailing "EOF" at
line 77); remove both the opening heredoc line and the trailing EOF so the
Python source is not wrapped in shell heredoc, or if this was meant for
deployment, move the heredoc into a separate shell script and update the target
path to the correct file for the driver station instead of modbus_plc.py.
In `@arduino/modbus_plc.py`:
- Around line 1-2: Remove the leftover shell heredoc wrapper from the top and
bottom of modbus_plc.py: delete the initial line "#cat << 'EOF' >
/home/jetson/modbus_plc.py" so the shebang "#!/usr/bin/env python3" is the very
first line, and delete the trailing "EOF" line near the end that is an undefined
name; this will make the module valid Python and eliminate the Ruff EXE005/F821
issues in the module (modbus_plc.py).
In `@field/arena_test.go`:
- Around line 48-67: createTestArena constructs an Arena without initializing
its notifiers, causing a nil dereference when handlePlcInputOutput calls
arena.RealtimeScoreNotifier.Notify; fix by initializing the ArenaNotifiers the
same way configureNotifiers does (or call configureNotifiers on the test Arena)
so RealtimeScoreNotifier and any other notifiers are non-nil before tests run —
update createTestArena to call configureNotifiers(arena) or to populate
ArenaNotifiers/RealtimeScoreNotifier explicitly.
In `@field/arena.go`:
- Around line 1287-1327: The code has a data race on the Fouls slice because
handlers append to arena.RedRealtimeScore.CurrentScore.Fouls concurrently with
the arena loop reading/copying scores in handlePlcInputOutput; add a mutex on
the score state (e.g., add sync.RWMutex ScoreMu to the Arena struct) and use it
to guard all accesses and mutations of RedRealtimeScore.CurrentScore and
BlueRealtimeScore.CurrentScore: acquire ScoreMu.RLock() when reading/copying
scores (including where oldRedScore := *redScore and before calling Equals) and
ScoreMu.Lock() when mutating (all increments in handlePlcInputOutput and the
append in the websocket scoring_panel handler), then release appropriately;
ensure the equality check oldRedScore.Equals(redScore) and any Notify() that
relies on consistent score snapshots happen while holding a read lock or use a
locked snapshot to avoid races.
In `@network/switch_test.go`:
- Around line 68-98: The mockTelnet helper currently never writes any prompts,
so runCommand in network/switch.go blocks waiting for login/password prompts;
update mockTelnet to emulate the Fortinet telnet handshake by writing the
expected login banner and password prompt to each accepted connection (e.g.,
send "login:" and then "Password:" or whatever runCommand expects), then read
credentials sent by the client and respond with the command prompt before
allowing the test reads to capture the sent commands; keep the existing
Accept/read structure (conn1/conn2) and use conn.Write with appropriate
timing/deadlines so runCommand advances instead of timing out.
In `@partner/tba.go`:
- Around line 398-412: The code currently divides by ranking.Played when
populating TbaRanking (fields RP, Match, Auto, Tower), causing a
division-by-zero panic if Played == 0; fix by guarding those calculations:
before assigning to tbaRankings[i], check if ranking.Played == 0 and set RP,
Match, Auto, Tower to 0.0 (or other safe default), otherwise compute them using
float32(ranking.Played); update the TbaRanking construction in the loop that
fills tbaRankings to use this guard for RP, Match, Auto, and Tower.
In `@plc/plc.go`:
- Around line 71-101: The constants for input indices (e.g., red1EStop,
red1AStop, redConnected1, blue1EStop, inputCount) are non-contiguous due to
resetting iota and inline arithmetic, causing plc.inputs size misalignment and
ReadDiscreteInputs/ GetInputNames failures; fix by creating a single contiguous
index scheme for the plc input array (either put all inputs in one const block
using iota in order or assign explicit sequential integer indices) and if you
must preserve Modbus hardware addresses keep them in a separate mapping table
(e.g., hardwareAddressByInput) so plc.inputs uses contiguous indices and
inputCount reflects the true array length used by ReadDiscreteInputs and
GetInputNames.
In `@static/js/audience_display.js`:
- Around line 203-219: The playoff branch currently reads deprecated fields
(data.Red.ScoreSummary.NumCoral / NumAlgae) causing undefined UI; change the
playoff path that checks currentMatch.Type === matchTypePlayoff to use the 2026
fields (e.g., use data.Red.ScoreSummary.TotalFuelPoints and
data.Blue.ScoreSummary.TotalFuelPoints for redCoral/blueCoral) and remove or
replace uses of data.*.ScoreSummary.NumAlgae when setting $(`#${redSide}Algae`)
and $(`#${blueSide}Algae`) (either map to the new algae field name in
ScoreSummary or omit the algae element update if the field no longer exists),
keeping the element IDs redSideCoral, blueSideCoral, redSideAlgae, blueSideAlgae
unchanged.
In `@templates/audience_display.html`:
- Around line 24-27: The img tags for the audience display (classes "Dive" and
"Reescape" in templates/audience_display.html) use Windows backslashes in their
src attributes; update the src values for the <img class="Dive" src=...> and
<img class="Reescape" src=...> to use forward slashes (e.g., /static/img/...) so
the browser can resolve the image URLs correctly.
In `@templates/match_review.html`:
- Around line 71-88: Remove the unconditional JSON.parse call that assigns
matchResult (the JSON.parse({{.MatchResultJson}}) at the top) and instead assign
rawJson = {{.MatchResultJson}} only, then rely on the existing guarded parsing
block that checks rawJson, uses try/catch and sets the default matchResult;
update references to matchResult so the guarded branch initializes it (symbols:
matchResult, rawJson, JSON.parse, the try/catch block and the $(document).ready
handler).
🟠 Major comments (13)
arduino/red_field_hub.ino-47-56 (1)
47-56:⚠️ Potential issue | 🟠 Major
Mb.C[16]contradicts the documented coil layout — likely should beMb.C[4].The comment on line 49 says "Modbus address 00005" which corresponds to
Mb.C[4](zero-indexed). Lines 59-64 describeMb.C[5]–Mb.C[15]as reserved virtual coils, totaling 16 coils. Reading fromMb.C[16]is out of the described range and inconsistent with the comments. If the PLC writes to coil address 5 (index 4), this code would never see it.🐛 Proposed fix
- bool plc_command = Mb.C[16]; + bool plc_command = Mb.C[4];web/match_play_test.go-146-167 (1)
146-167:⚠️ Potential issue | 🟠 Major
TestCommitCardsis incomplete — reset and escalation logic tests are missing.The function ends at line 167 with only a placeholder comment on line 166 ("Reset logic tests..."). The current test only verifies that yellow cards are assigned to teams; it does not test card escalation (yellow→red), reset behavior on replays, or red card handling. If these mechanics are still valid in 2026, add test cases for:
- Yellow card escalation to red
- Card reset/clearing on match replay
- Red card behavior and constraints
web/setup_settings.go-142-144 (1)
142-144: 🛠️ Refactor suggestion | 🟠 MajorInconsistent form field naming: snake_case vs. camelCase.
The new fields use snake_case (
energized_fuel_threshold,supercharged_fuel_threshold,traversal_point_threshold), while every other field in this handler uses camelCase (warmupDurationSec,autoDurationSec, etc.). This inconsistency makes the API surface harder to maintain and could lead to bugs if the template inadvertently uses the wrong convention.♻️ Suggested: align with existing camelCase convention
- eventSettings.EnergizedFuelThreshold, _ = strconv.Atoi(r.PostFormValue("energized_fuel_threshold")) - eventSettings.SuperchargedFuelThreshold, _ = strconv.Atoi(r.PostFormValue("supercharged_fuel_threshold")) - eventSettings.TraversalPointThreshold, _ = strconv.Atoi(r.PostFormValue("traversal_point_threshold")) + eventSettings.EnergizedFuelThreshold, _ = strconv.Atoi(r.PostFormValue("energizedFuelThreshold")) + eventSettings.SuperchargedFuelThreshold, _ = strconv.Atoi(r.PostFormValue("superchargedFuelThreshold")) + eventSettings.TraversalPointThreshold, _ = strconv.Atoi(r.PostFormValue("traversalPointThreshold"))Remember to update the corresponding
nameattributes intemplates/setup_settings.htmlto match.network/switch.go-150-167 (1)
150-167:⚠️ Potential issue | 🟠 MajorTelnet negotiation handler is fragile with partial reads.
c.Read(buf)can return fewer than 3 bytes per call (TCP doesn't guarantee message framing). If a 3-byte IAC sequence arrives as two separate reads (e.g., 2 + 1 bytes), the checkn < 3on Line 157 causes an early return, leaving negotiation incomplete. This can cause the switch to stall or refuse to enter interactive mode.Consider buffering reads to ensure you always process complete 3-byte IAC sequences:
🐛 Proposed fix
handleNegotiation := func(c net.Conn) { - buf := make([]byte, 3) for { c.SetReadDeadline(time.Now().Add(500 * time.Millisecond)) - n, err := c.Read(buf) - if err != nil || n < 3 || buf[0] != 255 { + buf := make([]byte, 1) + _, err := c.Read(buf) + if err != nil || buf[0] != 255 { return } - if buf[1] == 253 { - c.Write([]byte{255, 252, buf[2]}) - } else if buf[1] == 251 { - c.Write([]byte{255, 254, buf[2]}) + cmdBuf := make([]byte, 2) + if _, err := io.ReadFull(c, cmdBuf); err != nil { + return + } + if cmdBuf[0] == 253 { // DO -> WON'T + c.Write([]byte{255, 252, cmdBuf[1]}) + } else if cmdBuf[0] == 251 { // WILL -> DON'T + c.Write([]byte{255, 254, cmdBuf[1]}) } } }network/switch.go-176-197 (1)
176-197:⚠️ Potential issue | 🟠 MajorWrite errors silently ignored throughout the command execution flow.
writer.WriteStringandwriter.Flushreturn errors that are discarded. If the connection drops mid-command (e.g., switch reboots, network interruption), the function will continue sending subsequent commands into a broken pipe and ultimately return whatever partial output was read — without surfacing the failure.At minimum, propagate errors from
Flush:🛡️ Proposed fix
-send := func(s string, delay time.Duration) { - writer.WriteString(s + "\r") - writer.Flush() +send := func(s string, delay time.Duration) error { + if _, err := writer.WriteString(s + "\r"); err != nil { + return err + } + if err := writer.Flush(); err != nil { + return err + } time.Sleep(delay) + return nil }field/arena_notifiers.go-218-236 (1)
218-236:⚠️ Potential issue | 🟠 MajorHardcoded shift timing boundaries are fragile and have a boundary gap at
time == 0.The values 33, 58, 83, 108, 133, 163 are magic numbers that should be derived from game timing configuration (e.g.,
game.MatchTimingorEventSettingsduration fields) rather than hardcoded. This will break if phase durations are adjusted.Also note: when
time == 0, the first condition (time > 0) is false, so it falls through toelse if time < 58→currentShift = 2. Iftime == 0represents the start of teleop, this incorrectly skips shift 1.game/score_test.go-92-116 (1)
92-116:⚠️ Potential issue | 🟠 Major
TestG420PenaltyBonuscan never fail — the conditional assertion is a no-op.The
if summary.EndgameTowerPoints == 30guard on line 112 means: if the feature works, assert something tautologically true (30 == 30); if it doesn't work, silently pass. This test provides zero value and false confidence.Either assert the expected behavior unconditionally or remove the test until the rule lookup infrastructure is in place.
Proposed fix (assert unconditionally)
- // If your score.go logic includes foul.Rule() check, - // a more complete Mock might be needed. But if it's just checking RuleNumber: - if summary.EndgameTowerPoints == 30 { - // Successfully received 30 points compensation - assert.Equal(t, 30, summary.EndgameTowerPoints) - } + // Assert that the G420 penalty awards 30 endgame tower points. + // If the rule lookup infrastructure isn't ready, this test should + // be skipped with t.Skip(), not silently passed. + assert.Equal(t, 30, summary.EndgameTowerPoints)field/arena_notifiers.go-216-216 (1)
216-216:⚠️ Potential issue | 🟠 MajorReplace magic number
5with named constantTeleopPeriod.Line 216 uses
arena.MatchState == 5, but the MatchState type has named constants defined infield/arena.go. The value5representsTeleopPeriod. Use the named constant instead:if arena.MatchState == TeleopPeriod {This improves code clarity and prevents silent breakage if the constant values are ever reordered.
web/match_review.go-87-94 (1)
87-94:⚠️ Potential issue | 🟠 MajorUse
model.NewMatchResult()instead of a bare struct literal to properly initialize Score fields.The placeholder
&model.MatchResult{MatchId: match.Id}leavesRedScoreandBlueScoreas nil*game.Scorepointers.NewMatchResult()(in model/match_result.go) properly initializes these as non-nil pointers to emptygame.Scoreobjects. The same function is already used ingetMatchResultFromRequest(line 197–202) for the identical scenario, so this guard should be consistent.Note: This nil guard may be unreachable since
getMatchResultFromRequestalready handles nil results from the database, but if present, it should be done correctly.Proposed fix
if matchResult == nil { log.Printf("Warning: MatchResult is nil for match %d", match.Id) - matchResult = &model.MatchResult{ - MatchId: match.Id, - // 這裡可以根據 2026 年的結構初始化空的紅藍方分數 - } + matchResult = model.NewMatchResult() + matchResult.MatchId = match.Id + matchResult.MatchType = match.Type }templates/scoring_panel.html-148-161 (1)
148-161:⚠️ Potential issue | 🟠 MajorClimb "Level 1" option (value=1) maps to
EndgameLevel2in the backend enum.The
EndgameStatusenum ingame/score.godefines:EndgameNone=0,EndgameLevel2=1,EndgameLevel3=2— there is noEndgameLevel1. The radio button for "Level 1" sendsupdateClimb(0, 1), which the backend would store asEndgameLevel2(value 1). This is a semantic mismatch between what the UI displays and what the backend records.Either:
- Add
EndgameLevel1to the enum (beforeEndgameLevel2), shifting the other values, OR- Remove the Level 1 option from the UI if it's not a valid game state, OR
- Have the handler map UI level numbers to the correct enum values.
This is the same concern flagged in the
game/test_helpers.goandweb/scoring_panel_test.goreviews.field/arena_test.go-1-16 (1)
1-16:⚠️ Potential issue | 🟠 MajorSignificant test coverage regression: all prior arena tests appear to have been removed.
Per the AI summary, this file replaces extensive prior tests covering team assignments, match flow, state enforcement, Nexus loading, timeouts, substitutions, E-stops/A-stops, and PLC interactions. The new tests only cover 2026-specific logic. Missing this test coverage is a risk—regressions in the core arena lifecycle won't be caught.
Consider restoring the previous test suite and adding the new 2026 tests alongside it, or add a follow-up issue to restore coverage.
Would you like me to open an issue to track restoring the removed arena test coverage?
game/score.go-47-52 (1)
47-52:⚠️ Potential issue | 🟠 MajorComment says "10 pts per Robot" but code awards 15 points per
AutoTowerLevel1.Line 47 states
// Tower Level 1: 10 pts per Robotbut line 50 adds15. Either the comment or the point value is wrong.If 15 is correct, fix the comment
- // Tower Level 1: 10 pts per Robot + // Tower Level 1: 15 pts per Robot (Auto bonus) for _, reachedL1 := range score.AutoTowerLevel1 { if reachedL1 { summary.AutoTowerPoints += 15field/arena.go-1283-1286 (1)
1283-1286:⚠️ Potential issue | 🟠 MajorDuplicate calls:
updateHubStatus()andupdateGameSpecificMessage()are invoked twice per loop iteration.These are already called in
Update()at lines 774-776, which is the caller ofhandlePlcInputOutput()at line 799. This means every arena loop tick executes these functions twice when PLC is enabled. Remove the duplicate calls here.Proposed fix
- // 2026 REBUILT: Update Hub Status and Game Data - arena.updateHubStatus() - arena.updateGameSpecificMessage() - // 2026 REBUILT: Read PLC Fuel Counts and update Score redPlcFuel, bluePlcFuel := arena.Plc.GetFuelCounts()
🟡 Minor comments (16)
static/css/referee_panel.css-209-237 (1)
209-237:⚠️ Potential issue | 🟡 MinorFix Stylelint errors and translate comments to English.
- Static analysis flags several issues on line 236: use modern
rgb()with percentage alpha instead of legacyrgba()with decimal alpha.- Line 209 needs an empty line before the comment (per
comment-empty-line-beforerule).- All comments in this block are in Chinese, but the rest of the codebase uses English. Consider translating for consistency — the PR objectives also mention translating to English.
Proposed fix
+ -/* 通用 Endgame 文字樣式 */ +/* Generic Endgame text styles */ [class*="-endgame"] { font-weight: normal; - transition: color 0.3s ease; /* 變色時增加平滑過渡 */ + transition: color 0.3s ease; } -/* 狀態 0: None - 灰色 */ +/* Status 0: None - gray */ [class*="-endgame"][data-status="0"] { color: `#6c757d`; } -/* 狀態 1: Level 1 - 淺藍色 */ +/* Status 1: Level 1 - light blue */ [class*="-endgame"][data-status="1"] { color: `#17a2b8`; font-weight: bold; } -/* 狀態 2: Level 2 - 深藍色 (或自定義顏色) */ +/* Status 2: Level 2 - blue */ [class*="-endgame"][data-status="2"] { color: `#007bff`; font-weight: bold; } -/* 狀態 3: Level 3 - 金黃色 (最醒目) */ +/* Status 3: Level 3 - gold (most prominent) */ [class*="-endgame"][data-status="3"] { color: `#ffc107`; font-weight: bold; - text-shadow: 0 0 5px rgba(255, 193, 7, 0.3); + text-shadow: 0 0 5px rgb(255 193 7 / 30%); }game/foul.go-25-28 (1)
25-28:⚠️ Potential issue | 🟡 MinorUpdate comment to remove stale year reference.
Line 26 comment mentions "Special case in 2025 for G206". Since G206 remains active in 2026 (confirmed by rule definition in game/rule.go and active handling in game/score.go), remove the year reference. Update to: "Special case for G206, which is not actually a foul but does make the alliance ineligible for some bonus RPs."
arduino/modbus-plc.service-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorIncomplete heredoc instruction in the comment.
Line 1 contains a shell heredoc command as a comment but the corresponding
EOFterminator is missing at the end of the file, which could confuse anyone trying to use this as an install instruction.Proposed fix — add EOF and closing quote
Add after line 20:
EOF 'arduino/red_field_hub.ino-6-6 (1)
6-6:⚠️ Potential issue | 🟡 MinorHardcoded example MAC address
DE:AD:BE:EF:FE:ED.This is a commonly used placeholder MAC. If multiple Arduino devices are on the same network, they'll conflict. Ensure each device has a unique MAC.
game/rule.go-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorDo not change the original copyright year.
The original copyright is
2014 Team 254. Changing it to2026misrepresents when the file was originally authored. The modification note on line 3 (Modified for 2026 REBUILT Game Manual) is the correct way to indicate the update.Proposed fix
-// Copyright 2026 Team 254. All Rights Reserved. +// Copyright 2014 Team 254. All Rights Reserved.arduino/modbus_plc.py-65-68 (1)
65-68:⚠️ Potential issue | 🟡 MinorComment says "print once per second" but the loop sleeps 0.1s (prints 10x/sec).
Line 66 comment:
每 1 秒印一次就好，比較不會洗版("print once per second to avoid flooding"). Buttime.sleep(0.1)on Line 68 means this prints every 100ms. Either increase the sleep to 1.0s or use a counter/timer to throttle log output.model/event_settings_test.go-38-38 (1)
38-38:⚠️ Potential issue | 🟡 MinorIgnored error from
GetEventSettings.Line 38 discards the error:
settings, _ := db.GetEventSettings(). Same on Line 49. If either call fails, subsequent assertions will pass on nil/zero values, masking real failures. Useassert.Nil(t, err)consistently as done on Line 24.🛠️ Proposed fix
- settings, _ := db.GetEventSettings() + settings, err := db.GetEventSettings() + assert.Nil(t, err)- newSettings, _ := db.GetEventSettings() + newSettings, err := db.GetEventSettings() + assert.Nil(t, err)model/event_settings_test.go-15-30 (1)
15-30:⚠️ Potential issue | 🟡 MinorMissing
db.Close()— database handle leaked.Neither
TestEventSettingsDefaultsnorTestUpdateEventSettingscloses the database. This leaks file handles and may preventos.Removefrom succeeding on some platforms (e.g., Windows). Adddefer db.Close()after opening.🛠️ Proposed fix
db, err := OpenDatabase("test_settings.db") assert.Nil(t, err) defer os.Remove("test_settings.db") + defer db.Close()Apply the same pattern in
TestUpdateEventSettings.field/team_sign_test.go-16-18 (1)
16-18:⚠️ Potential issue | 🟡 MinorRemove redundant
os.Remove("test.db")calls on lines 18 and 44.
SetupTestArena(t)creates the database in a temporary directory viat.TempDir(), not in the current working directory. The hardcodeddefer os.Remove("test.db")attempts to remove a non-existent file from the current directory and is redundant—the temp directory is automatically cleaned up by Go's testing framework. Additionally,SetupTestArenaalready registers proper cleanup viat.Cleanup()to close the database connection.arduino/modbus_plc.py-56-63 (1)
56-63:⚠️ Potential issue | 🟡 MinorAdd explanatory comment for active-low relay logic on output coils.
The output inversion is intentional for active-low relay hardware (matching the input pattern documented at line 77), but the code lacks an explanatory comment. Unlike the input handling (which includes an inline comment at line 43 clarifying the active-low behavior), the output logic on lines 56-63 provides no such explanation, making it appear unintentional.
Add a comment similar to the input logic to clarify:
# 輸出: Active-low relay (Modbus 0 -> GPIO HIGH [activate], Modbus 1 -> GPIO LOW [deactivate])network/switch.go-65-77 (1)
65-77:⚠️ Potential issue | 🟡 MinorDuplicate step numbering in comments and silent failure on non-existent VLANs.
Line 65 is labeled "1. Remove the old DHCP Server settings", Line 72 is labeled "2. Remove Interface VLANs", but Line 86 is also labeled "2. Create new configuration". The second "2" should be "3".
Additionally, if this is a fresh switch (no VLANs exist), the
deletecommands will return errors from Fortinet CLI. TherunCommanderror return on Line 79 may causeConfigureTeamEthernetto fail on first use. Consider whether these deletion errors should be treated as non-fatal.templates/referee_panel.html-89-92 (1)
89-92:⚠️ Potential issue | 🟡 MinorDuplicate element IDs in reusable
scoreSummarytemplate.The tower elements use hardcoded IDs like
id="red-tower-0",id="red-tower-1",id="red-tower-2". SincescoreSummaryis instantiated twice (once forblueScoreSummaryand once forredScoreSummaryon lines 33-34), both instances will have the samered-tower-*IDs in the DOM. Duplicate IDs violate the HTML spec and causegetElementById/ jQuery#idselectors to only find the first match.The JS (
referee_panel.jslines 136-148) already uses container-scoped class selectors like$(${containerId} .team-${i+1}-tower), so these IDs appear unused. Either remove theidattributes or parameterize them using the template's.idfield.Proposed fix: remove unused hardcoded IDs
- <div class="tower-symbol team-1-tower" id="red-tower-0">❌</div> - <div class="tower-symbol team-2-tower" id="red-tower-1">❌</div> - <div class="tower-symbol team-3-tower" id="red-tower-2">❌</div> + <div class="tower-symbol team-1-tower">❌</div> + <div class="tower-symbol team-2-tower">❌</div> + <div class="tower-symbol team-3-tower">❌</div>static/js/referee_panel.js-97-105 (1)
97-105:⚠️ Potential issue | 🟡 MinorRemove unnecessary fallbacks that don't match the backend's actual message structure.
Lines 101-105 have fallback chains that don't correspond to what the backend sends. The
realtimeScorewebsocket message (generated in Go at field/arena_notifiers.go:252) sendsRedandBlueproperties (notRedRealtimeScore/BlueRealtimeScore), each with aScorefield (notCurrentScore). Simplify to:const redScore = data.Red.Score || {}; const blueScore = data.Blue.Score || {};Additionally, lines 205-208 reference
#redFarScoreStatusand#blueFarScoreStatus, which are commented out in the HTML template. Remove these dead calls or uncomment the elements if still needed.static/js/scoring_panel.js-4-7 (1)
4-7:⚠️ Potential issue | 🟡 MinorDefault timing values are inconsistent with the server-side defaults.
The client defaults
AutoDurationSec: 15andTeleopDurationSec: 135, butgame/match_timing.godefinesAutoDurationSec: 20andTeleopDurationSec: 140. While these will be overwritten whenmatchTimingarrives from the server, any logic that runs before the first websocket message will use incorrect timing.web/scoring_panel.go-148-166 (1)
148-166:⚠️ Potential issue | 🟡 MinorEdge case: if the
"score"envelope lacks an inner"command", the outer command remains"score"and silently falls through with no action.If the client sends
{"type": "score", ...}without a nestedcommandfield, the code keepscommand = "score"which doesn't match any handler — the message is silently ignored. Consider logging a warning or returning an error for malformed envelopes.game/score.go-60-71 (1)
60-71:⚠️ Potential issue | 🟡 MinorIncomplete comment: says "Level 2 (20pts), Level 3 (30pts)" but omits Level 1 (10pts) which is handled in the code.
Fix the comment
- // Endgame Tower: Level 2 (20pts), Level 3 (30pts) + // Endgame Tower: Level 1 (10pts), Level 2 (20pts), Level 3 (30pts)
🧹 Nitpick comments (46)
templates/match_review.html (1)
89-105: Implicit globalmatchResultandallianceResults.
matchResultandallianceResultsare not declared withvar/let/constin this scope — they rely on being globals defined inmatch_review.js. This works but is fragile. Consider adding a brief comment or an explicitvardeclaration for clarity.web/web.go (1)
1-2: Consider removing the personal attribution comment above the copyright.Line 1 adds a date/name comment (
//2026 1/22 Cindy) before the existing copyright header. This is inconsistent with the project's attribution style (copyright + author in the original header). Prefer using git blame for tracking authorship.arduino/modbus-plc.service (1)
7-9: Service runs as root — consider using a least-privilege user.Running the Modbus PLC service as
rootis a security risk, especially for a network-facing service. If GPIO access requires elevated privileges, consider using a dedicated service account added to the appropriate groups (e.g.,gpio) instead.arduino/red_field_hub.ino (1)
29-31: Use computed array size instead of magic number4.If
PIN_SENSORSis ever modified, the loop bound won't automatically adjust.Proposed fix
- for (int i = 0; i < 4; i++) { + for (int i = 0; i < sizeof(PIN_SENSORS)/sizeof(PIN_SENSORS[0]); i++) {Apply the same change on line 42.
network/switch_test.go (1)
60-65: Weakened assertions lose test value.Replacing exact string matching with
assert.Containsfor the all-teams test case (Lines 63-65) only verifies that a few substrings appear, without validating the complete command structure. If the ordering, IP addresses, or other VLAN parameters are wrong, this test won't catch it. Consider restoring full command assertion once the mock is fixed.arduino/modbus_plc.py (2)
86-88:GPIO.cleanup()is unreachable —StartTcpServerblocks indefinitely.
StartTcpServeron Line 87 is a blocking call that only returns on server shutdown (e.g., signal).GPIO.cleanup()on Line 88 will never execute during normal operation. Use a signal handler ortry/finallyto ensure cleanup:🛠️ Proposed fix
- StartTcpServer(context, address=("0.0.0.0", 502)) - GPIO.cleanup() + try: + StartTcpServer(context, address=("0.0.0.0", 502)) + finally: + GPIO.cleanup()
19-26: Module-level GPIO initialization makes this file un-importable on non-Jetson hosts.GPIO setup (Lines 20-26) and Modbus store creation (Lines 29-32) execute at import time. This prevents importing the module for testing or reuse on machines without Jetson GPIO hardware. Consider moving initialization into a function or guarding it with
if __name__ == "__main__".arduino/driverStation.py (3)
1-77: Significant code duplication witharduino/modbus_plc.py.Both files share nearly identical structure: GPIO init, Modbus store setup,
background_loop, daemon thread + TCP server. The main differences are the pin mappings (6 inputs vs 4 inputs + 2 outputs) and polling rate (0.05s vs 0.1s). Consider extracting the shared scaffolding into a common base module and parameterizing the pin configuration and loop behavior.
8-8:import sysis unused.
sysis imported on Line 8 but never used in this file. Same applies tomodbus_plc.py.
65-76: SameGPIO.cleanup()unreachability issue asmodbus_plc.py.
StartTcpServeron Line 75 blocks, makingGPIO.cleanup()on Line 76 unreachable. Apply the sametry/finallyfix suggested for the other file.templates/edit_match_result.html (1)
138-150: Defensive JSON parsing — good, but consider therawJsontype more carefully.The fallback logic is solid. However, on Line 139,
{{.MatchResultJson}}is rendered by Go'stemplate/htmlpackage. If the server outputs a JSON object literal directly (not a string), thenrawJsonis already a JS object, andtypeof rawJson === "string"correctly handles both cases. If the server outputsnullor an empty string, the guard on Line 141 catches it.One edge case: if
MatchResultJsonisundefined(template field missing), Line 139 would cause a JS error before the guard. Ensure the server always provides this field.web/referee_panel.go (1)
178-227: Card command: no validation ofargs.TeamIdagainst actual match teams in qualification mode.In qualification mode (Line 219-226), the card is applied to whichever
TeamIdis sent by the client without verifying that the team actually belongs to the specified alliance or is part of the current match. A malformed or malicious WebSocket message could associate a card with an arbitrary team ID.Since this is an admin-only endpoint (Line 77-79), the risk is low, but consider adding a check that
args.TeamIdis one of the current match's team IDs.static/css/audience_display.css (3)
38-47: Class names.Dive,.Reescape, and.Wrapperuse PascalCase instead of kebab-case.Stylelint flags these (lines 38, 43, 83). While functional, PascalCase class names are unconventional in CSS and inconsistent with the rest of the selectors in this file (e.g.,
.score-field,.final-team-row). Consider renaming to.dive,.reescape,.wrapper(or more descriptive names like.event-brand-left,.event-brand-right).
472-490: Duplicate#leftFinalScoreand#rightFinalScoreselectors.
#leftFinalScoreis defined at Line 472 and again at Line 485;#rightFinalScoreat Line 479 and Line 488. While the rules are additive (background-color vs. padding), splitting the same selector creates maintenance confusion. Merge them into single rule blocks.♻️ Suggested merge
-#leftFinalScore{ - background-color: red; - -} ... -#leftFinalScore { - padding-right: 150px; -} +#leftFinalScore { + background-color: red; + padding-right: 150px; +} -#rightFinalScore{ - background-color: `#2080ff`; -} ... -#rightFinalScore { - padding-left: 150px; -} +#rightFinalScore { + background-color: `#2080ff`; + padding-left: 150px; +}
155-162: Commented-out.avatarsblock — remove dead CSS.This block is fully commented out. If it's no longer needed with the 2026 layout, remove it to reduce file noise.
network/switch.go (1)
38-38: Hardcoded Fortinet username"admin"— not configurable.The
usernameis set to"admin"inNewSwitchand is not exposed throughEventSettings. If a deployment uses a different Fortinet username, there's no way to configure it without code changes. Consider adding it as a parameter or settings field.Also applies to: 48-52
static/js/match_review.js (2)
139-146: CSS attribute selectors lack value quoting — fragile with certain values.In
getInputElement(Line 141) andgetSelectElement(Line 146), the[value=${value}]selector inserts the value directly without CSS quotes. While team IDs are numeric and this works in practice, any value containing spaces, brackets, or special CSS characters would break the selector silently (returning an empty jQuery set).♻️ Safer selector construction
const getInputElement = function (alliance, name, value) { let selector = `input[name=${alliance}${name}]`; - if (value !== undefined) selector += `[value=${value}]`; + if (value !== undefined) selector += `[value="${value}"]`; return $(selector); };
50-58: EndgameStatuses fallback path bypasses jQuery safety.Line 54 constructs a raw jQuery selector
$(\input[name=EndgameStatuses${htmlIdx}][value=${...}]`)as a fallback when the alliance-prefixed input isn't found. This fallback queries inputs without the alliance prefix, which could match inputs from the *other* alliance's section if both are rendered on the same page. Since the template uses{{"{{alliance}}"}}` prefix for all input names, this fallback path may never match anything useful and could be dead code.templates/audience_display.html (1)
121-124: Inline styles on#shiftCounterduplicate CSS rules.The
styleattribute on Line 121 (font-family: 'Arboto Bold'; font-size: 24px; color:#222; text-align: center;) duplicates rules already defined inaudience_display.cssLines 252-258 for#shiftCounter. Inline styles override CSS and make maintenance harder. Remove the inline styles and rely on the stylesheet.web/match_review.go (1)
87-94: Comments are in Chinese — consider translating to English for consistency.The rest of the codebase uses English comments. Lines 87, 92, 102, 109, and 128 contain Chinese comments that should be translated for maintainability and consistency.
Also applies to: 102-103, 109-109, 128-129
plc/register_string.go (2)
1-1: Same concern: generated file edited manually.See the same note as for
plc/input_string.go— this file's header saysDO NOT EDIT, but it has been manually modified.
22-22:- 0is a no-op.
idx := int(i) - 0is equivalent toidx := int(i). This is a harmless artifact of the stringer template, but if you're maintaining this file manually now, you could simplify it.field/arena_notifiers.go (1)
238-245: Prefer named fields in struct literal to avoid positional fragility.
MatchTimeMessagenow has 6 fields. A positional literal silently assigns wrong values if fields are reordered. Use named fields for clarity and safety.Proposed fix
- return MatchTimeMessage{ - arena.MatchState, - int(arena.MatchTimeSec()), - arena.RedRealtimeScore.CurrentScore.HubActive, - arena.BlueRealtimeScore.CurrentScore.HubActive, - currentShift, - shiftTimeSec, - } + return MatchTimeMessage{ + MatchState: arena.MatchState, + MatchTimeSec: int(arena.MatchTimeSec()), + HubActiveRed: arena.RedRealtimeScore.CurrentScore.HubActive, + HubActiveBlue: arena.BlueRealtimeScore.CurrentScore.HubActive, + CurrentShift: currentShift, + ShiftTimeSec: shiftTimeSec, + }game/ranking_fields_test.go (2)
15-69: Tests don't assertPlayedcount orAutoPoints.Each scenario resets
rankingFieldsbut never assertsPlayedincrements orAutoPointsaccumulation. Consider adding at least:assert.Equal(t, 1, rankingFields.Played) assert.Equal(t, 25, rankingFields.AutoPoints) // for the win caseThis would ensure
AddScoreSummarycorrectly tracks all ranking fields, not just the ones currently tested.
55-61: Tie test uses the same pointer for both arguments.
rankingFields.AddScoreSummary(tieScore, tieScore, false)passes the identical pointer as both "our" and "opponent" summary. This works for a tie scenario but doesn't verify the method handles distinct objects with equal scores. Consider using two separateScoreSummaryinstances with the same values to avoid masking pointer-aliasing bugs inAddScoreSummary.web/scoring_panel_test.go (1)
196-196:time.Sleepfor synchronization is flaky.Using
time.Sleep(time.Millisecond * 10)to wait for async processing is a common source of test flakiness. Consider using a polling/retry loop or a channel-based synchronization mechanism if available.templates/referee_panel.html (2)
36-44: Misleading indentation in the fouls section — easy to misread nesting.The
navigation-buttonsdiv (line 36) and the</div>at line 44 are indented inconsistently relative to the containing<div id="fouls">(line 31). This makes it look like the</div>at line 44 closesfouls, when it actually closesnavigation-buttons. The subsequent<h3>Fouls</h3>(line 45) and foul buttons are still insidefoulsbut appear as if they're outside.Fix indentation to clarify nesting
- <div class="navigation-buttons" style="margin: 10px; display: flex; gap: 10px;"> - <button class="btn btn-primary" onclick="window.location.href='/panels/scoring/blue_near'"> - Blue Scoring Panel - </button> - <button class="btn btn-danger" onclick="window.location.href='/panels/scoring/red_near'"> - Red Scoring Panel - </button> - - </div> + <div class="navigation-buttons" style="margin: 10px; display: flex; gap: 10px;"> + <button class="btn btn-primary" onclick="window.location.href='/panels/scoring/blue_near'"> + Blue Scoring Panel + </button> + <button class="btn btn-danger" onclick="window.location.href='/panels/scoring/red_near'"> + Red Scoring Panel + </button> + </div>
108-113: Inconsistent CSS class casing:Energized-statusvssupercharged-status/traversal-status.Line 110 uses
Energized-status(capital E) while lines 111-112 usesupercharged-statusandtraversal-status(lowercase). This inconsistency must match the selectors inreferee_panel.js(line 177:.Energized-status, line 183:.supercharged-status, line 189:.traversal-status). Currently they do match, but the mixed casing is error-prone for maintenance. Consider normalizing to lowercase.static/js/referee_panel.js (3)
124-133: Missing.catch()on fetch — network errors are silently swallowed.The
fetch("/panels/referee/foul_list")chain has no.catch()handler. If the request fails (e.g., network issue, server error), the promise rejection will go unhandled, potentially triggering anunhandledrejectionevent in modern browsers.Proposed fix
fetch("/panels/referee/foul_list") .then(response => response.text()) .then(html => { if (html.indexOf("<!DOCTYPE") === -1 && html.indexOf("<html") === -1) { $("#foulList").html(html); } else { console.error("Foul list error: Received full page instead of snippet."); } - }); + }) + .catch(err => console.error("Failed to fetch foul list:", err));
205-208:handleScoringStatusreferences commented-out DOM elements.Lines 206 and 208 update
#redFarScoreStatusand#blueFarScoreStatus, but these elements are commented out inreferee_panel.html(lines 25, 27). jQuery will silently no-op, but this is dead code that should be cleaned up or the HTML elements should be uncommented if still needed.
172-193: Remove camelCase property checks — only PascalCase is present in the JSON payload.Lines 178, 184, 190 each check two casing variants (e.g.,
summary.EnergizedRankingPoint || summary.energizedRankingPoint). The Go backend serializesScoreSummaryfields in PascalCase by default; onlyEnergizedRankingPoint,SuperchargedRankingPoint, andTraversalRankingPointwill ever be present in the websocket message. The camelCase checks are dead code.game/test_helpers.go (1)
53-58: Positional struct literals are fragile — use named fields.
RankingFields{20, 625, 90, 100, 0.254, 3, 2, 1, 0, 10}relies on exact field ordering. IfRankingFieldsgains, removes, or reorders a field, these will silently compile with wrong values. Named fields are safer and self-documenting.Proposed refactor
func TestRanking1() *Ranking { - return &Ranking{254, 1, 0, RankingFields{20, 625, 90, 100, 0.254, 3, 2, 1, 0, 10}} + return &Ranking{ + TeamId: 254, Rank: 1, PreviousRank: 0, + RankingFields: RankingFields{ + RankingPoints: 20, MatchPoints: 625, AutoPoints: 90, TowerPoints: 100, + Random: 0.254, Wins: 3, Losses: 2, Ties: 1, Disqualifications: 0, Played: 10, + }, + } }plc/plc.go (1)
62-63: Typo in constant name:plcRetryIntevalSec→plcRetryIntervalSec.Fix
- plcRetryIntevalSec = 3 + plcRetryIntervalSec = 3Also update the usage on line 178.
plc/plc_test.go (1)
292-300: Empty test body — consider removing or adding at.Skipwith a reason.
TestPlcInputsGameSpecifichas no assertions. If it's intentionally empty for 2026, uset.Skip("No game-specific inputs in 2026")to make the intent clear and show up in test output.templates/scoring_panel.html (3)
59-76: Large block of commented-out code should be removed or tracked.The entire Fouls card (lines 59-76) is commented out with
<!-- -->. If this is intentionally disabled for 2026, add a TODO or link to an issue. Leaving large commented-out blocks degrades readability.
131-131:transform: scale(1.5)may cause layout overflow on small screens.Scaling the teleop fuel input group to 150% via inline CSS could cause it to overflow its container on mobile devices, despite the viewport meta tag. Consider using larger Bootstrap sizing classes (
btn-xlor custom padding) instead of a CSS transform, which doesn't affect the element's layout box.
215-224: Globalpositionandalliancevariables declared withvarin a<script>tag.These are implicitly global. While
scoring_panel.jslikely expects them, this coupling is fragile. Consider passing them via adata-*attribute on the container div or via a config object to make the dependency explicit.web/scoring_panel.go (2)
10-16: Remove commented-outlogimport and scattered debug log statements.The commented-out
//logimport (line 13) and numerous//log.Printf(...)statements throughout the file are debug artifacts. They add noise and should be removed before merging.
301-306: Remove custommaxfunction — it shadows the Go builtin.Go 1.21 introduced
maxas a builtin function. This project requires Go 1.23.0, so the builtin is available. The custommaxfunction (lines 301-306) shadows it, making code less clear and tools less useful. Replace all calls tomax()with the builtin.static/js/scoring_panel.js (3)
3-9:matchTimingis assigned withoutvar/let/const, creating an implicit global.This works because
match_timing.jslikely expects a globalmatchTiming, but implicit globals are error-prone and fail in strict mode. Declare it explicitly.Suggested fix
-matchTiming = { +var matchTiming = { AutoDurationSec: 15, WarmupDurationSec: 0, TeleopDurationSec: 135,
19-25: Sending a zero-delta fuel command as a "refresh" is a side-effectful workaround.Line 23 sends
{ Adjustment: 0, Autonomous: true }which triggers server-side score processing and aRealtimeScoreNotifier.Notify()(sincescoreChangedwill be set totrueon the server). While functional, this is fragile—if the server changes to skip no-ops, the refresh breaks. A dedicated"refresh"or"getScore"command would be cleaner.
77-93:alliancevariable is referenced but never declared in this file.
alliance(line 78) must be injected as a global from the HTML template. This implicit dependency is fragile. Consider documenting expected globals at the top of the file, or passing them explicitly during initialization.field/arena.go (2)
1179-1192: Sending game data packet on every DS packet cycle may be excessive.
sendGameDataPacketis called every ~500ms (the DS packet period) with the same static message for the entire teleop period. If this creates a new TCP message each time, consider sending it only once or when the message changes.
129-131: Remove deprecatedrand.Seedcall.
rand.Seedis deprecated since Go 1.20. The top-levelrandfunctions are automatically seeded at program startup, so this call is unnecessary and can be removed to suppress deprecation warnings.Proposed fix
- // Seed the random number generator for tie-breakers - rand.Seed(time.Now().UnixNano()) - arena.AllianceStations = make(map[string]*AllianceStation)static/js/audience_display.js (2)
174-184: Magic number5for TeleopPeriod match state.
data.MatchState === 5relies on the Go enum's integer value. If the enum order changes, this breaks silently. Define a named constant (e.g.,const MATCH_STATE_TELEOP = 5;) for clarity and maintainability.
193-196: Remove commented-out old scoring code.Lines 193-194 contain the old
BargePoints-based score rendering. These are dead code artifacts.
| func mockTelnet(t *testing.T, port int, command1 *string, command2 *string) { | ||
| go func() { | ||
| ln, err := net.Listen("tcp", fmt.Sprintf(":%d", port)) | ||
| assert.Nil(t, err) | ||
| if err != nil { | ||
| return // Avoid errors during parallel tests | ||
| } | ||
| defer ln.Close() | ||
| *command1 = "" | ||
| *command2 = "" | ||
|
|
||
| // Fake the first connection. | ||
| // Simulate first connection (Reset) | ||
| conn1, err := ln.Accept() | ||
| assert.Nil(t, err) | ||
| conn1.SetReadDeadline(time.Now().Add(10 * time.Millisecond)) | ||
| var reader bytes.Buffer | ||
| reader.ReadFrom(conn1) | ||
| *command1 = reader.String() | ||
| conn1.Close() | ||
| if err == nil { | ||
| conn1.SetReadDeadline(time.Now().Add(50 * time.Millisecond)) | ||
| var reader bytes.Buffer | ||
| reader.ReadFrom(conn1) | ||
| *command1 = reader.String() | ||
| conn1.Close() | ||
| } | ||
|
|
||
| // Fake the second connection. | ||
| // Simulate second connection (Config) | ||
| conn2, err := ln.Accept() | ||
| assert.Nil(t, err) | ||
| conn2.SetReadDeadline(time.Now().Add(10 * time.Millisecond)) | ||
| reader.Reset() | ||
| reader.ReadFrom(conn2) | ||
| *command2 = reader.String() | ||
| conn2.Close() | ||
| if err == nil { | ||
| conn2.SetReadDeadline(time.Now().Add(50 * time.Millisecond)) | ||
| var reader bytes.Buffer | ||
| reader.ReadFrom(conn2) | ||
| *command2 = reader.String() | ||
| conn2.Close() | ||
| } | ||
| }() | ||
| time.Sleep(100 * time.Millisecond) // Give it some time to open the socket. | ||
| time.Sleep(100 * time.Millisecond) |
There was a problem hiding this comment.
Mock telnet server doesn't simulate Fortinet login prompts — root cause of all pipeline failures.
The mockTelnet helper opens a plain TCP socket and immediately tries to read, but the Fortinet switch implementation in network/switch.go likely expects an interactive login prompt (e.g., a login: or Password: banner) before sending credentials and commands. Since the mock never writes anything to the connection, runCommand in the switch code is probably blocking on a prompt read, causing the 50ms read deadline in the mock to expire with empty data.
This explains all six pipeline failures — command1 and command2 are always empty strings.
To fix this, the mock needs to simulate the Fortinet telnet handshake:
🛠️ Sketch of a prompt-aware mock
- // Simulate first connection (Reset)
conn1, err := ln.Accept()
if err == nil {
- conn1.SetReadDeadline(time.Now().Add(50 * time.Millisecond))
- var reader bytes.Buffer
- reader.ReadFrom(conn1)
- *command1 = reader.String()
- conn1.Close()
+ // Send login prompt and read the full command exchange
+ conn1.Write([]byte("login: "))
+ // ... read username, send password prompt, read password, then read commands
+ conn1.SetReadDeadline(time.Now().Add(200 * time.Millisecond))
+ var reader bytes.Buffer
+ reader.ReadFrom(conn1)
+ *command1 = reader.String()
+ conn1.Close()
}You'll need to inspect the actual telnet flow in switch.go's runCommand to determine the exact prompt/response sequence the mock must emulate.
🤖 Prompt for AI Agents
In `@network/switch_test.go` around lines 68 - 98, The mockTelnet helper currently
never writes any prompts, so runCommand in network/switch.go blocks waiting for
login/password prompts; update mockTelnet to emulate the Fortinet telnet
handshake by writing the expected login banner and password prompt to each
accepted connection (e.g., send "login:" and then "Password:" or whatever
runCommand expects), then read credentials sent by the client and respond with
the command prompt before allowing the test reads to capture the sent commands;
keep the existing Accept/read structure (conn1/conn2) and use conn.Write with
appropriate timing/deadlines so runCommand advances instead of timing out.
| for i, ranking := range rankings { | ||
| tbaRankings[i] = TbaRanking{ | ||
| TeamKey: getTbaTeam(ranking.TeamId), | ||
| Rank: ranking.Rank, | ||
| RP: float32(ranking.RankingPoints) / float32(ranking.Played), | ||
| Coop: float32(ranking.CoopertitionPoints) / float32(ranking.Played), | ||
| Match: float32(ranking.MatchPoints) / float32(ranking.Played), | ||
| Auto: float32(ranking.AutoPoints) / float32(ranking.Played), | ||
| Barge: float32(ranking.BargePoints) / float32(ranking.Played), | ||
| Wins: ranking.Wins, | ||
| Losses: ranking.Losses, | ||
| Ties: ranking.Ties, | ||
| Dqs: ranking.Disqualifications, | ||
| Played: ranking.Played, | ||
| // 修正點 1: 使用 TowerPoints 取代 TotalTowerPoints | ||
| Tower: float32(ranking.TowerPoints) / float32(ranking.Played), | ||
| Wins: ranking.Wins, | ||
| Losses: ranking.Losses, | ||
| Ties: ranking.Ties, | ||
| Dqs: ranking.Disqualifications, | ||
| Played: ranking.Played, | ||
| } |
There was a problem hiding this comment.
Potential division by zero if ranking.Played is 0.
Lines 402-406 divide by ranking.Played to compute per-match averages. If a team has Played == 0 (e.g., no completed matches yet), this causes a division-by-zero panic. Add a guard:
🐛 Proposed fix
for i, ranking := range rankings {
+ played := ranking.Played
+ if played == 0 {
+ played = 1 // Avoid division by zero; averages will be 0
+ }
tbaRankings[i] = TbaRanking{
TeamKey: getTbaTeam(ranking.TeamId),
Rank: ranking.Rank,
- RP: float32(ranking.RankingPoints) / float32(ranking.Played),
- Match: float32(ranking.MatchPoints) / float32(ranking.Played),
- Auto: float32(ranking.AutoPoints) / float32(ranking.Played),
- Tower: float32(ranking.TowerPoints) / float32(ranking.Played),
+ RP: float32(ranking.RankingPoints) / float32(played),
+ Match: float32(ranking.MatchPoints) / float32(played),
+ Auto: float32(ranking.AutoPoints) / float32(played),
+ Tower: float32(ranking.TowerPoints) / float32(played),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for i, ranking := range rankings { | |
| tbaRankings[i] = TbaRanking{ | |
| TeamKey: getTbaTeam(ranking.TeamId), | |
| Rank: ranking.Rank, | |
| RP: float32(ranking.RankingPoints) / float32(ranking.Played), | |
| Coop: float32(ranking.CoopertitionPoints) / float32(ranking.Played), | |
| Match: float32(ranking.MatchPoints) / float32(ranking.Played), | |
| Auto: float32(ranking.AutoPoints) / float32(ranking.Played), | |
| Barge: float32(ranking.BargePoints) / float32(ranking.Played), | |
| Wins: ranking.Wins, | |
| Losses: ranking.Losses, | |
| Ties: ranking.Ties, | |
| Dqs: ranking.Disqualifications, | |
| Played: ranking.Played, | |
| // 修正點 1: 使用 TowerPoints 取代 TotalTowerPoints | |
| Tower: float32(ranking.TowerPoints) / float32(ranking.Played), | |
| Wins: ranking.Wins, | |
| Losses: ranking.Losses, | |
| Ties: ranking.Ties, | |
| Dqs: ranking.Disqualifications, | |
| Played: ranking.Played, | |
| } | |
| for i, ranking := range rankings { | |
| played := ranking.Played | |
| if played == 0 { | |
| played = 1 // Avoid division by zero; averages will be 0 | |
| } | |
| tbaRankings[i] = TbaRanking{ | |
| TeamKey: getTbaTeam(ranking.TeamId), | |
| Rank: ranking.Rank, | |
| RP: float32(ranking.RankingPoints) / float32(played), | |
| Match: float32(ranking.MatchPoints) / float32(played), | |
| Auto: float32(ranking.AutoPoints) / float32(played), | |
| // 修正點 1: 使用 TowerPoints 取代 TotalTowerPoints | |
| Tower: float32(ranking.TowerPoints) / float32(played), | |
| Wins: ranking.Wins, | |
| Losses: ranking.Losses, | |
| Ties: ranking.Ties, | |
| Dqs: ranking.Disqualifications, | |
| Played: ranking.Played, | |
| } |
🤖 Prompt for AI Agents
In `@partner/tba.go` around lines 398 - 412, The code currently divides by
ranking.Played when populating TbaRanking (fields RP, Match, Auto, Tower),
causing a division-by-zero panic if Played == 0; fix by guarding those
calculations: before assigning to tbaRankings[i], check if ranking.Played == 0
and set RP, Match, Auto, Tower to 0.0 (or other safe default), otherwise compute
them using float32(ranking.Played); update the TbaRanking construction in the
loop that fills tbaRankings to use this guard for RP, Match, Auto, and Tower.
| // First block: Define Address 0 | ||
| const ( | ||
| fieldEStop input = iota | ||
| red1EStop | ||
| red1AStop | ||
| red2EStop | ||
| red2AStop | ||
| red3EStop | ||
| red3AStop | ||
| blue1EStop | ||
| blue1AStop | ||
| blue2EStop | ||
| blue2AStop | ||
| blue3EStop | ||
| blue3AStop | ||
| redConnected1 | ||
| redConnected2 | ||
| redConnected3 | ||
| blueConnected1 | ||
| blueConnected2 | ||
| blueConnected3 | ||
| fieldEStop input = iota // iota = 0, so fieldEStop = 0 | ||
| ) | ||
|
|
||
| // Second block: Define inputs starting at Address 20 | ||
| // In a new const block, iota resets to 0 | ||
| const ( | ||
| red1EStop input = iota + 16 // iota = 0, value = 16 | ||
| red1AStop // iota = 1, value = 21 (automatically follows +20 rule) | ||
| red2EStop // 22 | ||
| red2AStop // 23 | ||
| red3EStop // 24 | ||
| red3AStop // 25 | ||
| redConnected1 // 32 | ||
| redConnected2 // 33 | ||
| redConnected3 // 34 | ||
| blue1EStop input = iota + 23 // 40 | ||
| blue1AStop // 41 | ||
| blue2EStop // 42 | ||
| blue2AStop // 43 | ||
| blue3EStop // 44 | ||
| blue3AStop // 45 | ||
| blueConnected1 // 46 | ||
| blueConnected2 // 47 | ||
| blueConnected3 // 48 | ||
|
|
||
| // inputCount will automatically become the last value + 1 | ||
| // Here it will be 49, used to define the array size | ||
| inputCount | ||
| ) |
There was a problem hiding this comment.
Critical: Input constant values are incorrect — causes the pipeline panic at Line 354.
The comments claim values like "21", "32", etc., but the actual iota arithmetic produces different values, AND creates large gaps in the index space:
| Constant | Comment claims | Actual value |
|---|---|---|
red1AStop |
21 | 17 |
redConnected1 |
32 | 22 |
blue1EStop |
40 | 32 |
inputCount |
49 | 41 |
The gaps (indices 1–15, 25–31) mean inputCount = 41, causing ReadDiscreteInputs(0, 41) to request more inputs than the device provides, triggering the pipeline slice bounds panic at Line 354. Additionally, GetInputNames() returns 41 entries (with unnamed gap indices), failing the test that expects 19 names.
If the intent is a contiguous mapping, use explicit values or a single iota block:
Proposed fix: use explicit constants to match actual hardware addresses
-// First block: Define Address 0
-const (
- fieldEStop input = iota // iota = 0, so fieldEStop = 0
-)
-
-// Second block: Define inputs starting at Address 20
-// In a new const block, iota resets to 0
-const (
- red1EStop input = iota + 16 // iota = 0, value = 16
- red1AStop // iota = 1, value = 21 (automatically follows +20 rule)
- red2EStop // 22
- red2AStop // 23
- red3EStop // 24
- red3AStop // 25
- redConnected1 // 32
- redConnected2 // 33
- redConnected3 // 34
- blue1EStop input = iota + 23 // 40
- blue1AStop // 41
- blue2EStop // 42
- blue2AStop // 43
- blue3EStop // 44
- blue3AStop // 45
- blueConnected1 // 46
- blueConnected2 // 47
- blueConnected3 // 48
-
- // inputCount will automatically become the last value + 1
- // Here it will be 49, used to define the array size
- inputCount
-)
+const (
+ fieldEStop input = iota
+ red1EStop
+ red1AStop
+ red2EStop
+ red2AStop
+ red3EStop
+ red3AStop
+ redConnected1
+ redConnected2
+ redConnected3
+ blue1EStop
+ blue1AStop
+ blue2EStop
+ blue2AStop
+ blue3EStop
+ blue3AStop
+ blueConnected1
+ blueConnected2
+ blueConnected3
+ inputCount
+)If specific Modbus addresses are required (e.g., fieldEStop at address 0 and red1EStop at address 16), then a separate address-mapping table should be used instead of overloading the array index with the hardware address. The array index for plc.inputs must be contiguous.
🤖 Prompt for AI Agents
In `@plc/plc.go` around lines 71 - 101, The constants for input indices (e.g.,
red1EStop, red1AStop, redConnected1, blue1EStop, inputCount) are non-contiguous
due to resetting iota and inline arithmetic, causing plc.inputs size
misalignment and ReadDiscreteInputs/ GetInputNames failures; fix by creating a
single contiguous index scheme for the plc input array (either put all inputs in
one const block using iota in order or assign explicit sequential integer
indices) and if you must preserve Modbus hardware addresses keep them in a
separate mapping table (e.g., hardwareAddressByInput) so plc.inputs uses
contiguous indices and inputCount reflects the true array length used by
ReadDiscreteInputs and GetInputNames.
| if (currentMatch.Type === matchTypePlayoff) { | ||
| redCoral = data.Red.ScoreSummary.NumCoral; | ||
| blueCoral = data.Blue.ScoreSummary.NumCoral; | ||
| redCoral = data.Red.ScoreSummary.NumCoral; | ||
| blueCoral = data.Blue.ScoreSummary.NumCoral; | ||
| } else { | ||
| redCoral = `${data.Red.ScoreSummary.NumCoralLevels}/${data.Red.ScoreSummary.NumCoralLevelsGoal}`; | ||
| blueCoral = `${data.Blue.ScoreSummary.NumCoralLevels}/${data.Blue.ScoreSummary.NumCoralLevelsGoal}`; | ||
| // 1. 計算紅藍兩隊的目標值 | ||
| const redFuelGoal = getFuelGoal(data.Red.ScoreSummary); | ||
| const blueFuelGoal = getFuelGoal(data.Blue.ScoreSummary); | ||
|
|
||
| // 2. 使用樣板字面值 (Template Literals) 正確帶入變數 | ||
| // 修正了你紅藍兩隊後綴不統一的問題（原本藍隊用 NumCoralLevelsGoal） | ||
| redCoral = `${data.Red.ScoreSummary.TotalFuelPoints}/${redFuelGoal}`; | ||
| blueCoral = `${data.Blue.ScoreSummary.TotalFuelPoints}/${blueFuelGoal}`; | ||
| } | ||
| $(`#${redSide}Coral`).text(redCoral); | ||
| $(`#${redSide}Algae`).text(data.Red.ScoreSummary.NumAlgae); | ||
| $(`#${blueSide}Coral`).text(blueCoral); | ||
| $(`#${blueSide}Algae`).text(data.Blue.ScoreSummary.NumAlgae); |
There was a problem hiding this comment.
References to removed fields: NumCoral and NumAlgae will be undefined.
ScoreSummary no longer has NumCoral (line 204-205) or NumAlgae (lines 217-219) in the 2026 model. These will render as undefined in the UI. The playoff path on line 204-205 should use the appropriate 2026 field (e.g., TotalFuelPoints), and the NumAlgae display should be updated or removed.
🤖 Prompt for AI Agents
In `@static/js/audience_display.js` around lines 203 - 219, The playoff branch
currently reads deprecated fields (data.Red.ScoreSummary.NumCoral / NumAlgae)
causing undefined UI; change the playoff path that checks currentMatch.Type ===
matchTypePlayoff to use the 2026 fields (e.g., use
data.Red.ScoreSummary.TotalFuelPoints and data.Blue.ScoreSummary.TotalFuelPoints
for redCoral/blueCoral) and remove or replace uses of
data.*.ScoreSummary.NumAlgae when setting $(`#${redSide}Algae`) and
$(`#${blueSide}Algae`) (either map to the new algae field name in ScoreSummary
or omit the algae element update if the field no longer exists), keeping the
element IDs redSideCoral, blueSideCoral, redSideAlgae, blueSideAlgae unchanged.
| <img class="Dive" src="/static\img\frc_rebuilt.png" /> | ||
| <!--<span>{{.EventSettings.Name}}</span>--> | ||
| <span id="matchName"></span> | ||
| <img class="Reescape" src="/static\img\frc_age.png" /> |
There was a problem hiding this comment.
Backslashes in image src paths will cause 404 errors.
Lines 24 and 27 use Windows-style backslashes in URLs:
src="/static\img\frc_rebuilt.png"src="/static\img\frc_age.png"
Web URLs require forward slashes. Most browsers and web servers will not resolve these paths, resulting in broken images on the audience display.
🐛 Fix path separators
-<img class="Dive" src="/static\img\frc_rebuilt.png" />
+<img class="Dive" src="/static/img/frc_rebuilt.png" />
<!--<span>{{.EventSettings.Name}}</span>-->
<span id="matchName"></span>
-<img class="Reescape" src="/static\img\frc_age.png" />
+<img class="Reescape" src="/static/img/frc_age.png" />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <img class="Dive" src="/static\img\frc_rebuilt.png" /> | |
| <!--<span>{{.EventSettings.Name}}</span>--> | |
| <span id="matchName"></span> | |
| <img class="Reescape" src="/static\img\frc_age.png" /> | |
| <img class="Dive" src="/static/img/frc_rebuilt.png" /> | |
| <!--<span>{{.EventSettings.Name}}</span>--> | |
| <span id="matchName"></span> | |
| <img class="Reescape" src="/static/img/frc_age.png" /> |
🤖 Prompt for AI Agents
In `@templates/audience_display.html` around lines 24 - 27, The img tags for the
audience display (classes "Dive" and "Reescape" in
templates/audience_display.html) use Windows backslashes in their src
attributes; update the src values for the <img class="Dive" src=...> and <img
class="Reescape" src=...> to use forward slashes (e.g., /static/img/...) so the
browser can resolve the image URLs correctly.
| $(document).ready(function() { | ||
| // 從 Go 後端傳入的 JSON 字串解析出原始資料 | ||
| matchResult = JSON.parse({{.MatchResultJson}}); | ||
| var rawJson = {{.MatchResultJson}}; | ||
|
|
||
| if (!rawJson || rawJson === "") { | ||
| matchResult = { | ||
| RedScore: { Fouls: [], RobotsBypassed: [false,false,false], AutoTowerLevel1: [false,false,false], EndgameStatuses: [0,0,0] }, | ||
| BlueScore: { Fouls: [], RobotsBypassed: [false,false,false], AutoTowerLevel1: [false,false,false], EndgameStatuses: [0,0,0] } | ||
| }; | ||
| } else { | ||
| try { | ||
| matchResult = (typeof rawJson === "string") ? JSON.parse(rawJson) : rawJson; | ||
| } catch (e) { | ||
| console.error("MatchResultJson parse error:", e); | ||
| matchResult = { RedScore: {Fouls:[]}, BlueScore: {Fouls:[]} }; | ||
| } | ||
| } |
There was a problem hiding this comment.
JSON.parse on line 73 will throw before the guarded parse on lines 82-87 is reached.
Line 73 unconditionally calls JSON.parse({{.MatchResultJson}}). If MatchResultJson is empty or malformed, this throws an uncaught exception and the try/catch block below never executes. Line 73 should be removed — the guarded block starting at line 76 already handles all cases.
🐛 Proposed fix
$(document).ready(function() {
- // 從 Go 後端傳入的 JSON 字串解析出原始資料
- matchResult = JSON.parse({{.MatchResultJson}});
var rawJson = {{.MatchResultJson}};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $(document).ready(function() { | |
| // 從 Go 後端傳入的 JSON 字串解析出原始資料 | |
| matchResult = JSON.parse({{.MatchResultJson}}); | |
| var rawJson = {{.MatchResultJson}}; | |
| if (!rawJson || rawJson === "") { | |
| matchResult = { | |
| RedScore: { Fouls: [], RobotsBypassed: [false,false,false], AutoTowerLevel1: [false,false,false], EndgameStatuses: [0,0,0] }, | |
| BlueScore: { Fouls: [], RobotsBypassed: [false,false,false], AutoTowerLevel1: [false,false,false], EndgameStatuses: [0,0,0] } | |
| }; | |
| } else { | |
| try { | |
| matchResult = (typeof rawJson === "string") ? JSON.parse(rawJson) : rawJson; | |
| } catch (e) { | |
| console.error("MatchResultJson parse error:", e); | |
| matchResult = { RedScore: {Fouls:[]}, BlueScore: {Fouls:[]} }; | |
| } | |
| } | |
| $(document).ready(function() { | |
| var rawJson = {{.MatchResultJson}}; | |
| if (!rawJson || rawJson === "") { | |
| matchResult = { | |
| RedScore: { Fouls: [], RobotsBypassed: [false,false,false], AutoTowerLevel1: [false,false,false], EndgameStatuses: [0,0,0] }, | |
| BlueScore: { Fouls: [], RobotsBypassed: [false,false,false], AutoTowerLevel1: [false,false,false], EndgameStatuses: [0,0,0] } | |
| }; | |
| } else { | |
| try { | |
| matchResult = (typeof rawJson === "string") ? JSON.parse(rawJson) : rawJson; | |
| } catch (e) { | |
| console.error("MatchResultJson parse error:", e); | |
| matchResult = { RedScore: {Fouls:[]}, BlueScore: {Fouls:[]} }; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@templates/match_review.html` around lines 71 - 88, Remove the unconditional
JSON.parse call that assigns matchResult (the JSON.parse({{.MatchResultJson}})
at the top) and instead assign rawJson = {{.MatchResultJson}} only, then rely on
the existing guarded parsing block that checks rawJson, uses try/catch and sets
the default matchResult; update references to matchResult so the guarded branch
initializes it (symbols: matchResult, rawJson, JSON.parse, the try/catch block
and the $(document).ready handler).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
field/team_sign.go (1)
53-53:⚠️ Potential issue | 🟡 Minor
teamSignYearis still set to 2025 for a 2026 game update.This constant is displayed on the physical team signs (Lines 254, 291). It should be updated to
2026to match the game season.- teamSignYear = 2025 + teamSignYear = 2026
🧹 Nitpick comments (1)
field/team_sign.go (1)
204-204: Nit: Remove the change-log comment.
// Removed BargePoints logicreads as a commit note rather than code documentation. The git history captures what was removed; inline comments should describe what the code does now, not what it used to do.
…lity, including fixing scoring logic, adding locking to prevent data races, and adjusting test cases to reflect the latest rules of the game.
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
field/arena_test.go (1)
250-262:⚠️ Potential issue | 🟡 MinorManually set
HubActive = falseis silently overwritten byupdateHubStatus()insidehandlePlcInputOutput().
handlePlcInputOutput()callsupdateHubStatus()under theScoreMulock, which resetsHubActivebased on match time. WithMatchStartTimeat its zero value andMatchState = TeleopPeriod,MatchTimeSec()returns a very large number, makingtimeLeftnegative — so the shift logic is skipped and both hubs default toactive = true. The manualHubActive = falsesetup at line 252 is a no-op.The test still passes (Blue PLC count is zero), but the Blue
HubActive = falseprecondition is never actually exercised. Consider using time-based simulation (as done inTestPlcFuelScoring_InactiveHub) to reliably test hub-inactive behaviour.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@field/arena_test.go` around lines 250 - 262, The test is silently overwritten because updateHubStatus() (called inside handlePlcInputOutput() under ScoreMu) recalculates HubActive based on MatchTimeSec() from MatchStartTime/MatchState; to fix, make the test drive match time like TestPlcFuelScoring_InactiveHub instead of manually flipping HubActive: set MatchStartTime and MatchState so MatchTimeSec() returns the intended elapsed time (or use the same time-based helper used in TestPlcFuelScoring_InactiveHub) before calling handlePlcInputOutput(), then assert Blue.HubActive is false; reference handlePlcInputOutput, updateHubStatus, HubActive, MatchStartTime, MatchState, and MatchTimeSec when locating the code to change.
🧹 Nitpick comments (6)
plc/fake_modbus_client_test.go (1)
28-29: Optional: add bounds guard before slicing to prevent panic on oversizedquantity.
client.inputs[0:quantity],client.registers[0:quantity](Line 55), and the coil loop inWriteMultipleCoils(Lines 41–43) will all panic at runtime ifquantity > 64. This was a latent issue at size 32 too, so it isn't new, and test callers currently stay within bounds — but a cheap guard would make the fake more robust against accidental future misuse.🛡️ Proposed guard for
ReadDiscreteInputs(apply similarly to the other two sites)func (client *FakeModbusClient) ReadDiscreteInputs(address, quantity uint16) (results []byte, err error) { if address != 0 { return nil, errors.New("unexpected address") } + if int(quantity) > len(client.inputs) { + return nil, errors.New("quantity exceeds input array size") + } if client.returnError { return nil, errors.New("dummy error") } inputsToRead := client.inputs[0:quantity] return boolToByte(inputsToRead), nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plc/fake_modbus_client_test.go` around lines 28 - 29, The slicing operations in ReadDiscreteInputs (client.inputs[0:quantity]), the ReadHoldingRegisters/ReadInputRegisters use of client.registers[0:quantity], and the coil loop in WriteMultipleCoils can panic when quantity > slice length; add a bounds guard in each of these functions (e.g., in ReadDiscreteInputs, ReadHoldingRegisters/ReadInputRegisters, and WriteMultipleCoils) that checks if quantity is larger than len(client.inputs) or len(client.registers) and either return a clear error (preferred) or clamp quantity to the available length before slicing/looping to prevent runtime panics.field/arena.go (2)
1302-1303: Remove the commented-outarena.ScoreMu.Unlock()— dead code artifact.The
defer arena.ScoreMu.Unlock()at line 1251 already handles the unlock. The commented-out manual call at line 1303 is a leftover that misleads readers into thinking an explicit unlock is still needed here.♻️ Suggested fix
- // Release lock after all score modifications and notifications are triggered. - //arena.ScoreMu.Unlock() -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@field/arena.go` around lines 1302 - 1303, Remove the dead commented-out unlock call — the leftover "//arena.ScoreMu.Unlock()" should be deleted because the mutex is already released by the earlier defer arena.ScoreMu.Unlock() in the same function; locate the commented line referencing arena.ScoreMu.Unlock() and simply remove that comment so readers aren't misled about needing an explicit unlock.
848-857:isOddShiftistruefor even-numbered shift indices — misleading name and incorrect comment.
shiftIndex%2 == 0istruewhenshiftIndexis 0, 2, 4 (mathematically even indices). Naming itisOddShiftand commenting "Index 0 & 2 are Odd" is the opposite of the mathematical truth, making the XOR logic below it confusing to reason about.♻️ Suggested rename
- // Logic toggle: Index 0 & 2 are Odd, Index 1 & 3 are Even. - isOddShift := (shiftIndex%2 == 0) + // Even shift indices (0, 2, 4…) → loser alliance is active first. + isEvenShiftIndex := (shiftIndex % 2 == 0) - redActive = redWinsAuto != isOddShift + redActive = redWinsAuto != isEvenShiftIndex🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@field/arena.go` around lines 848 - 857, The variable isOddShift and its comment are inverted: change isOddShift to isEvenShift (or invert the boolean) where shiftIndex is computed (shiftIndex := int((startTime - timeLeft) / shiftDuration)), update the comment to correctly state "Index 0, 2, 4 are even", and then adjust the XOR logic that sets redActive and blueActive to use isEvenShift (or flip the comparison) so the intent remains the same (e.g., redActive = redWinsAuto != isEvenShift) while keeping redActive, blueActive, shiftIndex, startTime, timeLeft, and shiftDuration names intact.game/score.go (2)
43-54:AutoFuelPointsassignment multiplies by 1 — redundant.
score.AutoFuelCount * 1is a no-op; assign the value directly.♻️ Suggested fix
- summary.AutoFuelPoints = score.AutoFuelCount * 1 + summary.AutoFuelPoints = score.AutoFuelCount🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@game/score.go` around lines 43 - 54, Replace the redundant multiplication by 1 when setting the autonomous fuel points: change the assignment to set summary.AutoFuelPoints directly from score.AutoFuelCount (i.e., remove the "* 1"), leaving the rest of the Auto points calculation (the loop over score.AutoTowerLevel1 contributing to summary.AutoTowerPoints and the final summary.AutoPoints aggregation) unchanged.
100-113: Redundant= falseassignments in RP else-branches.
summaryisnew(ScoreSummary), so all bool fields default tofalse. The explicitelse { ... = false }assignments are unnecessary, and the style is inconsistent withTraversalRankingPoint(lines 117-119) which has no else branch.♻️ Suggested cleanup
if totalFuel >= EnergizedFuelThreshold { summary.EnergizedRankingPoint = true - } else { - summary.EnergizedRankingPoint = false } if totalFuel >= SuperchargedFuelThreshold { summary.SuperchargedRankingPoint = true - } else { - summary.SuperchargedRankingPoint = false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@game/score.go` around lines 100 - 113, Remove the redundant else-branches that assign false to summary boolean fields: when computing totalFuel and setting summary.EnergizedRankingPoint and summary.SuperchargedRankingPoint, delete the else { ... = false } blocks and only set the field to true when the threshold is met (leave the default false otherwise) so behavior matches the pattern used by TraversalRankingPoint; update the code around totalFuel, EnergizedFuelThreshold, SuperchargedFuelThreshold, summary.EnergizedRankingPoint and summary.SuperchargedRankingPoint accordingly.field/arena_test.go (1)
272-274:TestPlcFieldEStopis an empty placeholder that tests nothing.Remove it or implement the intended field E-stop coverage. An empty test silently passes and adds false confidence in coverage reports.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@field/arena_test.go` around lines 272 - 274, TestPlcFieldEStop is an empty test that gives false confidence; locate the TestPlcFieldEStop function and either remove it or implement real assertions: set up the minimal PlcField instance or fixture the test suite uses, invoke the E-stop behavior (call the method/function that triggers field E-stop on the PlcField instance), and assert expected outcomes (e.g., error return, changed state/flag, emitted event or side-effect) and any cleanup; ensure you use the existing constructors/helpers used elsewhere in arena_test.go so the test compiles and verifies actual E-stop behavior instead of being a no-op.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@arduino/driverStation.py`:
- Line 75: The Modbus server is currently bound to all interfaces via
StartTcpServer(..., address=("0.0.0.0", 502)); change this to bind to a specific
interface used by the FRC field network (e.g., replace "0.0.0.0" with a
configured FIELD_NETWORK_IP or a constant like FIELD_INTERFACE_IP) or read the
bind address from configuration/environment so the server only listens on the
intended interface rather than 0.0.0.0.
- Around line 46-48: The code currently writes button state into Modbus coils by
calling store.setValues(1, i, [val]) after computing val = 1 if raw_val == 0
else 0; change this to write into discrete inputs (FC=2) instead of coils so
sensor readings are read-only from the network—i.e., replace the store.setValues
call to use function code 2 (store.setValues(2, i, [val])) for the same index i
and value.
- Line 77: Remove the stray undefined expression "EOF" in driverStation.py (it
appears after GPIO.cleanup()), which causes NameError on clean shutdown; locate
the EOF near the shutdown path used by StartTcpServer/cleanup logic and delete
it (or replace it with a no-op like "return" if needed to make control flow
explicit) so GPIO.cleanup() can complete without raising an exception.
- Line 6: The import of StartTcpServer is using the removed pymodbus.server.sync
path; replace "from pymodbus.server.sync import StartTcpServer" with "from
pymodbus.server import StartTcpServer" (or the equivalent direct import exposed
by pymodbus 3.x) in arduino/driverStation.py and make the same change in
modbus_plc.py so the StartTcpServer symbol is imported from pymodbus.server to
avoid ModuleNotFoundError with pymodbus ≥ 3.0.
In `@arduino/modbus_plc.py`:
- Around line 55-62: The loop handling OUTPUT_PINS in the block that reads
target_states currently inverts Modbus coil logic (treats 0 as ON); change the
conditional in that loop so a Modbus coil value of 1 drives GPIO.output(pin,
GPIO.HIGH) and appends "ON" to output_status, and any non-1 value drives
GPIO.output(pin, GPIO.LOW) and appends "OFF". Update the code that uses
OUTPUT_PINS, target_states, GPIO.output and output_status accordingly; if the
hardware is truly active-low, document that in comments rather than keeping
inverted logic in the loop.
- Line 5: The import of StartTcpServer is using the removed submodule path
`pymodbus.server.sync`; update the import to use the new location
`StartTcpServer` from `pymodbus.server` (same change applied as in
driverStation.py) so the module loads correctly on pymodbus 3.x; search for the
symbol StartTcpServer in arduino/modbus_plc.py and replace the old import source
accordingly.
In `@field/arena.go`:
- Around line 775-778: Remove the duplicate unprotected calls to
updateHubStatus() and updateGameSpecificMessage() from Update() so those writes
to CurrentScore.HubActive and GameSpecificMessage only occur once under lock;
ensure the single protected invocation remains in handlePlcInputOutput() inside
the arena.ScoreMu.Lock()/Unlock() region (keep the calls where you currently
have them there). Also handle the Plc.IsEnabled() == false path by adding an
explicit guarded block before the early return in handlePlcInputOutput() that
acquires ScoreMu and calls updateHubStatus() and updateGameSpecificMessage() so
the updates are always performed under ScoreMu even when the PLC is disabled.
- Around line 130-131: Remove the deprecated rand.Seed call: delete the
rand.Seed(time.Now().UnixNano()) invocation (symbol: rand.Seed and the use of
time.Now) since Go auto-seeds the global source; keep the "math/rand" import
because rand.Intn is used elsewhere (symbol: rand.Intn), and also remove the
"time" import if it becomes unused after deleting the seed call.
In `@field/team_sign_test.go`:
- Line 18: Remove the hard-coded defer os.Remove("test.db") in team_sign_test.go
and instead rely on the test helper or t.Cleanup for teardown: locate the test
that calls setupTestArena(t) and either let setupTestArena register cleanup (if
it already does) or create a unique DB path using t.TempDir() and register
t.Cleanup(func(){ os.Remove(dbPath) }) so you don’t double-delete or remove the
wrong file; update both occurrences that currently call os.Remove("test.db") to
use the safer pattern tied to the test’s lifecycle (refer to setupTestArena and
the test functions in team_sign_test.go).
- Around line 27-39: The inline comment block showing expected scores is stale
and contradicts the assertions for generateInMatchTeamRearText; update the
comments to match the asserted expected outputs (or change the assertions if
those are wrong). Specifically, replace the R095/B130 totals and their breakdown
with the actual expected totals used in the assertions (e.g., expect "R105-B220
25" for red perspective and "B220-R105 50" for blue perspective) so the comment
block accurately documents the assertions around generateInMatchTeamRearText.
In `@game/score.go`:
- Around line 60-71: The comment above the loop over score.EndgameStatuses is
inaccurate: it only mentions Level 2 and Level 3 but the switch also handles
EndgameLevel1 (10 pts). Update the comment to list all three levels and their
points (Level 1 = 10 pts, Level 2 = 20 pts, Level 3 = 30 pts) so it matches the
switch handling (EndgameLevel1, EndgameLevel2, EndgameLevel3) and the fact that
summary.EndgameTowerPoints is incremented accordingly.
- Around line 82-88: The current loop in game/score.go adds 30 to
summary.EndgameTowerPoints for every opponent foul with rule.RuleNumber ==
"G420", causing unbounded stacking; change the handling so the G420 compensation
is applied at most once per match: when encountering G420, either set a one-time
flag (e.g., endgameProtectionApplied) or set summary.EndgameTowerPoints =
max(summary.EndgameTowerPoints, 30) instead of +=, so subsequent G420
occurrences do not add additional points. Ensure the fix references the existing
rule.RuleNumber check and summary.EndgameTowerPoints update.
---
Outside diff comments:
In `@field/arena_test.go`:
- Around line 250-262: The test is silently overwritten because
updateHubStatus() (called inside handlePlcInputOutput() under ScoreMu)
recalculates HubActive based on MatchTimeSec() from MatchStartTime/MatchState;
to fix, make the test drive match time like TestPlcFuelScoring_InactiveHub
instead of manually flipping HubActive: set MatchStartTime and MatchState so
MatchTimeSec() returns the intended elapsed time (or use the same time-based
helper used in TestPlcFuelScoring_InactiveHub) before calling
handlePlcInputOutput(), then assert Blue.HubActive is false; reference
handlePlcInputOutput, updateHubStatus, HubActive, MatchStartTime, MatchState,
and MatchTimeSec when locating the code to change.
---
Duplicate comments:
In `@arduino/modbus_plc.py`:
- Line 88: There is a stray undefined identifier EOF in arduino/modbus_plc.py
that will raise NameError during shutdown after GPIO.cleanup(); remove the bare
EOF token (or, if it was intended as a literal, replace it with a proper quoted
string) so the shutdown/cleanup code that calls GPIO.cleanup() no longer
references an undefined name; search for the EOF token near the GPIO.cleanup()
or shutdown handler and delete or properly quote it to fix the Ruff F821 error.
---
Nitpick comments:
In `@field/arena_test.go`:
- Around line 272-274: TestPlcFieldEStop is an empty test that gives false
confidence; locate the TestPlcFieldEStop function and either remove it or
implement real assertions: set up the minimal PlcField instance or fixture the
test suite uses, invoke the E-stop behavior (call the method/function that
triggers field E-stop on the PlcField instance), and assert expected outcomes
(e.g., error return, changed state/flag, emitted event or side-effect) and any
cleanup; ensure you use the existing constructors/helpers used elsewhere in
arena_test.go so the test compiles and verifies actual E-stop behavior instead
of being a no-op.
In `@field/arena.go`:
- Around line 1302-1303: Remove the dead commented-out unlock call — the
leftover "//arena.ScoreMu.Unlock()" should be deleted because the mutex is
already released by the earlier defer arena.ScoreMu.Unlock() in the same
function; locate the commented line referencing arena.ScoreMu.Unlock() and
simply remove that comment so readers aren't misled about needing an explicit
unlock.
- Around line 848-857: The variable isOddShift and its comment are inverted:
change isOddShift to isEvenShift (or invert the boolean) where shiftIndex is
computed (shiftIndex := int((startTime - timeLeft) / shiftDuration)), update the
comment to correctly state "Index 0, 2, 4 are even", and then adjust the XOR
logic that sets redActive and blueActive to use isEvenShift (or flip the
comparison) so the intent remains the same (e.g., redActive = redWinsAuto !=
isEvenShift) while keeping redActive, blueActive, shiftIndex, startTime,
timeLeft, and shiftDuration names intact.
In `@game/score.go`:
- Around line 43-54: Replace the redundant multiplication by 1 when setting the
autonomous fuel points: change the assignment to set summary.AutoFuelPoints
directly from score.AutoFuelCount (i.e., remove the "* 1"), leaving the rest of
the Auto points calculation (the loop over score.AutoTowerLevel1 contributing to
summary.AutoTowerPoints and the final summary.AutoPoints aggregation) unchanged.
- Around line 100-113: Remove the redundant else-branches that assign false to
summary boolean fields: when computing totalFuel and setting
summary.EnergizedRankingPoint and summary.SuperchargedRankingPoint, delete the
else { ... = false } blocks and only set the field to true when the threshold is
met (leave the default false otherwise) so behavior matches the pattern used by
TraversalRankingPoint; update the code around totalFuel, EnergizedFuelThreshold,
SuperchargedFuelThreshold, summary.EnergizedRankingPoint and
summary.SuperchargedRankingPoint accordingly.
In `@plc/fake_modbus_client_test.go`:
- Around line 28-29: The slicing operations in ReadDiscreteInputs
(client.inputs[0:quantity]), the ReadHoldingRegisters/ReadInputRegisters use of
client.registers[0:quantity], and the coil loop in WriteMultipleCoils can panic
when quantity > slice length; add a bounds guard in each of these functions
(e.g., in ReadDiscreteInputs, ReadHoldingRegisters/ReadInputRegisters, and
WriteMultipleCoils) that checks if quantity is larger than len(client.inputs) or
len(client.registers) and either return a clear error (preferred) or clamp
quantity to the available length before slicing/looping to prevent runtime
panics.
| import time | ||
| import threading | ||
| import Jetson.GPIO as GPIO | ||
| from pymodbus.server.sync import StartTcpServer |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for requirements files and pymodbus version pins
find . -type f \( -name "requirements*.txt" -o -name "setup.py" -o -name "setup.cfg" -o -name "pyproject.toml" -o -name "Pipfile" -o -name "poetry.lock" \) | head -20Repository: Team254/cheesy-arena
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Search for pymodbus version specifications
rg "pymodbus" --type-add "config:*.{txt,cfg,toml,py}" -t config -B1 -A1Repository: Team254/cheesy-arena
Length of output: 622
🏁 Script executed:
#!/bin/bash
# Check for other pymodbus imports in the codebase to see version compatibility context
rg "from pymodbus" --type py -A1 -B1Repository: Team254/cheesy-arena
Length of output: 622
🏁 Script executed:
#!/bin/bash
# List all files in the repo root and arduino directory to find dependency files
ls -la | head -30
echo "---"
ls -la arduino/ | head -30Repository: Team254/cheesy-arena
Length of output: 2205
🏁 Script executed:
#!/bin/bash
# Search for any file mentioning version constraints
rg "pymodbus" --type-add "all:*" -i -B2 -A2 | head -100Repository: Team254/cheesy-arena
Length of output: 123
🌐 Web query:
pymodbus 3.x import StartTcpServer API change sync submodule
💡 Result:
In pymodbus 3.x, the old “split” import paths (notably ...sync and ...asynchronous) were removed/reshuffled. Use the top-level server (and client) packages instead.
✅ Correct imports (pymodbus 3.x)
Sync-style (blocking) helper:
from pymodbus.server import StartTcpServerPyModbus 3.x documents pymodbus.server.StartTcpServer(...) as the supported entry point. [1]
Asyncio server (if you want non-blocking / async app integration):
Use the asyncio server APIs documented under the 3.x server docs (all servers are implemented with asyncio; the “sync” server is an interface layer). [1]
What broke (common old imports)
These were used in older versions and commonly fail on 3.x:
from pymodbus.server.asynchronous import StartTcpServer(Twisted-era / older async layout; removed around 3.0 development) [2]from pymodbus.client.sync import ModbusTcpClient(theclient.syncsubmodule no longer exists; import frompymodbus.client) [3]from pymodbus.server.sync import StartTcpServer(seen in older docs/branches; not the current 3.x public import path) [4]
Replacement pattern (client side)
from pymodbus.client import ModbusTcpClient # or ModbusSerialClient, etc.This is the documented replacement for pymodbus.client.sync. [3]
Sources:
[1] PyModbus 3.x server docs (pymodbus.server.StartTcpServer) (pymodbus.readthedocs.io)
[2] Discussion of removed pymodbus.server.asynchronous imports / Twisted removal (stackoverflow.com)
[3] pymodbus.client.sync no longer exists; use pymodbus.client (python-forum.io)
[4] Older docs showing pymodbus.server.sync import path (pymodbus-n.readthedocs.io)
Citations:
- 1: https://pymodbus.readthedocs.io/en/stable/source/server.html?utm_source=openai
- 2: https://stackoverflow.com/questions/75398565/pymodbus-package-import-error-modulenotfound?utm_source=openai
- 3: https://python-forum.io/archive/index.php/thread-38873.html?utm_source=openai
- 4: https://pymodbus-n.readthedocs.io/en/3.0.0/source/example/synchronous_server.html?utm_source=openai
pymodbus.server.sync was removed in pymodbus 3.x — this import will fail.
from pymodbus.server.sync import StartTcpServer is incompatible with pymodbus ≥ 3.0. The sync submodule was removed; StartTcpServer is now exposed directly from pymodbus.server. This will throw ModuleNotFoundError at startup if pymodbus 3.x is installed.
Proposed fix
-from pymodbus.server.sync import StartTcpServer
+from pymodbus.server import StartTcpServerNote: modbus_plc.py has the same import issue and needs the same fix.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from pymodbus.server.sync import StartTcpServer | |
| from pymodbus.server import StartTcpServer |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@arduino/driverStation.py` at line 6, The import of StartTcpServer is using
the removed pymodbus.server.sync path; replace "from pymodbus.server.sync import
StartTcpServer" with "from pymodbus.server import StartTcpServer" (or the
equivalent direct import exposed by pymodbus 3.x) in arduino/driverStation.py
and make the same change in modbus_plc.py so the StartTcpServer symbol is
imported from pymodbus.server to avoid ModuleNotFoundError with pymodbus ≥ 3.0.
| val = 1 if raw_val == 0 else 0 | ||
|
|
||
| store.setValues(1, i, [val]) |
There was a problem hiding this comment.
Button state written to coils (FC=1) instead of discrete inputs (FC=2).
store.setValues(1, i, [val]) uses function code 1 (coils), which is writable by any remote Modbus client. Sensor/button readings should use FC=2 (discrete inputs) which is read-only from the network, preventing any external client from inadvertently overwriting live GPIO readings.
🛠️ Proposed fix
- store.setValues(1, i, [val])
+ store.setValues(2, i, [val])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val = 1 if raw_val == 0 else 0 | |
| store.setValues(1, i, [val]) | |
| val = 1 if raw_val == 0 else 0 | |
| store.setValues(2, i, [val]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@arduino/driverStation.py` around lines 46 - 48, The code currently writes
button state into Modbus coils by calling store.setValues(1, i, [val]) after
computing val = 1 if raw_val == 0 else 0; change this to write into discrete
inputs (FC=2) instead of coils so sensor readings are read-only from the
network—i.e., replace the store.setValues call to use function code 2
(store.setValues(2, i, [val])) for the same index i and value.
| t.daemon = True | ||
| t.start() | ||
|
|
||
| StartTcpServer(context, address=("0.0.0.0", 502)) |
There was a problem hiding this comment.
Modbus server bound to all interfaces (0.0.0.0:502).
If the Jetson has multiple network interfaces, this exposes the server beyond the intended FRC field network. Bind to a specific interface address if only the field network should have access.
🧰 Tools
🪛 Ruff (0.15.1)
[error] 75-75: Possible binding to all interfaces
(S104)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@arduino/driverStation.py` at line 75, The Modbus server is currently bound to
all interfaces via StartTcpServer(..., address=("0.0.0.0", 502)); change this to
bind to a specific interface used by the FRC field network (e.g., replace
"0.0.0.0" with a configured FIELD_NETWORK_IP or a constant like
FIELD_INTERFACE_IP) or read the bind address from configuration/environment so
the server only listens on the intended interface rather than 0.0.0.0.
|
|
||
| StartTcpServer(context, address=("0.0.0.0", 502)) | ||
| GPIO.cleanup() | ||
| EOF No newline at end of file |
There was a problem hiding this comment.
EOF is still present — the fix from commit 50da2b3 was incomplete.
The prior review noted this exact line and was marked resolved, but EOF remains. It's an undefined name (Ruff F821) and a useless expression (Ruff B018). While StartTcpServer blocks and this line is rarely reached, any clean server shutdown will execute GPIO.cleanup() and then evaluate EOF, raising a NameError instead of exiting cleanly.
🛠️ Proposed fix
StartTcpServer(context, address=("0.0.0.0", 502))
GPIO.cleanup()
-EOF📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| EOF |
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 77-77: Found useless expression. Either assign it to a variable or remove it.
(B018)
[error] 77-77: Undefined name EOF
(F821)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@arduino/driverStation.py` at line 77, Remove the stray undefined expression
"EOF" in driverStation.py (it appears after GPIO.cleanup()), which causes
NameError on clean shutdown; locate the EOF near the shutdown path used by
StartTcpServer/cleanup logic and delete it (or replace it with a no-op like
"return" if needed to make control flow explicit) so GPIO.cleanup() can complete
without raising an exception.
| import time | ||
| import threading | ||
| import Jetson.GPIO as GPIO | ||
| from pymodbus.server.sync import StartTcpServer |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and examine the import
find . -name "modbus_plc.py" -type f 2>/dev/null | head -20Repository: Team254/cheesy-arena
Length of output: 87
🏁 Script executed:
# Search for pymodbus version constraints in the project
find . -name "requirements*.txt" -o -name "setup.py" -o -name "pyproject.toml" -o -name "Pipfile" 2>/dev/null | head -20Repository: Team254/cheesy-arena
Length of output: 46
🏁 Script executed:
# Check for any driverStation.py mentioned in the review to understand the context
find . -name "driverStation.py" -type f 2>/dev/nullRepository: Team254/cheesy-arena
Length of output: 90
🏁 Script executed:
# Read the modbus_plc.py file to see the actual import
cat -n ./arduino/modbus_plc.py | head -30Repository: Team254/cheesy-arena
Length of output: 1181
🏁 Script executed:
# Read driverStation.py to check if it has the same import
cat -n ./arduino/driverStation.py | head -30Repository: Team254/cheesy-arena
Length of output: 1112
🏁 Script executed:
# Search for pymodbus version mentions anywhere in the project
rg "pymodbus" -i --type python | head -20Repository: Team254/cheesy-arena
Length of output: 94
🏁 Script executed:
# Search for all pymodbus references without type filter
rg "pymodbus" | head -30Repository: Team254/cheesy-arena
Length of output: 449
🌐 Web query:
pymodbus 3.0 StartTcpServer import path documentation
💡 Result:
In pymodbus 3.0.x, StartTcpServer is exported from the top-level server module:
from pymodbus.server import StartTcpServerThe asyncio variant is:
from pymodbus.server import StartAsyncTcpServerThese functions are documented under the pymodbus.server “Server” API (where pymodbus.server.StartTcpServer and pymodbus.server.StartAsyncTcpServer are listed). [1]
If you’re upgrading from older examples that use paths like pymodbus.server.sync, pymodbus.server.async, or pymodbus.server.asynchronous (Twisted-era), those are from older versions and will not work in 3.x. [2]
Sources:
[1] PyModbus Server API docs (pymodbus.server.StartTcpServer, StartAsyncTcpServer) https://pymodbus.readthedocs.io/en/v3.8.0/source/server.html
[2] Discussion of removed older async/Twisted imports in 3.0+ (example of import errors) https://stackoverflow.com/questions/75398565/pymodbus-package-import-error-modulenotfound
Same broken pymodbus.server.sync import as driverStation.py.
In pymodbus 3.x, StartTcpServer is at pymodbus.server, not pymodbus.server.sync. The .sync submodule no longer exists, so this import fails at startup on pymodbus ≥ 3.0.
🛠️ Proposed fix
-from pymodbus.server.sync import StartTcpServer
+from pymodbus.server import StartTcpServer📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from pymodbus.server.sync import StartTcpServer | |
| from pymodbus.server import StartTcpServer |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@arduino/modbus_plc.py` at line 5, The import of StartTcpServer is using the
removed submodule path `pymodbus.server.sync`; update the import to use the new
location `StartTcpServer` from `pymodbus.server` (same change applied as in
driverStation.py) so the module loads correctly on pymodbus 3.x; search for the
symbol StartTcpServer in arduino/modbus_plc.py and replace the old import source
accordingly.
| // 2026 REBUILT: Update Hub Status and Game Data | ||
| arena.updateHubStatus() | ||
| arena.updateGameSpecificMessage() | ||
|
|
There was a problem hiding this comment.
updateHubStatus and updateGameSpecificMessage are called unprotected in Update() and again under ScoreMu in handlePlcInputOutput() — data race + redundant work.
Two distinct problems arise from this double invocation:
-
Data race: The calls at lines 775-777 write to
CurrentScore.HubActive(andGameSpecificMessage) from the arena loop goroutine without holdingScoreMu. Concurrent websocket handler goroutines that read or mutateCurrentScorecan race against these unprotected writes. TheScoreMulock added tohandlePlcInputOutput()only covers the second invocation. -
Unprotected path when PLC is disabled: When
arena.Plc.IsEnabled()returnsfalse,handlePlcInputOutput()returns immediately at line 1187 — the protected calls at lines 1252-1254 are never reached. Only the unprotected calls inUpdate()execute, leavingHubActiveandGameSpecificMessagewritten without any lock. -
Redundant work: Both call sites execute on every arena loop tick; the first call's results are immediately overwritten by the second.
The fix is to remove the duplicate calls from Update() and ensure that the hub-status / game-message update always runs under ScoreMu, with explicit handling for the PLC-disabled case if needed.
🔒 Suggested fix
- // 2026 REBUILT: Update Hub Status and Game Data
- arena.updateHubStatus()
- arena.updateGameSpecificMessage()
-
// Send a match tick notification if passing an integer second threshold...And in handlePlcInputOutput(), keep the protected calls as-is:
arena.ScoreMu.Lock()
defer arena.ScoreMu.Unlock()
arena.updateHubStatus() // single, protected invocation
arena.updateGameSpecificMessage() // single, protected invocationFor the PLC-disabled path, add an explicit block before the early return:
func (arena *Arena) handlePlcInputOutput() {
+ arena.ScoreMu.Lock()
+ arena.updateHubStatus()
+ arena.updateGameSpecificMessage()
+ arena.ScoreMu.Unlock()
+
if !arena.Plc.IsEnabled() {
return
}Also applies to: 1249-1254
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@field/arena.go` around lines 775 - 778, Remove the duplicate unprotected
calls to updateHubStatus() and updateGameSpecificMessage() from Update() so
those writes to CurrentScore.HubActive and GameSpecificMessage only occur once
under lock; ensure the single protected invocation remains in
handlePlcInputOutput() inside the arena.ScoreMu.Lock()/Unlock() region (keep the
calls where you currently have them there). Also handle the Plc.IsEnabled() ==
false path by adding an explicit guarded block before the early return in
handlePlcInputOutput() that acquires ScoreMu and calls updateHubStatus() and
updateGameSpecificMessage() so the updates are always performed under ScoreMu
even when the PLC is disabled.
|
|
||
| func TestTeamSign_GenerateInMatchRearText(t *testing.T) { | ||
| arena := setupTestArena(t) | ||
| defer os.Remove("test.db") |
There was a problem hiding this comment.
Hardcoded "test.db" cleanup is fragile.
os.Remove("test.db") hard-codes the temporary database path. If setupTestArena(t) uses a different path or already registers a t.Cleanup for the database, this removes the wrong file (silently no-ops) or double-deletes. Prefer relying on the cleanup registered by the test helper, or use t.TempDir() for isolation.
Also applies to: 44-44
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@field/team_sign_test.go` at line 18, Remove the hard-coded defer
os.Remove("test.db") in team_sign_test.go and instead rely on the test helper or
t.Cleanup for teardown: locate the test that calls setupTestArena(t) and either
let setupTestArena register cleanup (if it already does) or create a unique DB
path using t.TempDir() and register t.Cleanup(func(){ os.Remove(dbPath) }) so
you don’t double-delete or remove the wrong file; update both occurrences that
currently call os.Remove("test.db") to use the safer pattern tied to the test’s
lifecycle (refer to setupTestArena and the test functions in team_sign_test.go).
| // 預期分數計算: | ||
| // Red Score: 95 (Auto:25 + Teleop:20 + End:50) | ||
| // Blue Score: 130 (Auto:20 + Teleop:40 + End:70) | ||
| // Red Fuel: 25 | ||
| // Blue Fuel: 50 | ||
|
|
||
| // Check timeout mode. | ||
| arena.FieldReset = true | ||
| arena.MatchState = TimeoutActive | ||
| assertSign("23:45", whiteColor, "Field Break: 23:45") | ||
| // 測試紅隊視角: "倒數時間 R分數-B分數 R球數" | ||
| // R095-B130 25 | ||
| assert.Equal(t, "01:23 R105-B220 25", generateInMatchTeamRearText(arena, true, "01:23")) | ||
|
|
||
| // Check blank mode. | ||
| arena.AllianceStationDisplayMode = "blank" | ||
| assertSign(" ", whiteColor, "") | ||
|
|
||
| // Check alliance selection. | ||
| arena.AllianceStationDisplayMode = "logo" | ||
| arena.AudienceDisplayMode = "allianceSelection" | ||
| arena.AllianceSelectionShowTimer = false | ||
| assertSign(" ", whiteColor, "") | ||
| arena.AllianceSelectionShowTimer = true | ||
| assertSign("23:45", whiteColor, "") | ||
| arena.AllianceStationDisplayMode = "blank" | ||
| assertSign(" ", whiteColor, "") | ||
| // 測試藍隊視角: "倒數時間 B分數-R分數 B球數" | ||
| // B130-R095 50 | ||
| assert.Equal(t, "01:23 B220-R105 50", generateInMatchTeamRearText(arena, false, "01:23")) |
There was a problem hiding this comment.
Stale score comments contradict the actual assertions.
Lines 28-31 document expected totals of R095 and B130, but the assertions at lines 35 and 39 check "R105-B220". Either the expected values or the comments are wrong, which makes the test misleading.
Update the comments to reflect the actual expected values (or correct the assertion if the comments capture the intended behavior):
- // Red Score: 95 (Auto:25 + Teleop:20 + End:50)
- // Blue Score: 130 (Auto:20 + Teleop:40 + End:70)
- // Red Fuel: 25
- // Blue Fuel: 50
- // 測試紅隊視角: "倒數時間 R分數-B分數 R球數"
- // R095-B130 25
+ // Red Score: 105 | Blue Score: 220
+ // Red Fuel: 25 | Blue Fuel: 50🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@field/team_sign_test.go` around lines 27 - 39, The inline comment block
showing expected scores is stale and contradicts the assertions for
generateInMatchTeamRearText; update the comments to match the asserted expected
outputs (or change the assertions if those are wrong). Specifically, replace the
R095/B130 totals and their breakdown with the actual expected totals used in the
assertions (e.g., expect "R105-B220 25" for red perspective and "B220-R105 50"
for blue perspective) so the comment block accurately documents the assertions
around generateInMatchTeamRearText.
| // Endgame Tower: Level 2 (20pts), Level 3 (30pts) | ||
| for _, status := range score.EndgameStatuses { | ||
| switch status { | ||
| case EndgameParked: | ||
| summary.BargePoints += 2 | ||
| case EndgameShallowCage: | ||
| summary.BargePoints += 6 | ||
| case EndgameDeepCage: | ||
| summary.BargePoints += 12 | ||
| case EndgameLevel1: | ||
| summary.EndgameTowerPoints += 10 | ||
| case EndgameLevel2: | ||
| summary.EndgameTowerPoints += 20 | ||
| case EndgameLevel3: | ||
| summary.EndgameTowerPoints += 30 | ||
| default: | ||
| } | ||
| } |
There was a problem hiding this comment.
Incomplete comment omits Level 1 from endgame tower point description.
The comment on line 60 only lists Level 2 and Level 3, but EndgameLevel1 (10 pts) is also handled in the switch below.
📝 Suggested fix
- // Endgame Tower: Level 2 (20pts), Level 3 (30pts)
+ // Endgame Tower: Level 1 (10pts), Level 2 (20pts), Level 3 (30pts)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Endgame Tower: Level 2 (20pts), Level 3 (30pts) | |
| for _, status := range score.EndgameStatuses { | |
| switch status { | |
| case EndgameParked: | |
| summary.BargePoints += 2 | |
| case EndgameShallowCage: | |
| summary.BargePoints += 6 | |
| case EndgameDeepCage: | |
| summary.BargePoints += 12 | |
| case EndgameLevel1: | |
| summary.EndgameTowerPoints += 10 | |
| case EndgameLevel2: | |
| summary.EndgameTowerPoints += 20 | |
| case EndgameLevel3: | |
| summary.EndgameTowerPoints += 30 | |
| default: | |
| } | |
| } | |
| // Endgame Tower: Level 1 (10pts), Level 2 (20pts), Level 3 (30pts) | |
| for _, status := range score.EndgameStatuses { | |
| switch status { | |
| case EndgameLevel1: | |
| summary.EndgameTowerPoints += 10 | |
| case EndgameLevel2: | |
| summary.EndgameTowerPoints += 20 | |
| case EndgameLevel3: | |
| summary.EndgameTowerPoints += 30 | |
| default: | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@game/score.go` around lines 60 - 71, The comment above the loop over
score.EndgameStatuses is inaccurate: it only mentions Level 2 and Level 3 but
the switch also handles EndgameLevel1 (10 pts). Update the comment to list all
three levels and their points (Level 1 = 10 pts, Level 2 = 20 pts, Level 3 = 30
pts) so it matches the switch handling (EndgameLevel1, EndgameLevel2,
EndgameLevel3) and the fact that summary.EndgameTowerPoints is incremented
accordingly.
| if rule != nil { | ||
| // Check for the opponent fouls that automatically trigger a ranking point. | ||
| if rule.IsRankingPoint { | ||
| switch rule.RuleNumber { | ||
| case "G410": | ||
| summary.CoralBonusRankingPoint = true | ||
| case "G418": | ||
| summary.BargeBonusRankingPoint = true | ||
| case "G428": | ||
| summary.BargeBonusRankingPoint = true | ||
| } | ||
| // Handle special rule G420 (Endgame Protection) | ||
| // Rule: If the opponent commits G420, our team gets Level 3 Climb points (30 points) | ||
| if rule.RuleNumber == "G420" { | ||
| summary.EndgameTowerPoints += 30 | ||
| } | ||
| } |
There was a problem hiding this comment.
G420 bonus stacks unboundedly — 30 pts added per foul occurrence.
The loop applies EndgameTowerPoints += 30 for every individual G420 foul committed by the opponent. If the opponent accumulates multiple G420 calls, the compensation points multiply accordingly (e.g., 3 fouls × 30 pts = 90 extra tower points) — on top of whatever climb points the alliance already earned. This appears unintentional; the intent seems to be a one-time maximum-level-climb compensation.
🐛 Suggested fix — apply G420 compensation at most once
+ g420Applied := false
for _, foul := range opponentScore.Fouls {
summary.FoulPoints += foul.PointValue()
if foul.IsMajor {
summary.NumOpponentMajorFouls++
}
rule := foul.Rule()
if rule != nil {
- if rule.RuleNumber == "G420" {
- summary.EndgameTowerPoints += 30
+ if rule.RuleNumber == "G420" && !g420Applied {
+ summary.EndgameTowerPoints += 30
+ g420Applied = true
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@game/score.go` around lines 82 - 88, The current loop in game/score.go adds
30 to summary.EndgameTowerPoints for every opponent foul with rule.RuleNumber ==
"G420", causing unbounded stacking; change the handling so the G420 compensation
is applied at most once per match: when encountering G420, either set a one-time
flag (e.g., endgameProtectionApplied) or set summary.EndgameTowerPoints =
max(summary.EndgameTowerPoints, 30) instead of +=, so subsequent G420
occurrences do not add additional points. Ensure the fix references the existing
rule.RuleNumber check and summary.EndgameTowerPoints update.
…s in the conforming states to conform to the latest logic.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
field/driver_station_connection_test.go (1)
31-31:⚠️ Potential issue | 🔴 CriticalUpdate four assertions —
AutoDurationSecchanged from 15 → 20.The constant was updated to 20 in
game/match_timing.go. With the test timing of Auto = 20 s, Pause = 2 s, Teleop = 140 s:
- Line 31 (PreMatch): Change
byte(15)tobyte(20)- Line 122 (AutoPeriod, 4 s elapsed): Change
byte(11)tobyte(16)- Line 130 (TeleopPeriod, 33 s elapsed): Change
byte(124)tobyte(129)(20 + 140 + 2 − 33)- Line 133 (TeleopPeriod, 150 s elapsed): Change
byte(7)tobyte(12)(20 + 140 + 2 − 150)Line 126 is correct as-is (returns
TeleopDurationSec= 140, independent of Auto duration).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@field/driver_station_connection_test.go` at line 31, Update four assertions in the driver station connection test to reflect the new AutoDurationSec = 20: in the PreMatch assertion that checks data[21] replace byte(15) with byte(20); in the AutoPeriod case (4 s elapsed) replace byte(11) with byte(16); in the TeleopPeriod case (33 s elapsed) replace byte(124) with byte(129); and in the TeleopPeriod case (150 s elapsed) replace byte(7) with byte(12). Leave the assertion that returns TeleopDurationSec (byte matching TeleopDurationSec = 140) unchanged; these changes align the test with the updated AutoDurationSec in game/match_timing.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@field/driver_station_connection_test.go`:
- Line 27: Remove the debug print statement fmt.Printf("Full Packet: %x\n",
data) which floods test output; either delete it or replace it with t.Logf("Full
Packet: %x", data) so the packet hex is only shown on test failure, and ensure
the test has access to *testing.T (e.g., use t.Logf inside the test function
that defines data).
---
Outside diff comments:
In `@field/driver_station_connection_test.go`:
- Line 31: Update four assertions in the driver station connection test to
reflect the new AutoDurationSec = 20: in the PreMatch assertion that checks
data[21] replace byte(15) with byte(20); in the AutoPeriod case (4 s elapsed)
replace byte(11) with byte(16); in the TeleopPeriod case (33 s elapsed) replace
byte(124) with byte(129); and in the TeleopPeriod case (150 s elapsed) replace
byte(7) with byte(12). Leave the assertion that returns TeleopDurationSec (byte
matching TeleopDurationSec = 140) unchanged; these changes align the test with
the updated AutoDurationSec in game/match_timing.go.
| defer dsConn.close() | ||
|
|
||
| data := dsConn.encodeControlPacket(arena) | ||
| fmt.Printf("Full Packet: %x\n", data) |
There was a problem hiding this comment.
Remove debug print before merging.
fmt.Printf("Full Packet: %x\n", data) is a debug artifact that will produce noisy output on every test run / CI pipeline.
🗑️ Proposed fix
- fmt.Printf("Full Packet: %x\n", data)If full-packet tracing is desired for ongoing diagnostics, consider using t.Logf instead, which only surfaces output on test failure:
+ t.Logf("Full Packet: %x", data)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fmt.Printf("Full Packet: %x\n", data) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@field/driver_station_connection_test.go` at line 27, Remove the debug print
statement fmt.Printf("Full Packet: %x\n", data) which floods test output; either
delete it or replace it with t.Logf("Full Packet: %x", data) so the packet hex
is only shown on test failure, and ensure the test has access to *testing.T
(e.g., use t.Logf inside the test function that defines data).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
templates/announcer_display_score_posted.html (2)
32-49:⚠️ Potential issue | 🟠 MajorScore breakdown section is now empty — add 2026 scoring rows.
The
<!--/-->block removes all breakdown rows, leaving the "Score"<h4>followed immediately by only Foul Points. Announcers see no meaningful point breakdown. The PR's stated intent is to modify the announcer display to reflect 2026 scoring (AutoFuelPoints, TeleopFuelPoints, TotalFuelPoints, AutoTowerPoints, EndgameTowerPoints, TotalTowerPoints, etc.), but none of those replacement rows have been added here.The commented-out block should be replaced with the new 2026 score category rows, e.g.:
🔧 Suggested replacement (adjust field names to match actual score_summary.go fields)
-<!-- -<div class="row justify-content-center"> - <div class="col-sm-6">Auto Leave Points</div> - <div class="col-sm-4">{{.summary.LeavePoints}}</div> -</div> -<div class="row justify-content-center"> - <div class="col-sm-6">Coral Points</div> - <div class="col-sm-4">{{.summary.CoralPoints}}</div> -</div> -<div class="row justify-content-center"> - <div class="col-sm-6">Algae Points</div> - <div class="col-sm-4">{{.summary.AlgaePoints}}</div> -</div> -<div class="row justify-content-center"> - <div class="col-sm-6">Barge Points</div> - <div class="col-sm-4">{{.summary.BargePoints}}</div> -</div> ---> +<div class="row justify-content-center"> + <div class="col-sm-6">Auto Fuel Points</div> + <div class="col-sm-4">{{.summary.AutoFuelPoints}}</div> +</div> +<div class="row justify-content-center"> + <div class="col-sm-6">Teleop Fuel Points</div> + <div class="col-sm-4">{{.summary.TeleopFuelPoints}}</div> +</div> +<div class="row justify-content-center"> + <div class="col-sm-6">Auto Tower Points</div> + <div class="col-sm-4">{{.summary.AutoTowerPoints}}</div> +</div> +<div class="row justify-content-center"> + <div class="col-sm-6">Endgame Tower Points</div> + <div class="col-sm-4">{{.summary.EndgameTowerPoints}}</div> +</div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/announcer_display_score_posted.html` around lines 32 - 49, Remove the HTML comment block and replace it with the 2026 scoring rows so the announcer shows the new breakdown; add Bootstrap rows using the template fields (e.g., .summary.AutoFuelPoints, .summary.TeleopFuelPoints, .summary.TotalFuelPoints, .summary.AutoTowerPoints, .summary.EndgameTowerPoints, .summary.TotalTowerPoints, plus existing .summary.FoulPoints and any other fields from score_summary.go such as .summary.TotalScore) so each category has a label column and a value column similar to the original pattern (two divs per row: "col-sm-6" label and "col-sm-4" value). Ensure field names match the actual struct in score_summary.go.
54-66:⚠️ Potential issue | 🟠 MajorUpdate template to use 2026 RP field names.
The template references
AutoBonusRankingPoint,CoralBonusRankingPoint, andBargeBonusRankingPoint—2025 field names that no longer exist in theScoreSummarystruct. These should be replaced with the 2026 fields:EnergizedRankingPoint,SuperchargedRankingPoint, andTraversalRankingPoint. Missing fields will silently render as "No" regardless of actual match results.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/announcer_display_score_posted.html` around lines 54 - 66, The template is using old 2025 ScoreSummary fields (AutoBonusRankingPoint, CoralBonusRankingPoint, BargeBonusRankingPoint) which no longer exist; update the three conditional checks in announcer_display_score_posted.html to use the 2026 fields EnergizedRankingPoint, SuperchargedRankingPoint, and TraversalRankingPoint respectively (keep the surrounding divs and the existing if/else rendering logic and the playoffMatch guard unchanged so the display still shows "Yes"/"No" based on the new fields).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@templates/announcer_display_score_posted.html`:
- Around line 32-49: Remove the HTML comment block and replace it with the 2026
scoring rows so the announcer shows the new breakdown; add Bootstrap rows using
the template fields (e.g., .summary.AutoFuelPoints, .summary.TeleopFuelPoints,
.summary.TotalFuelPoints, .summary.AutoTowerPoints, .summary.EndgameTowerPoints,
.summary.TotalTowerPoints, plus existing .summary.FoulPoints and any other
fields from score_summary.go such as .summary.TotalScore) so each category has a
label column and a value column similar to the original pattern (two divs per
row: "col-sm-6" label and "col-sm-4" value). Ensure field names match the actual
struct in score_summary.go.
- Around line 54-66: The template is using old 2025 ScoreSummary fields
(AutoBonusRankingPoint, CoralBonusRankingPoint, BargeBonusRankingPoint) which no
longer exist; update the three conditional checks in
announcer_display_score_posted.html to use the 2026 fields
EnergizedRankingPoint, SuperchargedRankingPoint, and TraversalRankingPoint
respectively (keep the surrounding divs and the existing if/else rendering logic
and the playoffMatch guard unchanged so the display still shows "Yes"/"No" based
on the new fields).
2 participants
This PR makes some modifications to the 2026 rules:
Summary by CodeRabbit
New Features
Updates