Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis change adds per-station GameData to AllianceStation, resets it at match start (PreMatch), and propagates station-specific GameData through updated DriverStationConnection methods with a change-detection helper that sends game-data packets when needed. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
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)
210-211:⚠️ Potential issue | 🟡 MinorFix race condition: test waits too briefly for connection setup to complete.
The test sends a driver station connection packet (line 203), sleeps for only 10ms (line 209), then immediately asserts
DsConnis non-nil (line 211). However,listenForDriverStations()runs asynchronously in a separate goroutine, and theDsConnassignment at driver_station_connection.go:384 is not guaranteed to complete within 10ms. The delay should be increased, or the test should wait until the connection is established before proceeding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@field/driver_station_connection_test.go` around lines 210 - 211, The test currently sleeps 10ms then asserts arena.AllianceStations["B2"].DsConn is non-nil, which races with the asynchronous listenForDriverStations goroutine; replace the fixed short sleep with a deterministic wait loop or synchronization so the test blocks until DsConn is set (e.g., poll arena.AllianceStations["B2"].DsConn for non-nil with a reasonable timeout like 1s, or use a channel/WaitGroup signaled by the connection setup in listenForDriverStations) before calling assert.NotNil on dsConn.
🧹 Nitpick comments (2)
field/arena.go (1)
643-646: Consider initializingGameDataonly on state transition.This code runs on every
Update()call while inPreMatchstate (every 10ms), repeatedly settingGameDatato empty. While functionally correct, it would be more efficient to only clearGameDataonce when transitioning intoPreMatchor when the match is loaded.♻️ Suggested optimization
case PreMatch: auto = true enabled = false - // Set all game data values to empty - for _, allianceStation := range arena.AllianceStations { - allianceStation.GameData = "" - }And add the initialization in
LoadMatch()orResetMatch()where it would only execute once:// In LoadMatch() after resetting scores for _, allianceStation := range arena.AllianceStations { allianceStation.GameData = "" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@field/arena.go` around lines 643 - 646, The loop that clears allianceStation.GameData inside Update() while in the PreMatch state should be moved so it runs only once on state transition; remove the per-tick loop that sets arena.AllianceStations[*].GameData = "" from Update()/PreMatch and instead initialize/clear GameData for each entry in arena.AllianceStations within LoadMatch() (or ResetMatch()) right after match loading/reset logic so GameData is set once when the match is loaded or the state transitions into PreMatch.field/driver_station_connection.go (1)
449-458:SentGameDatais updated before sending, preventing retries on failure.If
sendGameDataPacketfails (line 453-456),SentGameDatahas already been updated (line 451), so subsequent calls won't attempt to resend the data. Consider updatingSentGameDataonly after a successful send if retry behavior is desired.♻️ Proposed fix for retry-safe error handling
func (dsConn *DriverStationConnection) checkGameData(gameData string) { needsGameDataUpdate := dsConn.SentGameData != gameData - dsConn.SentGameData = gameData if needsGameDataUpdate { err := dsConn.sendGameDataPacket(gameData) if err != nil { log.Printf("Error sending game data packet to Team %d: %v", dsConn.TeamId, err) + } else { + dsConn.SentGameData = gameData } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@field/driver_station_connection.go` around lines 449 - 458, The checkGameData method updates dsConn.SentGameData before attempting to send, preventing retries if sendGameDataPacket fails; modify checkGameData so it only sets dsConn.SentGameData = gameData after sendGameDataPacket returns nil, i.e., call sendGameDataPacket(gameData) when needsGameDataUpdate is true and on success assign SentGameData, logging the error (using dsConn.TeamId) on failure without mutating SentGameData so subsequent calls will retry.
🤖 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 `@field/driver_station_connection_test.go`:
- Around line 210-211: The test currently sleeps 10ms then asserts
arena.AllianceStations["B2"].DsConn is non-nil, which races with the
asynchronous listenForDriverStations goroutine; replace the fixed short sleep
with a deterministic wait loop or synchronization so the test blocks until
DsConn is set (e.g., poll arena.AllianceStations["B2"].DsConn for non-nil with a
reasonable timeout like 1s, or use a channel/WaitGroup signaled by the
connection setup in listenForDriverStations) before calling assert.NotNil on
dsConn.
---
Nitpick comments:
In `@field/arena.go`:
- Around line 643-646: The loop that clears allianceStation.GameData inside
Update() while in the PreMatch state should be moved so it runs only once on
state transition; remove the per-tick loop that sets
arena.AllianceStations[*].GameData = "" from Update()/PreMatch and instead
initialize/clear GameData for each entry in arena.AllianceStations within
LoadMatch() (or ResetMatch()) right after match loading/reset logic so GameData
is set once when the match is loaded or the state transitions into PreMatch.
In `@field/driver_station_connection.go`:
- Around line 449-458: The checkGameData method updates dsConn.SentGameData
before attempting to send, preventing retries if sendGameDataPacket fails;
modify checkGameData so it only sets dsConn.SentGameData = gameData after
sendGameDataPacket returns nil, i.e., call sendGameDataPacket(gameData) when
needsGameDataUpdate is true and on success assign SentGameData, logging the
error (using dsConn.TeamId) on failure without mutating SentGameData so
subsequent calls will retry.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
field/arena.gofield/driver_station_connection.gofield/driver_station_connection_test.go
|
I have no clue why that test is failing. It's passing locally. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.go`:
- Around line 449-459: checkGameData currently swallows send errors from
sendGameDataPacket causing update() to appear successful; change checkGameData
to return an error (instead of void), call sendGameDataPacket and if it returns
an error return that error (do NOT set dsConn.SentGameData on failure), and
update callers (e.g., update()) to propagate or handle the returned error so TCP
send failures are surfaced rather than only logged; ensure references to
dsConn.SentGameData and sendGameDataPacket are preserved and error paths
log/contextualize before propagating.
1 participant
The existing game data method had a few flaws. It would only send to DS's that were connected when the send was triggered. This means that if a DS was disconnected when triggered, or if a DS disconnected after triggering, a DS might not get game data updates.
Solve this by checking if there is new game data to send during the dsConn update, and if so triggering the send.
This will also make the new game data method in 2027 easier, as that is likely transitioning to sending game data over the UDP packet instead of over TCP.
Summary by CodeRabbit