Conversation
📝 WalkthroughWalkthroughThis pull request replaces coral/reef/algae/barge scoring with a fuel-and-climb model, adds hub LED DMX control and hub-activation/tie-break logic, introduces shift/transition timing, and updates backend, frontend, tests, and templates to reflect fuel/climb and hub indicators. Changes
Sequence Diagram(s)sequenceDiagram
participant MatchTimer as MatchTimer
participant Arena as Arena
participant HubCtrl as HubController
participant DMX as DMXNetwork
participant Client as WebClient
MatchTimer->>Arena: OnMatchStart (select autoTieWinner)
Arena->>Arena: determineAutoWinner()
Arena->>Client: sendHubActivationGameData("R"/"B")
MatchTimer->>Arena: periodic Update()
Arena->>HubCtrl: handleHubLights(isRedActive, isBlueActive)
HubCtrl->>HubCtrl: setLedHubColors(RGBs)
HubCtrl->>DMX: transmit DMX packet (E1.31)
DMX-->>HubCtrl: send ACK/UDP result
Arena->>Client: realtime score update (includes AutoTieWinner)
Client->>Client: updateHubIndicators() (render active/flashing)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 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: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@dmx/dmx.go`:
- Around line 53-59: NewOpenDMXController currently accepts redChannel and
blueChannel but does no range checking even though SetHubColors indexes a
[512]byte with offsets +1 and +2; add validation that both redChannel and
blueChannel are within 0..509 inclusive, and change NewOpenDMXController to
return an error when they are out of range (e.g., func
NewOpenDMXController(redChannel, blueChannel int) (*OpenDMXController, error));
ensure error paths return nil plus a descriptive error and update callers to
handle the error; keep OpenDMXController and stopChan creation unchanged when
inputs are valid.
- Around line 91-103: The loop currently calls dmx.connect() on every ticker
tick while unhealthy, flooding logs; add a cooldown/backoff so connect() is only
invoked periodically: add a field like connectCooldown time.Duration (e.g., 2s)
and lastConnectAttempt time.Time to the DMX struct, and change the select branch
that checks !dmx.IsHealthy() to only call dmx.connect() if
time.Since(dmx.lastConnectAttempt) >= dmx.connectCooldown (update
lastConnectAttempt when attempting), otherwise skip or call a lightweight health
probe; keep dmx.update() behavior unchanged and tune the cooldown/backoff policy
(fixed or exponential) as needed to reduce reconnect attempts and log rate.
- Around line 196-199: The Stop method on OpenDMXController currently closes
dmx.stopChan directly which panics if Stop is called multiple times; add a
sync.Once field (e.g., stopOnce) to the OpenDMXController struct and change Stop
to call dmx.stopOnce.Do(func(){ close(dmx.stopChan) }) so the channel close
happens exactly once and subsequent Stop calls are no-ops; ensure the stopOnce
field name matches usages and that stopChan is still initialized where it was
before.
- Around line 158-186: The update() method builds a malformed ENTTEC DMX USB Pro
packet: fix the payload length and insert the DMX Start Code. Allocate packet
length 518, set packet[2]=0x01 and packet[3]=0x02 (513 bytes), set
packet[4]=0x00 (DMX start code), copy dmx.dmxData into packet[5:518], keep
packet[0]=0x7E/packet[1]=6 and packet[516/517] adjusted so the final end byte is
at packet[517]=0xE7 before calling dmx.serialPort.Write; also consider renaming
OpenDMXController or verifying whether the hardware is ENTTEC DMX USB Pro vs raw
Open DMX and adjust protocol implementation accordingly (update references to
resetConnection and isHealthy as needed).
In `@field/arena_test.go`:
- Around line 1188-1189: The disabled test function
_TestPlcMatchCycleGameSpecificWithCoopDisabled still references removed fields
(redProcessorCount, blueProcessorCount, redTrussLights, blueTrussLights) and the
removed game.CoralBonusCoopEnabled symbol; either remove the function entirely
or replace its body with a t.Skip call explaining it’s pending REBUILT 2026
logic, ensuring you update or remove any references to redProcessorCount,
blueProcessorCount, redTrussLights, blueTrussLights, and
game.CoralBonusCoopEnabled in that function (locate the function by name to
apply the change).
- Around line 1035-1036: The test function
_TestPlcMatchCycleGameSpecificWithCoopEnabled is causing build failures because
it references removed FakePlc fields (e.g., FakePlc.redProcessorCount,
FakePlc.blueProcessorCount, FakePlc.redTrussLights, FakePlc.blueTrussLights);
update the test to either remove or comment out its entire body so the compiler
no longer type-checks those invalid field accesses, or replace those obsolete
field references with the new FakePlc fields (e.g., redHubCount, blueHubCount,
etc.) and adjust any logic accordingly so the function compiles until the test
is fully rewritten for REBUILT 2026.
In `@field/arena.go`:
- Around line 1104-1111: The log call in sendHubActivationGameData dereferences
allianceStation.Team.Id without ensuring Team is non-nil, which can panic when a
DsConn exists but the station has no team; update the loop that calls
allianceStation.DsConn.sendGameDataPacket to check if allianceStation.Team !=
nil before accessing allianceStation.Team.Id and use a safe fallback (e.g.,
"unknown" or 0) in the log message when Team is nil, keeping the
sendGameDataPacket error handling the same.
In `@field/team_sign.go`:
- Around line 225-229: The formatted total fuel string currently computes total
as score.ActiveFuel+score.InactiveFuel, omitting the AutoFuel component; update
the return to use the correct total (either scoreSummary.TotalFuel or
score.AutoFuel + score.ActiveFuel + score.InactiveFuel) so the printed value
matches game/score.go TotalFuel logic (see symbols score, scoreSummary,
AutoFuel, ActiveFuel, InactiveFuel).
In `@go.mod`:
- Line 21: Replace the unmaintained module reference "github.com/tarm/serial" in
go.mod with the actively maintained "go.bug.st/serial.v1" and update all import
statements in the code from "github.com/tarm/serial" to "go.bug.st/serial.v1";
then adjust usages to match the new library's API (e.g., constructors, Config
types, Open/Close/Read/Write method names) so the serial port creation and
operations compile against go.bug.st/serial.v1.
In `@led/controller.go`:
- Around line 117-130: The populatePacket method uses StartChannel directly
which allows a zero value to produce an off-by-one index and overwrite the DMX
start code; add a guard in Controller.Update() (or before calling
populatePacket) to ensure StartChannel >= 1 (e.g., if c.StartChannel == 0 set it
to 1 or return/log an error) so populatePacket and pixelDataOffset arithmetic
never compute a negative/invalid channel index; adjust callers of populatePacket
to use the validated/normalized startChannel.
- Around line 85-107: The packet allocation in Controller.Update currently uses
createBlankPacket(3) which is too small when StartChannel > 4 because
populatePacket writes at indices computed from pixelDataOffset + StartChannel -
1 + {0,1,2}; change the allocation logic so when len(dmx.packet) == 0 you
allocate based on the actual start channel (e.g.
createBlankPacket(pixelDataOffset + dmx.StartChannel + 2) or otherwise ensure
len(dmx.packet) > pixelDataOffset + dmx.StartChannel + 1) before calling
populatePacket; update the code around Controller.Update, dmx.packet, and the
createBlankPacket call so the packet length safely covers writes performed by
populatePacket using StartChannel and pixelDataOffset.
In `@model/event_settings.go`:
- Around line 115-117: EventSettings RP threshold fields (EnergizedRPThreshold,
SuperchargedRPThreshold, TraversalRPThreshold) default to zero in existing
records which makes comparisons in game/score.go always succeed; fix this by
adding a fallback in GetEventSettings to detect zero values and set them to the
intended game-package defaults (EnergizedRPThreshold=100,
SuperchargedRPThreshold=360, TraversalRPThreshold=50) before returning (or
persist a one-time migration update to the DB), and ensure field/arena.go
receives those non-zero values so ranking-point logic uses correct thresholds.
In `@static/js/scoring_panel.js`:
- Around line 212-226: handleAutoClimbClick currently assumes
currentRealtimeScore is set and dereferences currentRealtimeScore.Red/Blue and
AutoClimbStatuses, which can throw if no realtimeScore has arrived; before
reading those properties in handleAutoClimbClick, add a guard that verifies
currentRealtimeScore exists, that the selected alliance object
(currentRealtimeScore.Red or .Blue) and its .Score and .AutoClimbStatuses exist
and that AutoClimbStatuses[teamPosition-1] is defined; if the guard fails, bail
out early (no-op) or optionally log/debug and do not call websocket.send;
reference handleAutoClimbClick, currentRealtimeScore, alliance, and
AutoClimbStatuses when making this check.
In `@static/js/wall_display.js`:
- Around line 217-335: The three duplicated functions isRedHubActive,
isBlueHubActive, and shouldHubFlash should be extracted into a shared module
(e.g., create static/js/hub_utils.js) and the copies removed from
static/js/wall_display.js and static/js/audience_display.js; move the exact
implementations into hub_utils.js, expose them for consumption (either as ES
module exports or attach them to a shared global object used by both displays),
then update wall_display.js and audience_display.js to import or reference those
functions instead of defining them locally so both files call the single shared
implementations (keep the function names isRedHubActive, isBlueHubActive,
shouldHubFlash unchanged to minimize call-site edits).
- Around line 217-261: The isRedHubActive function currently uses hardcoded
timing and magic-match-state numbers; replace those with values derived from the
global matchTiming object and named state constants. Compute teleopStartSec =
matchTiming.WarmupDurationSec + matchTiming.AutoDurationSec +
matchTiming.PauseDurationSec, teleopEndSec = teleopStartSec +
matchTiming.TeleopDurationSec, and read transitionDurationSec,
endGameDurationSec and shiftDurationSec from matchTiming fields (use the
backend-exposed names), then use named constants (e.g., AUTO_PERIOD,
PAUSE_PERIOD, TELEOP_PERIOD) instead of 3/4/5 when checking matchState; update
the post-transition shift calculation to use shiftDurationSec and preserve the
redWonAuto logic against the computed shift. Ensure all references in
isRedHubActive to hardcoded numbers are replaced with these computed values and
named constants (function: isRedHubActive, global: matchTiming).
🟡 Minor comments (8)
game/foul.go-25-28 (1)
25-28:⚠️ Potential issue | 🟡 MinorStale year reference in comment.
The comment says "Special case in 2025 for G206" but this PR targets the 2026 game. If G206 still applies in 2026, update the year; if not, this entire block should be reconsidered.
Proposed fix
- if foul.Rule() != nil && foul.Rule().RuleNumber == "G206" { - // Special case in 2025 for G206, which is not actually a foul but does make the alliance ineligible for - // some bonus RPs. - return 0 - } + if foul.Rule() != nil && foul.Rule().RuleNumber == "G206" { + // Special case in 2026 for G206, which is not actually a foul but does make the alliance ineligible for + // some bonus RPs. + return 0 + }partner/tba.go-649-676 (1)
649-676:⚠️ Potential issue | 🟡 Minor
TeleopPointsunderreports teleop contributions.Line 661 sets
breakdown.TeleopPoints = scoreSummary.ActiveFuelPoints, which excludes teleop climb points. TheScoreSummarystruct contains aTeleopClimbPointsfield (already used on line 670), and the game scoring combines both:MatchPoints = ... + ActiveFuelPoints + TeleopClimbPoints. This inconsistency means the TBA breakdown won't sum correctly toTotalPoints.Use
breakdown.TeleopPoints = scoreSummary.ActiveFuelPoints + scoreSummary.TeleopClimbPointsto match the game's teleop scoring.game/ranking_fields_test.go-14-14 (1)
14-14:⚠️ Potential issue | 🟡 MinorReplace
rand.Seed(0)with a local seeded generator to maintain test reproducibility.
rand.Seedis deprecated since Go 1.20, and this test's assertions depend on deterministic seeding. SinceAddScoreSummaryusesrand.Float64()internally, update the test to:r := rand.New(rand.NewSource(0)) // Then pass r to AddScoreSummary if possible, or set it globally via rand.NewAlternatively, if the function signature cannot be modified, create a local random source but note this will require refactoring how
AddScoreSummaryaccesses randomness. Simply removing the seed call would break the test because Go 1.20+ auto-seeds the global generator randomly, not deterministically.static/css/audience_display.css-179-199 (1)
179-199:⚠️ Potential issue | 🟡 MinorStylelint:
.rp-labelmissing generic font family fallback.Line 181 uses
font-family: "FuturaLT"without a generic fallback (e.g.,sans-serif). While this matches the pre-existing pattern throughout the file, Stylelint flags it. Consider adding a generic fallback at least in new code:font-family: "FuturaLT", sans-serif;static/css/audience_display.css-153-173 (1)
153-173:⚠️ Potential issue | 🟡 MinorHub indicator styling is clean. Minor: rename keyframe
hubFlashtohub-flashper kebab-case convention.Stylelint flags
hubFlashas not matching the expectedkebab-casepattern for@keyframesnames. Since this is new code, it's worth aligning.Proposed fix
-.hub-indicator.flashing { - animation: hubFlash 0.5s ease-in-out infinite; -} -@keyframes hubFlash { +.hub-indicator.flashing { + animation: hub-flash 0.5s ease-in-out infinite; +} +@keyframes hub-flash { 0%, 100% { opacity: 1; } 50% { opacity: 0.3; } }game/score.go-44-51 (1)
44-51:⚠️ Potential issue | 🟡 Minor
AutoClimbStatusessilently ignores Level2/Level3 values.Only
EndgameLevel1awards points (15 pts), but the field type allows anyEndgameStatus. If a scoring panel or data import sets Level2/Level3 during auto, it would silently score 0 climb points with no error. Consider either adding a validation guard or extending theswitchwith explicit cases that log a warning.field/arena.go-660-665 (1)
660-665:⚠️ Potential issue | 🟡 MinorWeak randomness source for auto tie-breaker.
time.Now().UnixNano()%2is biased on many platforms — timer resolution can make even/odd outcomes non-uniform. Consider usingmath/rand(orcrypto/randif this must be auditable) for a fairer coin flip.Proposed fix
+import "math/rand" + // Randomly choose a tie-breaker for auto (50/50 chance) -if time.Now().UnixNano()%2 == 0 { +if rand.Intn(2) == 0 { arena.autoTieWinner = "red" } else { arena.autoTieWinner = "blue" }templates/scoring_panel.html-141-155 (1)
141-155:⚠️ Potential issue | 🟡 MinorAdd
ontouchstartanddisabledattributes to the climb buttons.The
teleop-climb-buttontemplate (line 153) is missing both attributes that all other scoring buttons in this template include. TheteamButton(line 101),counterbuttons (lines 109, 120), andreef-button(line 131) all haveontouchstartfor iOS responsiveness and startdisabled. The CSS even includes a:enabledselector for climb buttons (line 511), indicating they should support the disabled state. Additionally, the JavaScriptupdateUIModefunction manages disabled state for other scoring buttons but does not cover theclimb-input-buttonclass, meaning these buttons lack proper state management.Reference: Other button patterns in the template
{{define "teamButton"}} <button id="{{.id}}" class="team-button team-{{.index}} {{.class}}" onclick="{{.onclick}}" ontouchstart disabled>{{define "counter"}} <button class="counter-decrement scoring-button" onclick="handleCounterClick('{{.id}}', -1);" ontouchstart disabled>{{define "reefColumn"}} <button class="reef-button scoring-button" onclick="handleReefClick({{$index}}, 5 - {{$i}})" ontouchstart disabled></button>
🧹 Nitpick comments (19)
dmx/dmx.go (3)
30-38:Stop()is absent from theControllerinterface.
OpenDMXController.Stop()is the only way to gracefully shut down theRun()loop, but it's not on the interface. Code that holds aControllercan only callClose(), which doesn't stop the goroutine — the loop will keep attempting reconnects. If the interface is meant for polymorphic use (e.g., test doubles), consider addingStop()to it.
69-71:IsEnabled()readsportwithout the mutex, unlike every other accessor.
SetAddresswritesdmx.portunder the mutex, butIsEnabledreads it unprotected. This is technically a data race if called concurrently withSetAddress. In practice the race window is small (only called once at the start ofRun), but it's inconsistent withIsHealthywhich does lock.♻️ Quick fix for consistency
func (dmx *OpenDMXController) IsEnabled() bool { + dmx.mutex.Lock() + defer dmx.mutex.Unlock() return dmx.port != "" }
16-19: Consider consolidatingColortype withled.Colorto reduce duplication.Both
dmx.Colorandled.Colordefine identical structs withR, G, B uint8fields and the same predefined color constants. The duplicate definition violates DRY principles. Sincedmx.Coloris not used outside the dmx package andled.Coloris already established as the standard color type in the codebase,dmxcould import and reuseled.Colorto eliminate the duplication.field/team_sign.go (1)
206-212: TODO: Ranking point progress display is a placeholder.The current implementation just shows
TotalFuelwithout context. This is fine as a stub, but worth tracking to completion before release.Would you like me to open an issue to track the REBUILT-specific ranking point progress display?
led/controller.go (2)
15-21: Duplicate constant:sourceNameis unused.
sACNSourceName(line 17) andsourceName(line 18) are both"Cheesy Arena". OnlysACNSourceNameis referenced (line 208). Remove the unusedsourceName.Proposed fix
const ( sACNPort = 5568 sACNSourceName = "Cheesy Arena" - sourceName = "Cheesy Arena" pixelDataOffset = 126 heartbeatInterval = 1 * time.Second )
158-173: Stale comment on line 159.The commented-out code says
"ACN-E1.17\x00\x00\x00"but the actual bytes encode"ASC-E1.17\x00\x00\x00"(which is the correct sACN identifier). The stale comment could mislead future readers.Proposed fix — use copy() for clarity
// ACN packet identifier - // copy(packet[4:16], []byte("ACN-E1.17\x00\x00\x00")) - - // ACN packet identifier - packet[4] = 0x41 - packet[5] = 0x53 - packet[6] = 0x43 - packet[7] = 0x2d - packet[8] = 0x45 - packet[9] = 0x31 - packet[10] = 0x2e - packet[11] = 0x31 - packet[12] = 0x37 - packet[13] = 0x00 - packet[14] = 0x00 - packet[15] = 0x00 + // ACN packet identifier (E1.17) + copy(packet[4:16], []byte("ASC-E1.17\x00\x00\x00"))web/reports.go (1)
84-87: Column headers "Coop" and "Barge" are misleading for REBUILT 2026."Coop" always shows "0" now, and "Barge" actually displays
EndgamePoints. Consider renaming these headers to match the new semantics (e.g., remove or blank the "Coop" column; rename "Barge" to "Endgame").Proposed fix
- pdf.CellFormat(colWidths["Coop"], rowHeight, "Coop", "1", 0, "C", true, 0, "") + pdf.CellFormat(colWidths["Coop"], rowHeight, "Coop*", "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, "") + pdf.CellFormat(colWidths["Barge"], rowHeight, "Endgame", "1", 0, "C", true, 0, "")web/scoring_panel.go (1)
230-243: TheautoFuelserver-sideAutonomousguard is redundant but harmless.The JS client always sends
Autonomous: truefor the"autoFuel"command (line 208 ofscoring_panel.js). The server-sideif args.Autonomouscheck at line 239 provides defense-in-depth, but note that if the condition is false, the increment is silently dropped with no error sent back to the client. This is acceptable as-is.Also, the TODO on line 230 ("Add REBUILT-specific scoring commands here") appears partially stale since fuel commands are already present below it. Consider removing or updating it to indicate what additional commands are expected.
static/js/scoring_panel.js (1)
152-157:climbLevelNamesused as array index lookup — ensure server enum values stay in sync.
climbLevelNamesis indexed by the numeric value ofAutoClimbStatuses[i]andTeleopClimbStatuses[i](lines 175, 180). If the server-sideEndgameStatusenum ever adds values beyond index 3 or reorders, this will silently produceundefined. Consider adding a bounds check or a fallback.web/setup_settings.go (1)
143-145: RP threshold parsing follows existing conventions but silently defaults to 0 on invalid input.Like all other
strconv.Atoiusages in this file, parse errors are discarded. If an admin accidentally submits an empty field, the threshold silently becomes 0 (vs. defaults of 100, 360, 50 fromgame/score.go). This is consistent with the rest of the file, but worth noting since a 0 threshold could unintentionally award ranking points.💡 Optional: fall back to current value on parse failure
- eventSettings.EnergizedRPThreshold, _ = strconv.Atoi(r.PostFormValue("energizedRPThreshold")) - eventSettings.SuperchargedRPThreshold, _ = strconv.Atoi(r.PostFormValue("superchargedRPThreshold")) - eventSettings.TraversalRPThreshold, _ = strconv.Atoi(r.PostFormValue("traversalRPThreshold")) + if val, err := strconv.Atoi(r.PostFormValue("energizedRPThreshold")); err == nil { + eventSettings.EnergizedRPThreshold = val + } + if val, err := strconv.Atoi(r.PostFormValue("superchargedRPThreshold")); err == nil { + eventSettings.SuperchargedRPThreshold = val + } + if val, err := strconv.Atoi(r.PostFormValue("traversalRPThreshold")); err == nil { + eventSettings.TraversalRPThreshold = val + }game/match_timing_test.go (1)
122-194: Consider adding tests withwonAuto=falsefor theForScoringvariants.Both
TestIsRedHubActiveForScoringandTestIsBlueHubActiveForScoringonly exercise thewonAuto=trueparameter. While the baseIsRedHubActive/IsBlueHubActivetests cover both cases, the grace-period logic in theForScoringvariants interacts with thepreviousShiftparity check, which differs based onwonAuto. Adding a few assertions withfalsewould improve confidence in the grace-period → shift-transition interaction.static/css/wall_display.css (1)
186-212: Stylelint flags on font-family quoting and missing generic fallback are consistent with the rest of this file.The existing declarations (lines 72, 123, 226) all use the same quoted
"FuturaLT"/"FuturaLTBold"pattern without a generic fallback. No action needed for this PR, but if the team decides to address the stylelint warnings, it would be a file-wide cleanup.field/arena_notifiers.go (1)
323-323: Hardcodedfalsefor CoopertitionEnabled — consider removing the field entirely.Since Coopertition is not used in REBUILT, sending a hardcoded
falseis correct but the field is still in the struct and consumed by the frontend (line 459 ofaudience_display.js). If this feature is definitively removed, you could clean up the field and the frontend toggle. Not urgent — the current approach works.plc/plc.go (1)
307-309: Missing doc comment onSetHubMotors.
GetHubBallCountsandSetHubLightsboth have doc comments;SetHubMotorsis the only public method without one.Proposed fix
+// Sets the on/off state of the hub motors. func (plc *ModbusPlc) SetHubMotors(state bool) {field/arena.go (2)
1065-1086:determineAutoWinner()is called up to 3 times perUpdate()cycle, each time invokingSummarize()twice.Within
handlePlcInputOutput(around Line 1241) for fuel routing, again at Line 1271 for PLC hub lights, and once more inhandleHubLights(Line 1330) for DMX LEDs. SinceUpdate()runs on a 10ms loop, this creates 6Summarize()calls per cycle. Consider computing the auto winner once at the top ofhandlePlcInputOutput(or inUpdate()) and passing it down or caching it as a field.Also applies to: 1268-1276, 1326-1334
1268-1322: Substantial code duplication between PLC hub lights and DMX hub lights.The flash-timing logic (computing
teleopStartSec,teleopEndSec,transitionEndSec,shouldFlash, shift-boundary checks) is duplicated almost verbatim betweenhandlePlcInputOutput(Lines 1268–1322) andhandleHubLights(Lines 1326–1395). Extract the shared timing/flash computation into a helper to keep both paths in sync and reduce maintenance burden.Also applies to: 1326-1395
game/match_timing.go (3)
90-96: Dead code:postTransitionSec < 0is unreachable.At Lines 93–95 (and the symmetric Lines 143–145),
postTransitionSecis guaranteed non-negative because execution only reaches this point whenmatchTimeSec >= transitionEndSec(thematchTimeSec < transitionEndSecguard returnedtrueearlier at Lines 76/127).Proposed fix (IsRedHubActive)
postTransitionSec := matchTimeSec - transitionEndSec - if postTransitionSec < 0 { - return false - } shift := int(postTransitionSec / ShiftDurationSec)Also applies to: 140-146
65-107: Consider extracting shared hub-activity logic to reduce Red/Blue duplication.
IsRedHubActiveandIsBlueHubActiveare structurally identical — the only difference is thewonAutoparameter interpretation. Similarly for theForScoringvariants. A shared helper (e.g.,isHubActive(matchTimeSec, wonAuto bool)) could serve both, reducing ~90 lines of near-duplicate code.Also applies to: 116-158
160-212: Grace period logic for scoring is correct but has a redundant early-return.Lines 180–183 check
matchTimeSec >= transitionEndSec && < transitionEndSec+GracePeriodand returntrue. This case is also covered by the shift-based grace check below (shift 0,timeInShift < GracePeriod,previousShift < 0→ returntrue). The early return is harmless (both paths yieldtrue) but adds an extra code path to maintain. This applies symmetrically to the blue variant (Lines 233–237).
| // TODO: Rewrite this test for REBUILT 2026 game logic | ||
| func _TestPlcMatchCycleGameSpecificWithCoopEnabled(t *testing.T) { |
There was a problem hiding this comment.
Build failure: Renaming to _Test does not prevent compilation — dead code still references removed FakePlc fields.
Prefixing with _ stops go test from discovering the function, but the compiler still parses and type-checks it. The function body references fields that no longer exist on FakePlc (e.g., redProcessorCount, blueProcessorCount, redTrussLights, blueTrussLights), causing the pipeline failure shown:
undefined: FakePlc.redProcessorCount
To fix the build, either:
- Comment out or delete the function bodies entirely, or
- Update the field references to match the new
FakePlcstruct (redHubCount,blueHubCount, etc.), even if the tests are not yet rewritten.
Option 1: Stub out the function bodies
// TODO: Rewrite this test for REBUILT 2026 game logic
func _TestPlcMatchCycleGameSpecificWithCoopEnabled(t *testing.T) {
- arena := setupTestArena(t)
- var plc FakePlc
- plc.isEnabled = true
- arena.Plc = &plc
-
- // Check that no inputs or outputs are active before the match starts.
- assert.Equal(t, PreMatch, arena.MatchState)
- plc.redProcessorCount = 5
- ... (entire body)
+ t.Skip("TODO: Rewrite this test for REBUILT 2026 game logic")
}🤖 Prompt for AI Agents
In `@field/arena_test.go` around lines 1035 - 1036, The test function
_TestPlcMatchCycleGameSpecificWithCoopEnabled is causing build failures because
it references removed FakePlc fields (e.g., FakePlc.redProcessorCount,
FakePlc.blueProcessorCount, FakePlc.redTrussLights, FakePlc.blueTrussLights);
update the test to either remove or comment out its entire body so the compiler
no longer type-checks those invalid field accesses, or replace those obsolete
field references with the new FakePlc fields (e.g., redHubCount, blueHubCount,
etc.) and adjust any logic accordingly so the function compiles until the test
is fully rewritten for REBUILT 2026.
led/controller.go
Outdated
| func (dmx *Controller) populatePacket(color Color, startChannel int) { | ||
| // Clear DMX data area | ||
| for i := pixelDataOffset; i < len(dmx.packet); i++ { | ||
| dmx.packet[i] = 0 | ||
| } | ||
|
|
||
| // Light mapping (3 channels): | ||
| // 1: Red | ||
| // 2: Green | ||
| // 3: Blue | ||
| dmx.packet[pixelDataOffset+startChannel-1+0] = color.R | ||
| dmx.packet[pixelDataOffset+startChannel-1+1] = color.G | ||
| dmx.packet[pixelDataOffset+startChannel-1+2] = color.B | ||
| } |
There was a problem hiding this comment.
Zero-value StartChannel corrupts the DMX packet.
If StartChannel is left at its zero value (0), line 127 computes pixelDataOffset + 0 - 1 = 125, which overwrites the DMX start code byte instead of channel data. This silently produces a malformed sACN packet.
Either validate that StartChannel >= 1 in Update(), or initialize it to 1 by default.
Proposed fix — add a guard in Update()
func (dmx *Controller) Update() {
color := dmx.color
if dmx.conn == nil {
// This controller is not configured; do nothing.
return
}
+ if dmx.StartChannel < 1 {
+ dmx.StartChannel = 1
+ }
+
// Create the template packet if it doesn't already exist.
if len(dmx.packet) == 0 {
dmx.packet = createBlankPacket(3)
}🤖 Prompt for AI Agents
In `@led/controller.go` around lines 117 - 130, The populatePacket method uses
StartChannel directly which allows a zero value to produce an off-by-one index
and overwrite the DMX start code; add a guard in Controller.Update() (or before
calling populatePacket) to ensure StartChannel >= 1 (e.g., if c.StartChannel ==
0 set it to 1 or return/log an error) so populatePacket and pixelDataOffset
arithmetic never compute a negative/invalid channel index; adjust callers of
populatePacket to use the validated/normalized startChannel.
| EnergizedRPThreshold int | ||
| SuperchargedRPThreshold int | ||
| TraversalRPThreshold int |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "EnergizedRPThreshold|SuperchargedRPThreshold|TraversalRPThreshold" --type=go -C 3Repository: Team254/cheesy-arena
Length of output: 4259
🏁 Script executed:
cat -n model/event_settings.go | head -160Repository: Team254/cheesy-arena
Length of output: 6590
🏁 Script executed:
# Look for migration files related to event_settings
fd -e sql -e go | xargs grep -l "EnergizedRPThreshold\|SuperchargedRPThreshold\|TraversalRPThreshold" 2>/dev/null || echo "No migration files found"Repository: Team254/cheesy-arena
Length of output: 138
🏁 Script executed:
# Check for any database migration logic in the repository
find . -type f \( -name "*.sql" -o -name "*migration*" \) | head -20Repository: Team254/cheesy-arena
Length of output: 46
Zero-value defaults for RP thresholds will trivially award ranking points to all alliances.
Existing databases with EventSettings records will have these fields as 0. The comparison logic in game/score.go (lines 104, 109, 115) uses >= thresholds:
if fuelForRankingPoints >= EnergizedRPThreshold { award RP }
With a 0 threshold, this condition is always true (0 >= 0), causing ranking points to be awarded regardless of performance. When field/arena.go (lines 285-287) updates game variables from settings, this broken state takes effect immediately.
While the web UI allows reconfiguration, users must discover the issue themselves. Add a migration or default fallback in GetEventSettings to preserve the game package defaults (100, 360, 50) for missing values, or ensure the UI prominently surfaces the need to reconfigure thresholds after upgrade.
🤖 Prompt for AI Agents
In `@model/event_settings.go` around lines 115 - 117, EventSettings RP threshold
fields (EnergizedRPThreshold, SuperchargedRPThreshold, TraversalRPThreshold)
default to zero in existing records which makes comparisons in game/score.go
always succeed; fix this by adding a fallback in GetEventSettings to detect zero
values and set them to the intended game-package defaults
(EnergizedRPThreshold=100, SuperchargedRPThreshold=360, TraversalRPThreshold=50)
before returning (or persist a one-time migration update to the DB), and ensure
field/arena.go receives those non-zero values so ranking-point logic uses
correct thresholds.
| // Toggle auto climb between None (0) and L1 (1) | ||
| const handleAutoClimbClick = function (teamPosition) { | ||
| // Get current state from the score | ||
| let currentScore; | ||
| if (alliance === "red") { | ||
| currentScore = currentRealtimeScore.Red.Score; | ||
| } else { | ||
| currentScore = currentRealtimeScore.Blue.Score; | ||
| } | ||
|
|
||
| const currentLevel = currentScore.AutoClimbStatuses[teamPosition - 1]; | ||
| const newLevel = currentLevel === 0 ? 1 : 0; // Toggle between None and L1 | ||
|
|
||
| websocket.send("autoClimb", {TeamPosition: teamPosition, EndgameStatus: newLevel}); | ||
| } |
There was a problem hiding this comment.
Null dereference if auto climb is clicked before first realtimeScore message.
currentRealtimeScore is initialized to null (line 11). If handleAutoClimbClick is invoked before any realtimeScore websocket message has been received, accessing currentRealtimeScore.Red.Score or .Blue.Score on line 217/219 will throw a TypeError.
This can happen because scoringAvailable is set based on match state (line 97), independently of whether a realtimeScore event has arrived yet.
🛡️ Proposed fix: guard against null
const handleAutoClimbClick = function (teamPosition) {
// Get current state from the score
+ if (!currentRealtimeScore) return;
let currentScore;
if (alliance === "red") {
currentScore = currentRealtimeScore.Red.Score;📝 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.
| // Toggle auto climb between None (0) and L1 (1) | |
| const handleAutoClimbClick = function (teamPosition) { | |
| // Get current state from the score | |
| let currentScore; | |
| if (alliance === "red") { | |
| currentScore = currentRealtimeScore.Red.Score; | |
| } else { | |
| currentScore = currentRealtimeScore.Blue.Score; | |
| } | |
| const currentLevel = currentScore.AutoClimbStatuses[teamPosition - 1]; | |
| const newLevel = currentLevel === 0 ? 1 : 0; // Toggle between None and L1 | |
| websocket.send("autoClimb", {TeamPosition: teamPosition, EndgameStatus: newLevel}); | |
| } | |
| // Toggle auto climb between None (0) and L1 (1) | |
| const handleAutoClimbClick = function (teamPosition) { | |
| // Get current state from the score | |
| if (!currentRealtimeScore) return; | |
| let currentScore; | |
| if (alliance === "red") { | |
| currentScore = currentRealtimeScore.Red.Score; | |
| } else { | |
| currentScore = currentRealtimeScore.Blue.Score; | |
| } | |
| const currentLevel = currentScore.AutoClimbStatuses[teamPosition - 1]; | |
| const newLevel = currentLevel === 0 ? 1 : 0; // Toggle between None and L1 | |
| websocket.send("autoClimb", {TeamPosition: teamPosition, EndgameStatus: newLevel}); | |
| } |
🤖 Prompt for AI Agents
In `@static/js/scoring_panel.js` around lines 212 - 226, handleAutoClimbClick
currently assumes currentRealtimeScore is set and dereferences
currentRealtimeScore.Red/Blue and AutoClimbStatuses, which can throw if no
realtimeScore has arrived; before reading those properties in
handleAutoClimbClick, add a guard that verifies currentRealtimeScore exists,
that the selected alliance object (currentRealtimeScore.Red or .Blue) and its
.Score and .AutoClimbStatuses exist and that AutoClimbStatuses[teamPosition-1]
is defined; if the guard fails, bail out early (no-op) or optionally log/debug
and do not call websocket.send; reference handleAutoClimbClick,
currentRealtimeScore, alliance, and AutoClimbStatuses when making this check.
| const isRedHubActive = function(matchTimeSec, matchState, redWonAuto) { | ||
| const teleopStartSec = 23; // warmup(0) + auto(20) + pause(3) | ||
| const transitionDurationSec = 10; // First 10 seconds of teleop when both hubs are active | ||
| const teleopEndSec = 163; // teleopStartSec + teleop(140) | ||
| const endGameDurationSec = 30; | ||
|
|
||
| // During auto and pause, both hubs are active | ||
| // matchState: 3 = AUTO_PERIOD, 4 = PAUSE_PERIOD (see match_timing.js) | ||
| if (matchState === 3 || matchState === 4) { | ||
| return true; | ||
| } | ||
|
|
||
| // During transition period (first 10 seconds of teleop), both hubs are active | ||
| if (matchState === 5 && matchTimeSec >= teleopStartSec && matchTimeSec < teleopStartSec + transitionDurationSec) { | ||
| return true; | ||
| } | ||
|
|
||
| if (matchTimeSec >= teleopEndSec - endGameDurationSec && matchTimeSec < teleopEndSec) { | ||
| return true; | ||
| } | ||
|
|
||
| // After the match ends, hubs are not active | ||
| if (matchTimeSec >= teleopEndSec) { | ||
| return false; | ||
| } | ||
|
|
||
| if (matchTimeSec < teleopStartSec) { | ||
| return false; | ||
| } | ||
|
|
||
| // Calculate which alternating shift we're in (after transition period) | ||
| const postTransitionSec = matchTimeSec - (teleopStartSec + transitionDurationSec); | ||
| if (postTransitionSec < 0) { | ||
| return false; | ||
| } | ||
| const shift = Math.floor(postTransitionSec / 25); | ||
|
|
||
| if (redWonAuto) { | ||
| // Red won auto, so Red is INACTIVE first, then alternates | ||
| return shift % 2 === 1; | ||
| } else { | ||
| // Blue won auto, so Red is ACTIVE first, then alternates | ||
| return shift % 2 === 0; | ||
| } | ||
| }; | ||
|
|
||
| const isBlueHubActive = function(matchTimeSec, matchState, blueWonAuto) { | ||
| const teleopStartSec = 23; // warmup(0) + auto(20) + pause(3) | ||
| const transitionDurationSec = 10; // First 10 seconds of teleop when both hubs are active | ||
| const teleopEndSec = 163; // teleopStartSec + teleop(140) | ||
| const endGameDurationSec = 30; | ||
|
|
||
| // During auto and pause, both hubs are active | ||
| // matchState: 3 = AUTO_PERIOD, 4 = PAUSE_PERIOD (see match_timing.js) | ||
| if (matchState === 3 || matchState === 4) { | ||
| return true; | ||
| } | ||
|
|
||
| // During transition period (first 10 seconds of teleop), both hubs are active | ||
| if (matchState === 5 && matchTimeSec >= teleopStartSec && matchTimeSec < teleopStartSec + transitionDurationSec) { | ||
| return true; | ||
| } | ||
|
|
||
| if (matchTimeSec >= teleopEndSec - endGameDurationSec && matchTimeSec < teleopEndSec) { | ||
| return true; | ||
| } | ||
|
|
||
| // After the match ends, hubs are not active | ||
| if (matchTimeSec >= teleopEndSec) { | ||
| return false; | ||
| } | ||
|
|
||
| if (matchTimeSec < teleopStartSec) { | ||
| return false; | ||
| } | ||
|
|
||
| // Calculate which alternating shift we're in (after transition period) | ||
| const postTransitionSec = matchTimeSec - (teleopStartSec + transitionDurationSec); | ||
| if (postTransitionSec < 0) { | ||
| return false; | ||
| } | ||
| const shift = Math.floor(postTransitionSec / 25); | ||
|
|
||
| if (blueWonAuto) { | ||
| // Blue won auto, so Blue is INACTIVE first, then alternates | ||
| return shift % 2 === 1; | ||
| } else { | ||
| redCoral = `${data.Red.ScoreSummary.NumCoralLevels}/${data.Red.ScoreSummary.NumCoralLevelsGoal}`; | ||
| blueCoral = `${data.Blue.ScoreSummary.NumCoralLevels}/${data.Blue.ScoreSummary.NumCoralLevelsGoal}`; | ||
| // Red won auto, so Blue is ACTIVE first, then alternates | ||
| return shift % 2 === 0; | ||
| } | ||
| $(`#${redSide}Coral`).text(redCoral); | ||
| $(`#${redSide}Algae`).text(data.Red.ScoreSummary.NumAlgae); | ||
| $(`#${blueSide}Coral`).text(blueCoral); | ||
| $(`#${blueSide}Algae`).text(data.Blue.ScoreSummary.NumAlgae); | ||
| }; | ||
|
|
||
| const shouldHubFlash = function(matchTimeSec, matchState) { | ||
| const teleopStartSec = 23; // warmup(0) + auto(20) + pause(3) | ||
| const transitionDurationSec = 10; | ||
| const transitionEndSec = teleopStartSec + transitionDurationSec; | ||
| const teleopEndSec = 163; // teleopStartSec + teleop(140) | ||
| const shiftDurationSec = 25; | ||
| const flashThresholdSec = 3; | ||
|
|
||
| if (matchTimeSec >= teleopEndSec - flashThresholdSec && matchTimeSec < teleopEndSec) { | ||
| return true; | ||
| } | ||
|
|
||
| // Flash during last 3 seconds of transition period | ||
| if (matchTimeSec >= transitionEndSec - flashThresholdSec && matchTimeSec < transitionEndSec) { | ||
| return true; | ||
| } | ||
|
|
||
| // Flash during last 3 seconds of each shift (during teleop, not in END GAME) | ||
| // matchState: 5 = TELEOP_PERIOD (see match_timing.js) | ||
| if (matchState === 5 && matchTimeSec >= transitionEndSec && matchTimeSec < teleopEndSec - 30) { | ||
| const postTransitionSec = matchTimeSec - transitionEndSec; | ||
| const timeInShift = postTransitionSec % shiftDurationSec; | ||
| return timeInShift >= shiftDurationSec - flashThresholdSec; | ||
| } | ||
|
|
||
| return false; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Massive code duplication — isRedHubActive, isBlueHubActive, shouldHubFlash are copy-pasted from audience_display.js.
These three functions (~120 lines) are identical in both files. Extract them into a shared utility file (e.g., static/js/hub_utils.js) included by both displays. This avoids divergent bug fixes and eases maintenance.
🤖 Prompt for AI Agents
In `@static/js/wall_display.js` around lines 217 - 335, The three duplicated
functions isRedHubActive, isBlueHubActive, and shouldHubFlash should be
extracted into a shared module (e.g., create static/js/hub_utils.js) and the
copies removed from static/js/wall_display.js and static/js/audience_display.js;
move the exact implementations into hub_utils.js, expose them for consumption
(either as ES module exports or attach them to a shared global object used by
both displays), then update wall_display.js and audience_display.js to import or
reference those functions instead of defining them locally so both files call
the single shared implementations (keep the function names isRedHubActive,
isBlueHubActive, shouldHubFlash unchanged to minimize call-site edits).
| const isRedHubActive = function(matchTimeSec, matchState, redWonAuto) { | ||
| const teleopStartSec = 23; // warmup(0) + auto(20) + pause(3) | ||
| const transitionDurationSec = 10; // First 10 seconds of teleop when both hubs are active | ||
| const teleopEndSec = 163; // teleopStartSec + teleop(140) | ||
| const endGameDurationSec = 30; | ||
|
|
||
| // During auto and pause, both hubs are active | ||
| // matchState: 3 = AUTO_PERIOD, 4 = PAUSE_PERIOD (see match_timing.js) | ||
| if (matchState === 3 || matchState === 4) { | ||
| return true; | ||
| } | ||
|
|
||
| // During transition period (first 10 seconds of teleop), both hubs are active | ||
| if (matchState === 5 && matchTimeSec >= teleopStartSec && matchTimeSec < teleopStartSec + transitionDurationSec) { | ||
| return true; | ||
| } | ||
|
|
||
| if (matchTimeSec >= teleopEndSec - endGameDurationSec && matchTimeSec < teleopEndSec) { | ||
| return true; | ||
| } | ||
|
|
||
| // After the match ends, hubs are not active | ||
| if (matchTimeSec >= teleopEndSec) { | ||
| return false; | ||
| } | ||
|
|
||
| if (matchTimeSec < teleopStartSec) { | ||
| return false; | ||
| } | ||
|
|
||
| // Calculate which alternating shift we're in (after transition period) | ||
| const postTransitionSec = matchTimeSec - (teleopStartSec + transitionDurationSec); | ||
| if (postTransitionSec < 0) { | ||
| return false; | ||
| } | ||
| const shift = Math.floor(postTransitionSec / 25); | ||
|
|
||
| if (redWonAuto) { | ||
| // Red won auto, so Red is INACTIVE first, then alternates | ||
| return shift % 2 === 1; | ||
| } else { | ||
| // Blue won auto, so Red is ACTIVE first, then alternates | ||
| return shift % 2 === 0; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the default MatchTiming values and WarmupDurationSec
echo "=== MatchTiming definition in Go ==="
rg -n "WarmupDurationSec|MatchTiming" --type=go -C3 | head -50
echo ""
echo "=== Looking for match timing constants ==="
rg -n "WarmupDurationSec|AutoDurationSec|PauseDurationSec|TeleopDurationSec" --type=go | head -30
echo ""
echo "=== Match state enum/constants ==="
rg -n "AutoPeriod|PausePeriod|TeleopPeriod|WarmupPeriod" --type=go | head -30
echo ""
echo "=== JS match timing handler and websocket ==="
rg -n "handleMatchTiming|matchTiming" --type=js -C3 | head -60
echo ""
echo "=== Checking wall_display.js around line 525 for websocket ==="
sed -n '515,535p' static/js/wall_display.jsRepository: Team254/cheesy-arena
Length of output: 12417
🏁 Script executed:
#!/bin/bash
echo "=== Match state numeric constants ==="
rg -n "const.*AutoPeriod|const.*PausePeriod|const.*TeleopPeriod" --type=js
echo ""
echo "=== Looking for match_timing.js constants section ==="
head -100 static/js/match_timing.js
echo ""
echo "=== Checking if isRedHubActive uses matchTiming variable ==="
rg -n "isRedHubActive" --type=js -A 50 | head -80
echo ""
echo "=== Check if game config defines MatchTiming structure ==="
rg -n "type MatchTiming" --type=goRepository: Team254/cheesy-arena
Length of output: 8073
🏁 Script executed:
#!/bin/bash
echo "=== Check for game config/MatchTiming defaults ==="
find . -name "*.go" -type f | xargs grep -l "MatchTiming.*=" | head -5
echo ""
echo "=== Find where MatchTiming is defined ==="
rg -n "type MatchTiming\|var MatchTiming\|MatchTiming\s*=" --type=go | head -30
echo ""
echo "=== Check games folder for current season config ==="
ls -la game/
echo ""
echo "=== Look at game package for default MatchTiming ==="
rg -n "MatchTiming" game/*.go | head -40Repository: Team254/cheesy-arena
Length of output: 5150
🏁 Script executed:
#!/bin/bash
echo "=== game/match_timing.go full content ==="
cat game/match_timing.go
echo ""
echo "=== Check for TransitionDurationSec and ShiftDurationSec ==="
rg -n "TransitionDurationSec|ShiftDurationSec|EndGameDurationSec" --type=go | head -20Repository: Team254/cheesy-arena
Length of output: 12398
Hardcoded timing constants are misaligned with the configurable backend.
The function hardcodes all timing thresholds—teleopStartSec=23, transitionDurationSec=10, teleopEndSec=163, endGameDurationSec=30, and shiftDurationSec=25—while the backend exposes configurable MatchTiming values (WarmupDurationSec, AutoDurationSec, PauseDurationSec, TeleopDurationSec) via the matchTiming websocket message (registered at line 524–525). The function receives this timing data in the global matchTiming variable (see match_timing.js) but ignores it, instead relying on hardcoded values that correspond to default backend settings (0+20+3 = teleop start, etc.). If backend timing is changed via event settings, the frontend hub activation state will drift silently.
Use the matchTiming global variable to derive these thresholds, or compute them from backend constants exposed via the game.MatchTiming Go struct already sent via websocket. Also replace magic match-state numbers (3, 4, 5) with named JavaScript constants for clarity and maintainability.
🤖 Prompt for AI Agents
In `@static/js/wall_display.js` around lines 217 - 261, The isRedHubActive
function currently uses hardcoded timing and magic-match-state numbers; replace
those with values derived from the global matchTiming object and named state
constants. Compute teleopStartSec = matchTiming.WarmupDurationSec +
matchTiming.AutoDurationSec + matchTiming.PauseDurationSec, teleopEndSec =
teleopStartSec + matchTiming.TeleopDurationSec, and read transitionDurationSec,
endGameDurationSec and shiftDurationSec from matchTiming fields (use the
backend-exposed names), then use named constants (e.g., AUTO_PERIOD,
PAUSE_PERIOD, TELEOP_PERIOD) instead of 3/4/5 when checking matchState; update
the post-transition shift calculation to use shiftDurationSec and preserve the
redWonAuto logic against the computed shift. Ensure all references in
isRedHubActive to hardcoded numbers are replaced with these computed values and
named constants (function: isRedHubActive, global: matchTiming).
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
field/arena.go (2)
1241-1241:determineAutoWinner()(and its twoSummarize()calls) fires 3+ times per 10 ms tick.The result is stable within a single
Update()cycle. Cache it once inhandlePlcInputOutput()and pass it intohandleHubLights()/setLedHubColors()as a parameter, or compute it once inUpdate()and thread it through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@field/arena.go` at line 1241, determineAutoWinner() (and its internal Summarize() calls) is being invoked repeatedly within a single Update() cycle; compute its result once and pass it down instead of re-calling: move the call to determineAutoWinner()/Summarize() into Update() (or call it once inside handlePlcInputOutput()), store the boolean pair (redWonAuto, blueWonAuto) in a local variable, and change handlePlcInputOutput(), handleHubLights(), and setLedHubColors() signatures to accept that cached result as a parameter so those functions use the precomputed value rather than invoking determineAutoWinner()/Summarize() multiple times.
1277-1300: Flash-timing block is copy-pasted verbatim intohandleHubLights()(lines 1337–1359).
teleopStartSec,teleopEndSec,transitionEndSec, and the entireshouldFlashderivation appear identically in both functions. Extract to a helper:♻️ Proposed refactor — extract helper
+// computeShouldFlash returns true when the hub LEDs/lights should flash, +// i.e. within 3 seconds of a hub-deactivation boundary. +func computeShouldFlash(matchTimeSec float64) bool { + teleopStartSec := float64(game.MatchTiming.WarmupDurationSec + game.MatchTiming.AutoDurationSec + game.MatchTiming.PauseDurationSec) + teleopEndSec := teleopStartSec + float64(game.MatchTiming.TeleopDurationSec) + transitionEndSec := teleopStartSec + float64(game.TransitionDurationSec) + + if matchTimeSec >= teleopEndSec-3.0 && matchTimeSec < teleopEndSec { + return true + } + if matchTimeSec >= transitionEndSec-3.0 && matchTimeSec < transitionEndSec { + return true + } + if matchTimeSec >= transitionEndSec && matchTimeSec < teleopEndSec-float64(game.EndGameDurationSec) { + postTransitionSec := matchTimeSec - transitionEndSec + timeInShift := math.Mod(postTransitionSec, float64(game.ShiftDurationSec)) + if timeInShift >= float64(game.ShiftDurationSec)-3.0 { + return true + } + } + return false +}Then replace both duplicated blocks with
shouldFlash := computeShouldFlash(matchTimeSec).This also lets you swap the manual modulo in both call sites for the already-imported
math.Mod, consistent withsetLedHubColors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@field/arena.go` around lines 1277 - 1300, Duplicate flash-timing logic (teleopStartSec, teleopEndSec, transitionEndSec and shouldFlash derivation) appears in both handleHubLights and the other block; extract this logic into a single helper function (e.g., computeShouldFlash(matchTimeSec float64, game GameType) bool) and replace both duplicated blocks with shouldFlash := computeShouldFlash(matchTimeSec). In the new helper computeShouldFlash use the existing game.MatchTiming and game fields (WarmupDurationSec, AutoDurationSec, PauseDurationSec, TransitionDurationSec, TeleopDurationSec, EndGameDurationSec, ShiftDurationSec) to compute teleopStartSec/teleopEndSec/transitionEndSec and the three flash conditions (end of match, end of transition, last 3s of each shift), and use math.Mod instead of the manual integer division/modulo when computing timeInShift so callers like handleHubLights and setLedHubColors share the single implementation.
🤖 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/arena.go`:
- Around line 1088-1101: The comment above sendHubActivationGameData incorrectly
says "B" is sent for all ties; update the comment to reflect that
determineAutoWinner may use autoTieWinner (random coin flip) so "R" is sent when
Red won auto or won the tie-breaker coin flip and "B" otherwise; locate
sendHubActivationGameData and its call to determineAutoWinner and change the
explanatory comments accordingly (mention autoTieWinner/coin flip behavior) so
the comment matches the actual logic.
- Around line 1316-1318: The comment says "turn on both hub lights for testing"
but the code calls arena.Plc.SetHubLights(false, false) which turns them off;
update one to match intent—either change the call in arena.Plc.SetHubLights(...)
to use true, true when arena.MatchState == PreMatch so the lights are turned on,
or edit the comment to state "turn off" if the intended behavior is off; locate
this logic around arena.MatchState and the SetHubLights invocation and make the
change so comment and call are consistent.
- Around line 1216-1261: The code currently checks inGracePeriod at the outer if
but doesn't handle it in the inner branches so any positive delta during the
grace period is never applied; update the inner branching so the grace period is
treated like teleop scoring (route new fuel to ActiveFuel or InactiveFuel using
the same logic as TeleopPeriod). Concretely, modify the branch that checks
arena.MatchState (replace the existing "else if arena.MatchState ==
TeleopPeriod" with "else if arena.MatchState == TeleopPeriod || inGracePeriod"
or otherwise include inGracePeriod in that branch), and reuse
arena.determineAutoWinner(), arena.MatchTimeSec(), and
game.IsRedHubActiveForScoring / game.IsBlueHubActiveForScoring to assign
redScore.ActiveFuel/InactiveFuel and blueScore.ActiveFuel/InactiveFuel for
positive deltas.
---
Duplicate comments:
In `@field/arena.go`:
- Around line 1104-1111: When iterating arena.AllianceStations before calling
allianceStation.DsConn.sendGameDataPacket(gameData), guard against a nil
allianceStation.Team to avoid nil-pointer dereference: check
allianceStation.Team != nil (and optionally validate allianceStation.Team.Id)
before using Team.Id in the log or elsewhere; if Team is nil, log a safe message
that identifies the station/connection (e.g., "empty station" or use another
non-nil identifier) and continue. Update the loop around sendGameDataPacket and
the log.Printf call to perform this nil check and use a safe fallback when Team
is nil.
---
Nitpick comments:
In `@field/arena.go`:
- Line 1241: determineAutoWinner() (and its internal Summarize() calls) is being
invoked repeatedly within a single Update() cycle; compute its result once and
pass it down instead of re-calling: move the call to
determineAutoWinner()/Summarize() into Update() (or call it once inside
handlePlcInputOutput()), store the boolean pair (redWonAuto, blueWonAuto) in a
local variable, and change handlePlcInputOutput(), handleHubLights(), and
setLedHubColors() signatures to accept that cached result as a parameter so
those functions use the precomputed value rather than invoking
determineAutoWinner()/Summarize() multiple times.
- Around line 1277-1300: Duplicate flash-timing logic (teleopStartSec,
teleopEndSec, transitionEndSec and shouldFlash derivation) appears in both
handleHubLights and the other block; extract this logic into a single helper
function (e.g., computeShouldFlash(matchTimeSec float64, game GameType) bool)
and replace both duplicated blocks with shouldFlash :=
computeShouldFlash(matchTimeSec). In the new helper computeShouldFlash use the
existing game.MatchTiming and game fields (WarmupDurationSec, AutoDurationSec,
PauseDurationSec, TransitionDurationSec, TeleopDurationSec, EndGameDurationSec,
ShiftDurationSec) to compute teleopStartSec/teleopEndSec/transitionEndSec and
the three flash conditions (end of match, end of transition, last 3s of each
shift), and use math.Mod instead of the manual integer division/modulo when
computing timeInShift so callers like handleHubLights and setLedHubColors share
the single implementation.
| // Sends game data to all driver stations indicating which hub will go inactive first. | ||
| // "R" means Red hub goes inactive first (Red won auto). | ||
| // "B" means Blue hub goes inactive first (Blue won auto or tie). | ||
| func (arena *Arena) sendHubActivationGameData() { | ||
| // Determine who won auto | ||
| redWonAuto, _ := arena.determineAutoWinner() | ||
|
|
||
| // Determine which hub goes inactive first | ||
| var gameData string | ||
| if redWonAuto { | ||
| gameData = "R" // Red won auto, so Red hub goes inactive first | ||
| } else { | ||
| gameData = "B" // Blue won auto or tie, so Blue hub goes inactive first | ||
| } |
There was a problem hiding this comment.
Comment incorrectly states "B" is sent for all ties.
The tie-breaker in autoTieWinner is randomly either "red" or "blue", so "R" can equally result from a tie. The comment should read something like: "R" is sent when Red won auto or won the tie-breaker coin flip; "B" otherwise.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@field/arena.go` around lines 1088 - 1101, The comment above
sendHubActivationGameData incorrectly says "B" is sent for all ties; update the
comment to reflect that determineAutoWinner may use autoTieWinner (random coin
flip) so "R" is sent when Red won auto or won the tie-breaker coin flip and "B"
otherwise; locate sendHubActivationGameData and its call to determineAutoWinner
and change the explanatory comments accordingly (mention autoTieWinner/coin flip
behavior) so the comment matches the actual logic.
| if arena.MatchState == AutoPeriod || arena.MatchState == PausePeriod || arena.MatchState == TeleopPeriod || | ||
| inGracePeriod { | ||
| redScore.ProcessorAlgae, blueScore.ProcessorAlgae = arena.Plc.GetProcessorCounts() | ||
| redHubFuel, blueHubFuel := arena.Plc.GetHubBallCounts() | ||
|
|
||
| // Calculate the delta since last read using current score totals | ||
| redTotalFuel := redScore.AutoFuel + redScore.ActiveFuel + redScore.InactiveFuel | ||
| blueTotalFuel := blueScore.AutoFuel + blueScore.ActiveFuel + blueScore.InactiveFuel | ||
| redDelta := redHubFuel - redTotalFuel | ||
| blueDelta := blueHubFuel - blueTotalFuel | ||
|
|
||
| // Only process positive deltas (new FUEL scored) | ||
| if redDelta > 0 || blueDelta > 0 { | ||
| if arena.MatchState == AutoPeriod || arena.MatchState == PausePeriod { | ||
| // During auto and pause (transition), all FUEL counts as auto FUEL | ||
| // This includes the grace period after auto ends to account for FUEL in flight | ||
| if redDelta > 0 { | ||
| redScore.AutoFuel += redDelta | ||
| } | ||
| if blueDelta > 0 { | ||
| blueScore.AutoFuel += blueDelta | ||
| } | ||
| } else if arena.MatchState == TeleopPeriod { | ||
| // During teleop, route to active or inactive based on which hub is currently active | ||
| // Include grace period after hub deactivates to account for FUEL in flight | ||
| // Determine who won auto to know which hub is active | ||
| redWonAuto, blueWonAuto := arena.determineAutoWinner() | ||
|
|
||
| matchTimeSec := arena.MatchTimeSec() | ||
|
|
||
| if redDelta > 0 { | ||
| if game.IsRedHubActiveForScoring(matchTimeSec, redWonAuto) { | ||
| redScore.ActiveFuel += redDelta | ||
| } else { | ||
| redScore.InactiveFuel += redDelta | ||
| } | ||
| } | ||
|
|
||
| if blueDelta > 0 { | ||
| if game.IsBlueHubActiveForScoring(matchTimeSec, blueWonAuto) { | ||
| blueScore.ActiveFuel += blueDelta | ||
| } else { | ||
| blueScore.InactiveFuel += blueDelta | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Grace-period fuel is silently discarded — inGracePeriod is never reached by any inner scoring branch.
inGracePeriod is intentionally included in the outer if (line 1216) so fuel still in flight at the end of teleop is counted. However, when MatchState == PostMatch (the only state where inGracePeriod is true), neither inner branch fires: AutoPeriod || PausePeriod is false and TeleopPeriod is false. Any positive delta is computed but never written to ActiveFuel or InactiveFuel, so the fuel is permanently lost.
🐛 Proposed fix
- } else if arena.MatchState == TeleopPeriod {
+ } else if arena.MatchState == TeleopPeriod || inGracePeriod {📝 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.
| if arena.MatchState == AutoPeriod || arena.MatchState == PausePeriod || arena.MatchState == TeleopPeriod || | |
| inGracePeriod { | |
| redScore.ProcessorAlgae, blueScore.ProcessorAlgae = arena.Plc.GetProcessorCounts() | |
| redHubFuel, blueHubFuel := arena.Plc.GetHubBallCounts() | |
| // Calculate the delta since last read using current score totals | |
| redTotalFuel := redScore.AutoFuel + redScore.ActiveFuel + redScore.InactiveFuel | |
| blueTotalFuel := blueScore.AutoFuel + blueScore.ActiveFuel + blueScore.InactiveFuel | |
| redDelta := redHubFuel - redTotalFuel | |
| blueDelta := blueHubFuel - blueTotalFuel | |
| // Only process positive deltas (new FUEL scored) | |
| if redDelta > 0 || blueDelta > 0 { | |
| if arena.MatchState == AutoPeriod || arena.MatchState == PausePeriod { | |
| // During auto and pause (transition), all FUEL counts as auto FUEL | |
| // This includes the grace period after auto ends to account for FUEL in flight | |
| if redDelta > 0 { | |
| redScore.AutoFuel += redDelta | |
| } | |
| if blueDelta > 0 { | |
| blueScore.AutoFuel += blueDelta | |
| } | |
| } else if arena.MatchState == TeleopPeriod { | |
| // During teleop, route to active or inactive based on which hub is currently active | |
| // Include grace period after hub deactivates to account for FUEL in flight | |
| // Determine who won auto to know which hub is active | |
| redWonAuto, blueWonAuto := arena.determineAutoWinner() | |
| matchTimeSec := arena.MatchTimeSec() | |
| if redDelta > 0 { | |
| if game.IsRedHubActiveForScoring(matchTimeSec, redWonAuto) { | |
| redScore.ActiveFuel += redDelta | |
| } else { | |
| redScore.InactiveFuel += redDelta | |
| } | |
| } | |
| if blueDelta > 0 { | |
| if game.IsBlueHubActiveForScoring(matchTimeSec, blueWonAuto) { | |
| blueScore.ActiveFuel += blueDelta | |
| } else { | |
| blueScore.InactiveFuel += blueDelta | |
| } | |
| } | |
| } | |
| } | |
| if arena.MatchState == AutoPeriod || arena.MatchState == PausePeriod || arena.MatchState == TeleopPeriod || | |
| inGracePeriod { | |
| redHubFuel, blueHubFuel := arena.Plc.GetHubBallCounts() | |
| // Calculate the delta since last read using current score totals | |
| redTotalFuel := redScore.AutoFuel + redScore.ActiveFuel + redScore.InactiveFuel | |
| blueTotalFuel := blueScore.AutoFuel + blueScore.ActiveFuel + blueScore.InactiveFuel | |
| redDelta := redHubFuel - redTotalFuel | |
| blueDelta := blueHubFuel - blueTotalFuel | |
| // Only process positive deltas (new FUEL scored) | |
| if redDelta > 0 || blueDelta > 0 { | |
| if arena.MatchState == AutoPeriod || arena.MatchState == PausePeriod { | |
| // During auto and pause (transition), all FUEL counts as auto FUEL | |
| // This includes the grace period after auto ends to account for FUEL in flight | |
| if redDelta > 0 { | |
| redScore.AutoFuel += redDelta | |
| } | |
| if blueDelta > 0 { | |
| blueScore.AutoFuel += blueDelta | |
| } | |
| } else if arena.MatchState == TeleopPeriod || inGracePeriod { | |
| // During teleop, route to active or inactive based on which hub is currently active | |
| // Include grace period after hub deactivates to account for FUEL in flight | |
| // Determine who won auto to know which hub is active | |
| redWonAuto, blueWonAuto := arena.determineAutoWinner() | |
| matchTimeSec := arena.MatchTimeSec() | |
| if redDelta > 0 { | |
| if game.IsRedHubActiveForScoring(matchTimeSec, redWonAuto) { | |
| redScore.ActiveFuel += redDelta | |
| } else { | |
| redScore.InactiveFuel += redDelta | |
| } | |
| } | |
| if blueDelta > 0 { | |
| if game.IsBlueHubActiveForScoring(matchTimeSec, blueWonAuto) { | |
| blueScore.ActiveFuel += blueDelta | |
| } else { | |
| blueScore.InactiveFuel += blueDelta | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@field/arena.go` around lines 1216 - 1261, The code currently checks
inGracePeriod at the outer if but doesn't handle it in the inner branches so any
positive delta during the grace period is never applied; update the inner
branching so the grace period is treated like teleop scoring (route new fuel to
ActiveFuel or InactiveFuel using the same logic as TeleopPeriod). Concretely,
modify the branch that checks arena.MatchState (replace the existing "else if
arena.MatchState == TeleopPeriod" with "else if arena.MatchState == TeleopPeriod
|| inGracePeriod" or otherwise include inGracePeriod in that branch), and reuse
arena.determineAutoWinner(), arena.MatchTimeSec(), and
game.IsRedHubActiveForScoring / game.IsBlueHubActiveForScoring to assign
redScore.ActiveFuel/InactiveFuel and blueScore.ActiveFuel/InactiveFuel for
positive deltas.
| } else if arena.MatchState == PreMatch { | ||
| // During pre-match, turn on both hub lights for testing | ||
| arena.Plc.SetHubLights(false, false) |
There was a problem hiding this comment.
Comment and code are contradictory — comment says "turn on" but SetHubLights(false, false) turns both off.
Decide whether the intent is on (true, true) or off (false, false) and fix whichever is wrong.
🐛 Proposed fix (if lights should be on during pre-match for testing)
} else if arena.MatchState == PreMatch {
- // During pre-match, turn on both hub lights for testing
- arena.Plc.SetHubLights(false, false)
+ // During pre-match, turn on both hub lights for testing
+ arena.Plc.SetHubLights(true, true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@field/arena.go` around lines 1316 - 1318, The comment says "turn on both hub
lights for testing" but the code calls arena.Plc.SetHubLights(false, false)
which turns them off; update one to match intent—either change the call in
arena.Plc.SetHubLights(...) to use true, true when arena.MatchState == PreMatch
so the lights are turned on, or edit the comment to state "turn off" if the
intended behavior is off; locate this logic around arena.MatchState and the
SetHubLights invocation and make the change so comment and call are consistent.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
led/controller.go (1)
17-19: Remove unusedsourceNameconstant.
sourceNameat Line 19 is never referenced anywhere; onlysACNSourceNameis used increateBlankPacket. Dead constants add confusion about which identifier to use.🧹 Proposed cleanup
const ( sACNPort = 5568 sACNSourceName = "Cheesy Arena" - sourceName = "Cheesy Arena" pixelDataOffset = 126 heartbeatInterval = 1 * time.Second NumSegments = 16 )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@led/controller.go` around lines 17 - 19, Remove the unused constant sourceName: it's never referenced and duplicates sACNSourceName used by createBlankPacket; delete the declaration "sourceName = \"Cheesy Arena\"" and keep only sACNSourceName to avoid confusion and dead code, then run tests/compile to ensure no other references to sourceName remain (if any, replace them with sACNSourceName).field/arena.go (2)
1278-1300:shouldFlashcalculation is duplicated verbatim betweenhandlePlcInputOutputandhandleHubLights.Lines 1278–1300 and 1337–1359 compute
teleopStartSec,teleopEndSec,transitionEndSec, andshouldFlashwith identical logic. Extract a helper to keep the timing logic in one place.♻️ Suggested helper
+// computeHubFlashState returns whether hub LEDs should flash and the relevant +// timing boundaries for the given matchTimeSec. +func (arena *Arena) computeHubFlashState(matchTimeSec float64) (shouldFlash bool, teleopStartSec, transitionEndSec float64) { + teleopStartSec = float64(game.MatchTiming.WarmupDurationSec + game.MatchTiming.AutoDurationSec + game.MatchTiming.PauseDurationSec) + teleopEndSec := teleopStartSec + float64(game.MatchTiming.TeleopDurationSec) + transitionEndSec = teleopStartSec + float64(game.TransitionDurationSec) + + if matchTimeSec >= teleopEndSec-3.0 && matchTimeSec < teleopEndSec { + shouldFlash = true + } + if matchTimeSec >= transitionEndSec-3.0 && matchTimeSec < transitionEndSec { + shouldFlash = true + } + if matchTimeSec >= transitionEndSec && matchTimeSec < teleopEndSec-float64(game.EndGameDurationSec) { + postTransitionSec := matchTimeSec - transitionEndSec + if game.ShiftDurationSec > 0 { + timeInShift := math.Mod(postTransitionSec, float64(game.ShiftDurationSec)) + if timeInShift >= float64(game.ShiftDurationSec)-3.0 { + shouldFlash = true + } + } + } + return +}Also applies to: 1337-1359
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@field/arena.go` around lines 1278 - 1300, The flashing-timing logic is duplicated in handlePlcInputOutput and handleHubLights; extract a single helper (e.g., computeShouldFlash or isMatchInFlashWindow) that accepts game.MatchTiming and matchTimeSec and returns the boolean shouldFlash; move the calculations of teleopStartSec, teleopEndSec, transitionEndSec and the three flashing checks into that helper and replace the duplicated blocks in handlePlcInputOutput and handleHubLights with a call to the new helper.
660-665: Prefermath/randovertime.Now().UnixNano()%2for the tie-break coin flip.
UnixNano() % 2can exhibit bias (nanosecond timestamps are not uniformly distributed mod 2) and reads as surprising.rand.Intn(2)is the idiomatic Go coin flip.♻️ Proposed fix
- // Randomly choose a tie-breaker for auto (50/50 chance) - if time.Now().UnixNano()%2 == 0 { - arena.autoTieWinner = "red" - } else { - arena.autoTieWinner = "blue" - } + // Randomly choose a tie-breaker for auto (50/50 chance) + if rand.Intn(2) == 0 { + arena.autoTieWinner = "red" + } else { + arena.autoTieWinner = "blue" + }Add
"math/rand"to the imports if not already present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@field/arena.go` around lines 660 - 665, Replace the coin-flip using time.Now().UnixNano()%2 with math/rand's idiomatic call: use rand.Intn(2) to pick between "red" and "blue" for arena.autoTieWinner; add "math/rand" to imports if missing and ensure the logic in the block that currently checks time.Now().UnixNano()%2 assigns arena.autoTieWinner = "red" when rand.Intn(2) == 0 and "blue" otherwise.
🤖 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/arena.go`:
- Around line 1384-1386: handleHubLights() currently calls
arena.HubLedNotifier.Notify() every loop; change-gate this by only calling
Notify when the hub LED state actually changed. After calling
arena.RedHubLeds.Update() and arena.BlueHubLeds.Update(), compute whether the
visible colors changed (either compare the controller's colors array/slice or
snapshot a lastNotifiedColors for each hub and compare to the current
colors/GetColor() result) or use any boolean change flag returned by
led.Controller.Update() if available; only invoke arena.HubLedNotifier.Notify()
when that comparison/flag indicates a difference, and update the
lastNotifiedColors snapshot when you notify.
In `@led/controller.go`:
- Around line 34-40: Color.Scale currently converts scaled channel values with
uint8(float64(...) * multiplier) which truncates and wraps for values >255;
change Scale to perform saturating scaling: for each channel (R/G/B) compute a
float result := float64(c.X) * multiplier, handle non-finite results (NaN/Inf)
by treating as 0 or max as appropriate, round the result (e.g. math.Round),
clamp it to the 0..255 range, then cast to uint8. Update the Color.Scale
function to use this saturating clamp behavior so callers passing multiplier
>1.0 do not overflow.
---
Duplicate comments:
In `@field/arena.go`:
- Around line 1088-1101: Update the inaccurate comment in
sendHubActivationGameData: clarify that determineAutoWinner() resolves ties via
a random coin flip (autoTieWinner) so "B" is not always sent for ties; state
that on a tied auto the winner is randomly chosen (could be "R" or "B") and that
gameData is set to "R" when redWinsAuto is true and "B" otherwise. Refer to
sendHubActivationGameData and determineAutoWinner/autoTieWinner when editing the
comment.
- Around line 1316-1318: The comment for the PreMatch branch contradicts the
code: it says "turn on both hub lights for testing" but calls
arena.Plc.SetHubLights(false, false) which turns them off; update either the
comment or the call so behavior and text match. Locate the PreMatch check
(arena.MatchState == PreMatch) and either change the call to
arena.Plc.SetHubLights(true, true) to actually turn lights on, or modify the
comment to state that lights are turned off for testing—ensure the chosen fix
keeps the intent consistent across arena.MatchState, PreMatch, and SetHubLights.
- Around line 1104-1111: The log call can panic because allianceStation.Team may
be nil even when allianceStation.DsConn != nil; update the loop over
arena.AllianceStations (and the block around
allianceStation.DsConn.sendGameDataPacket) to additionally check
allianceStation.Team != nil before accessing allianceStation.Team.Id, and when
Team is nil log a safe message (e.g., use team ID fallback like "unknown" or log
that the station has no team) so that sendGameDataPacket is attempted only when
DsConn is non-nil and Team is non-nil and any error logging references a non-nil
Team.
- Around line 1216-1261: The code currently computes redDelta/blueDelta during
the grace period but never applies them because inGracePeriod is only true
during PostMatch and the inner branches only match Auto/Pause or Teleop; update
the branching so inGracePeriod is routed into the correct scoring branch instead
of being ignored — specifically, adjust the inner conditions that decide Auto vs
Teleop handling (the if that adds to redScore.AutoFuel/blueScore.AutoFuel and
the else if that uses arena.determineAutoWinner(), arena.MatchTimeSec(),
game.IsRedHubActiveForScoring()/game.IsBlueHubActiveForScoring() to add to
ActiveFuel/InactiveFuel) to also treat the grace period as the appropriate phase
(i.e., include a check that sends inGracePeriod into the Auto branch when it is
the auto-ending grace, or into the Teleop branch when it’s the teleop-ending
hub-grace), ensuring positive deltas are added to redScore/blueScore instead of
being discarded.
In `@led/controller.go`:
- Around line 121-130: The packet build is still vulnerable to a zero
StartChannel and to an undersized packet: populatePacket computes offsets like
pixelDataOffset + startChannel - 1 and writes up to pixelDataOffset +
(NumSegments-1)*3 + startChannel, so if StartChannel == 0 it overwrites the DMX
start-code, and createBlankPacket(NumSegments*3) can be too small. Fix by (1)
validating StartChannel in Update or at the start of populatePacket (e.g.,
require StartChannel >= 1 or clamp it to 1) so you never subtract 1 from zero,
and (2) allocate dmx.packet with enough bytes using createBlankPacket with size
= pixelDataOffset + NumSegments*3 + max(0, StartChannel-1) (or compute the exact
max write index used by populatePacket and allocate one plus that index) so
writes like pixelDataOffset + (NumSegments-1)*3 + StartChannel stay within
bounds.
---
Nitpick comments:
In `@field/arena.go`:
- Around line 1278-1300: The flashing-timing logic is duplicated in
handlePlcInputOutput and handleHubLights; extract a single helper (e.g.,
computeShouldFlash or isMatchInFlashWindow) that accepts game.MatchTiming and
matchTimeSec and returns the boolean shouldFlash; move the calculations of
teleopStartSec, teleopEndSec, transitionEndSec and the three flashing checks
into that helper and replace the duplicated blocks in handlePlcInputOutput and
handleHubLights with a call to the new helper.
- Around line 660-665: Replace the coin-flip using time.Now().UnixNano()%2 with
math/rand's idiomatic call: use rand.Intn(2) to pick between "red" and "blue"
for arena.autoTieWinner; add "math/rand" to imports if missing and ensure the
logic in the block that currently checks time.Now().UnixNano()%2 assigns
arena.autoTieWinner = "red" when rand.Intn(2) == 0 and "blue" otherwise.
In `@led/controller.go`:
- Around line 17-19: Remove the unused constant sourceName: it's never
referenced and duplicates sACNSourceName used by createBlankPacket; delete the
declaration "sourceName = \"Cheesy Arena\"" and keep only sACNSourceName to
avoid confusion and dead code, then run tests/compile to ensure no other
references to sourceName remain (if any, replace them with sACNSourceName).
| arena.RedHubLeds.Update() | ||
| arena.BlueHubLeds.Update() | ||
| arena.HubLedNotifier.Notify() |
There was a problem hiding this comment.
HubLedNotifier.Notify() fires unconditionally at 100 Hz — should be change-gated.
handleHubLights() is invoked every 10 ms arena loop iteration, and Line 1386 notifies subscribers on every call regardless of whether any hub LED color changed. Every other notifier in this codebase is gated on an actual state change (e.g., RealtimeScoreNotifier is only fired when scores differ; MatchTimeNotifier only fires on second boundaries). Broadcasting over WebSocket at 100 Hz will saturate clients during a match, even when LEDs are static.
Move the Notify() call inside a conditional that detects a color change, or leverage the existing heartbeat/delta mechanism in led.Controller.Update() (which already avoids re-sending unchanged packets) to drive the notification.
♻️ Suggested approach
+ prevRed := arena.RedHubLeds.GetColor()
+ prevBlue := arena.BlueHubLeds.GetColor()
+
arena.RedHubLeds.Update()
arena.BlueHubLeds.Update()
- arena.HubLedNotifier.Notify()
+
+ if !arena.RedHubLeds.GetColor().Equals(prevRed) || !arena.BlueHubLeds.GetColor().Equals(prevBlue) {
+ arena.HubLedNotifier.Notify()
+ }Note: for animated chase patterns, GetColor() returns the solid color field, not per-segment values. A more robust approach compares the colors array or tracks a lastNotifiedColors snapshot.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@field/arena.go` around lines 1384 - 1386, handleHubLights() currently calls
arena.HubLedNotifier.Notify() every loop; change-gate this by only calling
Notify when the hub LED state actually changed. After calling
arena.RedHubLeds.Update() and arena.BlueHubLeds.Update(), compute whether the
visible colors changed (either compare the controller's colors array/slice or
snapshot a lastNotifiedColors for each hub and compare to the current
colors/GetColor() result) or use any boolean change flag returned by
led.Controller.Update() if available; only invoke arena.HubLedNotifier.Notify()
when that comparison/flag indicates a difference, and update the
lastNotifiedColors snapshot when you notify.
| func (c Color) Scale(multiplier float64) Color { | ||
| return Color{ | ||
| R: uint8(float64(c.R) * multiplier), | ||
| G: uint8(float64(c.G) * multiplier), | ||
| B: uint8(float64(c.B) * multiplier), | ||
| } | ||
| } |
There was a problem hiding this comment.
Color.Scale silently overflows for multiplier > 1.0.
uint8(float64(c.R) * multiplier) truncates rather than saturates, so any caller passing a value above 1.0 will wrap silently. All current call sites happen to be ≤ 1.0, but this is a footgun for future use.
🛡️ Proposed fix — saturating scale
func (c Color) Scale(multiplier float64) Color {
+ clamp := func(v float64) uint8 {
+ if v < 0 { return 0 }
+ if v > 255 { return 255 }
+ return uint8(v)
+ }
return Color{
- R: uint8(float64(c.R) * multiplier),
- G: uint8(float64(c.G) * multiplier),
- B: uint8(float64(c.B) * multiplier),
+ R: clamp(float64(c.R) * multiplier),
+ G: clamp(float64(c.G) * multiplier),
+ B: clamp(float64(c.B) * multiplier),
}
}📝 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.
| func (c Color) Scale(multiplier float64) Color { | |
| return Color{ | |
| R: uint8(float64(c.R) * multiplier), | |
| G: uint8(float64(c.G) * multiplier), | |
| B: uint8(float64(c.B) * multiplier), | |
| } | |
| } | |
| func (c Color) Scale(multiplier float64) Color { | |
| clamp := func(v float64) uint8 { | |
| if v < 0 { return 0 } | |
| if v > 255 { return 255 } | |
| return uint8(v) | |
| } | |
| return Color{ | |
| R: clamp(float64(c.R) * multiplier), | |
| G: clamp(float64(c.G) * multiplier), | |
| B: clamp(float64(c.B) * multiplier), | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@led/controller.go` around lines 34 - 40, Color.Scale currently converts
scaled channel values with uint8(float64(...) * multiplier) which truncates and
wraps for values >255; change Scale to perform saturating scaling: for each
channel (R/G/B) compute a float result := float64(c.X) * multiplier, handle
non-finite results (NaN/Inf) by treating as 0 or max as appropriate, round the
result (e.g. math.Round), clamp it to the 0..255 range, then cast to uint8.
Update the Color.Scale function to use this saturating clamp behavior so callers
passing multiplier >1.0 do not overflow.
2 participants
Summary by CodeRabbit
New Features
Changes