Vision comms LED state, disable unused triggers, and cleanup#458
Vision comms LED state, disable unused triggers, and cleanup#458phurley67 wants to merge 2 commits into
Conversation
- Add VISION_COMMS_NO_POSE LED state (blinking yellow) for Mac alive but no tags - Add LED trigger for vision comms state while disabled - Comment out hood stowed trigger - Comment out middle canned shot binding - Remove stale commented-out dpad_down debug code - Delete unused TestSubsytem.java Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a new LED state to distinguish “vision comms alive but no pose/tags” from “vision comms down,” and cleans up unused/commented bindings and dead code.
Changes:
- Introduce
VISION_COMMS_NO_POSELED state (blinking yellow) and add a disabled-mode trigger to enable it. - Disable/comment out unused triggers/bindings and remove stale commented debug code.
- Delete unused
TestSubsytem.java.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/main/java/frc/util/TestSubsytem.java | Removes an unused subsystem implementation. |
| src/main/java/frc/robot/RobotContainer.java | Adds new LED behavior + trigger; comments out unused bindings/triggers. |
| src/main/java/frc/robot/constants/LEDConstants.java | Adds new LED enum state for the comms-alive/no-pose condition. |
| // Blinking YELLOW (this trigger) = Mac is alive, just no tags | ||
| // Default pattern = Everything working fine | ||
| new Trigger(new LEDBooleanSupplier(() -> DriverStation.isDisabled() | ||
| && vision.isCommsAlive() |
There was a problem hiding this comment.
PhotonVision currently does not define isCommsAlive() (only getMacMiniConnection() exists), so this new trigger will not compile on this branch. Either add isCommsAlive() to PhotonVision in this PR, or switch this call to an existing API and update the logic accordingly (or ensure this PR is merged only after the dependency that introduces the method).
| && vision.isCommsAlive() | |
| && vision.getMacMiniConnection() |
| @@ -253,6 +253,17 @@ private void configureLeds() { | |||
| // Turn off the "vision bad" LED state once the drivetrain has moved away from the origin, indicating we likely have a valid pose estimate. | |||
| new Trigger(new LEDBooleanSupplier(() -> (DriverStation.isDisabled() && drivetrain.getPose().getTranslation().getDistance(new Translation2d()) < 0.1))).whileTrue(leds.enableState(LED_STATES.VISION_BAD.id())); | |||
There was a problem hiding this comment.
As written, VISION_BAD is enabled whenever disabled + pose is near the origin, regardless of whether vision comms are alive. This conflicts with the new semantics described below (solid red = comms down) and will also be true at the same time as VISION_COMMS_NO_POSE when comms are alive. Consider gating VISION_BAD with a comms-down check (e.g., !vision.isCommsAlive() once available) so the states are mutually exclusive and the LED meanings match the comments.
| new Trigger(new LEDBooleanSupplier(() -> (DriverStation.isDisabled() && drivetrain.getPose().getTranslation().getDistance(new Translation2d()) < 0.1))).whileTrue(leds.enableState(LED_STATES.VISION_BAD.id())); | |
| new Trigger(new LEDBooleanSupplier(() -> (DriverStation.isDisabled() | |
| && !vision.isCommsAlive() | |
| && drivetrain.getPose().getTranslation().getDistance(new Translation2d()) < 0.1))) | |
| .whileTrue(leds.enableState(LED_STATES.VISION_BAD.id())); |
| // Show blinking yellow while disabled when the Mac Mini is sending | ||
| // heartbeats (comms alive) but no pose data is available (no tags | ||
| // visible). This helps the drive team distinguish: | ||
| // Solid RED (VISION_BAD) = Mac is dead or comms down | ||
| // Blinking YELLOW (this trigger) = Mac is alive, just no tags | ||
| // Default pattern = Everything working fine | ||
| new Trigger(new LEDBooleanSupplier(() -> DriverStation.isDisabled() | ||
| && vision.isCommsAlive() | ||
| && drivetrain.getPose().getTranslation().getDistance(new Translation2d()) < 0.1)) | ||
| .whileTrue(leds.enableState(LED_STATES.VISION_COMMS_NO_POSE.id())); |
There was a problem hiding this comment.
VISION_COMMS_NO_POSE will be enabled at the same time as VISION_BAD with the current triggers. Because LEDSubsystem.applyLEDS() applies higher state IDs first and lower IDs last (higher priority), VISION_BAD (lower ordinal) will override this new blinking-yellow state, so the yellow may never be visible. Ensure the triggers are mutually exclusive (or adjust state priority/IDs) so the intended indication can actually display.
| // VISION_COMMS_NO_POSE: Mac Mini is alive and sending heartbeats, | ||
| // but no AprilTags are visible. Shows blinking yellow on the | ||
| // underglow while disabled so the driver knows "comms good, just | ||
| // no tags" vs "everything is broken" (VISION_BAD = solid red). | ||
| VISION_COMMS_NO_POSE, |
There was a problem hiding this comment.
LED_STATES.id() returns ordinal(), so inserting VISION_COMMS_NO_POSE here shifts the numeric IDs (and therefore priority ordering) of every state after it. If anything external (Shuffleboard arrays, logs, PathPlanner events, driver expectations) relies on stable state IDs or priority, this is brittle. Consider assigning explicit fixed IDs (or appending new states at the end and documenting the priority scheme).
The VISION_COMMS_NO_POSE trigger depends on vision.isCommsAlive() which is added in #453 (networking-vision). Commented out with a TODO to uncomment after that PR lands. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Closes #455
Summary
Split from #337. Depends on #456 for
vision.isCommsAlive().Test plan
🤖 Generated with Claude Code