ExcaLib v2: Phoenix-6-native declarative library (audit + research + design + implementation) - #47
Open
YehudaRothstein wants to merge 14 commits into
Open
ExcaLib v2: Phoenix-6-native declarative library (audit + research + design + implementation)#47YehudaRothstein wants to merge 14 commits into
YehudaRothstein wants to merge 14 commits into
Conversation
Full inventory of the current library, robot code, dependencies, and ranked pain points (P-01..P-27) ahead of the library rebuild. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Per-team notes (5990, 5951, 6328, 3061, 254, 1678, CTRE, DogLog, Epilogue, maple-sim), AdvantageKit translation ledger, license table, and prioritized adopt/adapt/avoid recommendations R-01..R-23. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Package layout, Phoenix 6 FOC control layer, mechanism archetypes, state-machine engine, superstructure coordinator, telemetry/sim/auto approach, migration plan, and open decisions OD-1..OD-8. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Approved additions for ExcaLib v2 telemetry (DogLog + SignalLogger) and physics simulation (maple-sim + dyn4j). No version changes to existing vendordeps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- ControlMode (TorqueCurrentFOC default / Voltage fallback / DutyCycle) - Gains, MotionConstraints (MotionMagic + Expo), CurrentBudget records mapping to Phoenix 6 configs - CANDeviceId (id+bus value type, credit 254) - DeviceConfigs: config apply + read-back verify + retry + Alert (credit 254/3061 pattern) - SignalHub: per-bus batched signal registry with frequency tiers, optimizeAll, latency-compensated reads Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ics sim - ExcaTalonFX: verified config, SignalHub-registered signals, pre-allocated FOC + Voltage control requests, mechanism-unit API, follower support - Mechanism base: disconnect Alerts (debounced), DogLog telemetry, async brake/coast, coastCommand, SysId via SignalLogger - PositionalMechanism: MotionMagic(Expo) goals, soft-limit clamp, continuous-wrap turret goals, atGoal trigger - VelocityMechanism: MotionMagicVelocity setpoints, atSpeed trigger - RollerMechanism: volts/amps/duty output, current-spike detection - MechanismSim: TalonFXSimState + WPILib physics models (rotational/arm) Credits: 6328 RollerSystem, 254 CurrentSpikeDetector, TRIGON sim/dual-config concepts, CTRE Phoenix6-Examples idioms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- StateMachine<S>: enum states, default-deny transition table, guards (pending-request retry), onEnter/whileIn/onExit actions, Trigger integration, requestCommand/requestAndWaitCommand, DogLog telemetry (credit: 254 transition-table concept, simplified; MA5951 concept) - Superstructure<G> base: goal requests, in(goal) triggers, fan-out via machine onEnter actions (credit: 1678 SuperstructureGoal concept) - 9 unit tests covering transitions, guards, deny-by-default, whileIn command lifecycle - build.gradle: run tests on the WPILib JDK (other JDKs' bundled MSVC runtime crashes wpiHal.dll on Windows) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Telemetry: DogLog + SignalLogger lifecycle (NT publish off at comp) - TunableNumber: NT-backed live tuning with ifChanged, disabled outside tuning mode (credit: 6328 LoggedTunableNumber concept, no AK) - FaultReporter: 1 Hz device fault scan -> Alerts + DogLog faults (credit: 3061-lib concept); auto-registered by ExcaTalonFX - AllianceFlip: rotational-symmetry alliance flipping, season dims injected - Zones: Rectangle2d field-zone Triggers (credit: MA5951 concept) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR establishes a rebuilt FRC library ( ChangesPlanning docs and build/vendordep setup
ExcaLib v2 library implementation
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Driver
participant SwerveSubsystem
participant LimelightMegaTag2
participant AutoBuilder
Driver->>SwerveSubsystem: fieldCentricDriveCommand(x, y, omega)
SwerveSubsystem->>SwerveSubsystem: periodic() -> updateVision()
SwerveSubsystem->>LimelightMegaTag2: getEstimate()
LimelightMegaTag2-->>SwerveSubsystem: Estimate(pose, timestamp, tagCount)
SwerveSubsystem->>SwerveSubsystem: addVisionMeasurement(pose, stdDevs)
AutoBuilder->>SwerveSubsystem: configureAutoBuilder(translationGains, rotationGains)
sequenceDiagram
participant Caller
participant Superstructure
participant StateMachine
Caller->>Superstructure: request(goal)
Superstructure->>StateMachine: request(target)
StateMachine->>StateMachine: evaluate transition table / guard
alt guard passes
StateMachine->>StateMachine: exitState -> enterState
StateMachine-->>Superstructure: true
else guard fails
StateMachine->>StateMachine: store pending
StateMachine-->>Superstructure: false
StateMachine->>StateMachine: periodic() retries pending
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
…o glue - SwerveSubsystem: CTRE SwerveDrivetrain wrapper (250 Hz odometry, built-in sim thread), applyRequest command factory, operator perspective, DogLog telemetry - LimelightMegaTag2: self-contained NT client (orientation feed + botpose_orb_wpiblue), distance/tag-count scaled std-devs, external rejection gate for turret-mounted camera - DriveCommands: field-centric teleop (deadband rescaled), brake, idle - DriveToPose: profiled-PID short-range aligner - Autos: named-command registration + safe auto chooser - PathPlanner AutoBuilder wiring with wheel-force feedforwards Credits: CTRE Phoenix6-Examples SwerveWithPathPlanner idioms, 3061/254 swerve-on-CTRE pattern. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request lays down an initial ExcaLib v2 (frc.excalib2) implementation (device/config layer, mechanism archetypes, state machine, telemetry/sim helpers) alongside Phase 0/1/2 planning docs (audit, research, design) and new vendordeps to support the rebuild work.
Changes:
- Adds a new
frc.excalib2library skeleton: CTRE Phoenix 6 TalonFX wrapper + config verification, mechanism archetypes, state machine, and telemetry utilities. - Adds new vendordeps (DogLog + maple-sim) and a JUnit test for the new state machine.
- Adds rebuild documentation in
docs/(Audit/Research/Design) and adjusts Gradle test execution on Windows.
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| vendordeps/maple-sim.json | Adds maple-sim vendordep (currently pinned to a specific version). |
| vendordeps/DogLog.json | Adds DogLog vendordep for logging. |
| src/test/java/frc/excalib2/statemachine/StateMachineTest.java | New JUnit tests for StateMachine. |
| src/main/java/frc/excalib2/util/Zones.java | Field-zone Trigger helpers (rectangle + alliance-flipped rectangle). |
| src/main/java/frc/excalib2/util/AllianceFlip.java | Alliance-based pose/translation/rotation flipping helpers. |
| src/main/java/frc/excalib2/telemetry/TunableNumber.java | NT-backed tunable number with change detection + batch callback. |
| src/main/java/frc/excalib2/telemetry/Telemetry.java | Initializes DogLog options + CTRE SignalLogger. |
| src/main/java/frc/excalib2/telemetry/FaultReporter.java | Periodic TalonFX fault scanning to Alerts + DogLog faults. |
| src/main/java/frc/excalib2/superstructure/Superstructure.java | Base superstructure wrapper around StateMachine goal requests. |
| src/main/java/frc/excalib2/statemachine/StateMachine.java | Core enum-driven state machine with guards/actions + Commands integration. |
| src/main/java/frc/excalib2/sim/MechanismSim.java | Per-mechanism physics sim bridge for TalonFX sim state. |
| src/main/java/frc/excalib2/mechanisms/VelocityMechanism.java | Velocity mechanism archetype (MotionMagicVelocity + atSpeed trigger). |
| src/main/java/frc/excalib2/mechanisms/RollerMechanism.java | Roller mechanism archetype (open loop + current-spike trigger). |
| src/main/java/frc/excalib2/mechanisms/PositionalMechanism.java | Positional mechanism archetype (MotionMagic Expo/trap + wrap/limits). |
| src/main/java/frc/excalib2/mechanisms/MechanismConfig.java | Declarative builder/config for mechanisms (ratios, gains, motion, limits, sim). |
| src/main/java/frc/excalib2/mechanisms/Mechanism.java | Base mechanism plumbing: device init, telemetry, disconnect alert, sim step, async neutral mode, SysId. |
| src/main/java/frc/excalib2/device/SignalHub.java | Central status-signal registration + batched refresh + bus optimization. |
| src/main/java/frc/excalib2/device/ExcaTalonFX.java | TalonFX wrapper: signal registration, preallocated requests, control-mode switch. |
| src/main/java/frc/excalib2/device/DeviceConfigs.java | Apply + refresh + verify + retry for CTRE device configs. |
| src/main/java/frc/excalib2/device/CANDeviceId.java | CAN identity record (id + bus name). |
| src/main/java/frc/excalib2/control/MotionConstraints.java | MotionMagic constraint record (trapezoid/expo) -> CTRE configs. |
| src/main/java/frc/excalib2/control/Gains.java | Closed-loop gain record -> CTRE Slot0Configs. |
| src/main/java/frc/excalib2/control/CurrentBudget.java | Current budget record -> CTRE current/torque-current configs. |
| src/main/java/frc/excalib2/control/ControlMode.java | Control mode enum for request-family selection. |
| docs/RESEARCH.md | Phase 1 research notes and recommendations. |
| docs/DESIGN.md | Phase 2 design proposal for ExcaLib v2. |
| docs/AUDIT.md | Phase 0 audit of current codebase (baseline findings/pain points). |
| build.gradle | Adjusts test execution to use WPILib JDK on Windows when available. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+97
to
+101
| public Command requestAndWaitCommand(S target) { | ||
| return Commands.runOnce(() -> request(target)) | ||
| .andThen(Commands.waitUntil(() -> current == target)) | ||
| .withName(name + " -> " + target + " (wait)"); | ||
| } |
Comment on lines
+8
to
+30
| import org.junit.jupiter.api.AfterEach; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import java.util.concurrent.atomic.AtomicBoolean; | ||
| import java.util.concurrent.atomic.AtomicInteger; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.*; | ||
|
|
||
| class StateMachineTest { | ||
| private enum TestState { | ||
| IDLE, SPINNING, SHOOTING | ||
| } | ||
|
|
||
| @BeforeEach | ||
| void setup() { | ||
| assertTrue(HAL.initialize(500, 0)); | ||
| DriverStationSim.setEnabled(true); | ||
| DriverStationSim.notifyNewData(); | ||
| CommandScheduler.getInstance().cancelAll(); | ||
| } |
Comment on lines
+1
to
+5
| # Phase 0 — Audit: ExcaLib & Rebuilt2026 Robot Code | ||
|
|
||
| > **Date:** 2026-07-02 · **Branch:** `feature/lib-rebuild` · **Baseline:** `2f015c3` (main) | ||
| > **Build status at audit time:** ✅ `./gradlew build` passes | ||
| > **Scope:** full inventory of the current library (`frc.excalib`), robot code (`frc.robot`), dependencies, and ranked pain points, prior to the ground-up library rebuild. |
| @@ -0,0 +1,312 @@ | |||
| # Phase 2 — Design: ExcaLib v2 | |||
|
|
|||
| > **Date:** 2026-07-02 · **Branch:** `feature/lib-rebuild` · **Status: ⏸ AWAITING APPROVAL — no implementation code until this design is approved.** | |||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/frc/excalib2/device/ExcaTalonFX.java`:
- Around line 105-114: `ExcaTalonFX.setAmps()` silently commands `neutral` when
`controlMode` is not `TORQUE_CURRENT_FOC`, which hides unexpected torque-control
calls. Update `setAmps()` to emit a warning or error through the existing
logging path before falling back to `neutral`, including the requested amps and
the current `ControlMode` so callers can diagnose misconfiguration. Keep the
actual safety behavior unchanged, but make the fallback visible in the
`ExcaTalonFX` torque-control path.
In `@src/main/java/frc/excalib2/mechanisms/Mechanism.java`:
- Around line 39-40: `Mechanism` currently relies on each owning subsystem
manually calling `periodic()`, which makes its 20ms telemetry, disconnect alert,
and sim updates fragile. Update `Mechanism` so it self-registers its
housekeeping task with `PeriodicScheduler.PERIOD.MILLISECONDS_20.add(...)`
instead of depending on external delegation, and keep the existing `periodic()`
logic in that scheduled task (or a shared helper it calls) so the behavior still
runs automatically without requiring subsystem authors to remember anything.
- Around line 61-64: The follower motor and CANcoder in Mechanism are
created/configured but not included in fault/disconnect tracking. Update the
Mechanism setup so the follower motor is also monitored beyond motor.connected()
in the disconnectedAlert path, and make sure the CANcoder created in the
CANcoder block is registered with
frc.excalib2.telemetry.FaultReporter.register(...) like the leader ExcaTalonFX.
Use the existing Mechanism and disconnectedAlert symbols to locate the
monitoring logic and add both devices to the fault-reporting pipeline.
In `@src/main/java/frc/excalib2/mechanisms/MechanismConfig.java`:
- Around line 160-163: The continuous-wrap contract in MechanismConfig is not
being enforced, so add a validation in MechanismConfig.toTalonFXConfiguration()
or the Mechanism constructor that checks continuousWrap against the softLimits
fields. If continuousWrap is enabled and either soft limit is missing, throw an
IllegalStateException before building the configuration so
PositionalMechanism.wrapToRange cannot fall back to unbounded limits. Reference
MechanismConfig.continuousWrap() and the softLimits state when implementing the
check.
- Around line 165-169: The MechanismConfig.tolerance(Angle) builder is
overloading toleranceRotations for both position and velocity semantics, which
breaks unit safety for VelocityMechanism.atSpeed. Split the configuration into
separate, clearly typed builders/fields such as positionTolerance(Angle) and
velocityTolerance(AngularVelocity), and update PositionalMechanism and
VelocityMechanism to read the appropriate field instead of sharing
toleranceRotations. Keep the existing mechanism-specific symbols
(MechanismConfig, tolerance, VelocityMechanism.atSpeed) aligned with their real
physical units so callers cannot accidentally configure speed tolerance using an
Angle.
In `@src/main/java/frc/excalib2/statemachine/StateMachine.java`:
- Around line 47-56: The StateMachine constructor is eagerly invoking
initial-state side effects by calling enterState(builder.initial), which runs
the initial onEnter action and schedules the whileIn command during object
construction. Update StateMachine so construction only stores builder state and
defer the initial entry to an explicit start()/initialize() method (or similar)
that the owner calls after all subsystem fields are fully constructed. Use the
existing enterState method and the current/builder.initial symbols as the main
entry points, and ensure the constructor no longer triggers onEnter/whileIn
behavior.
In `@src/main/java/frc/excalib2/telemetry/FaultReporter.java`:
- Around line 75-85: FaultReporter.poll is still relying on manual per-loop
invocation, but it should be scheduled as a 20ms periodic task instead. Update
the registration path to use
PeriodicScheduler.PERIOD.MILLISECONDS_20.add(FaultReporter::poll), ideally in a
static initializer or Telemetry.init, and remove any need to call poll from
robotPeriodic so the existing poll() scan logic runs automatically every loop.
- Around line 50-59: The scan() method in FaultReporter is refreshing each
StatusSignal individually, which bypasses the library’s batched refresh flow.
Remove the per-signal refresh() calls for hardwareFault, deviceTempFault,
bootDuringEnable, and undervoltage, and rely on the existing once-per-loop
refresh mechanism (TalonFXMotor.refreshAll() or the SignalHub batching used
elsewhere) before reading the signal values. Keep the update(...) calls in
scan() and make them consume the already-refreshed values directly so
FaultReporter stays aligned with the rest of the telemetry code.
In `@src/main/java/frc/excalib2/telemetry/TunableNumber.java`:
- Around line 38-52: TunableNumber currently latches tuning state in the
constructor, so instances created before enableTuning() never get a live
NetworkTables entry. Update TunableNumber to lazily create the entry on first
get() access instead of deciding in the constructor, and make entry and key
non-final so the class can initialize the /Tuning/ key path when tuning becomes
enabled. Keep get() as the single place that checks tuningEnabled and
creates/uses the DoubleTopic entry.
In `@src/test/java/frc/excalib2/statemachine/StateMachineTest.java`:
- Around line 38-46: Add a test in StateMachineTest that builds a StateMachine
with whileIn(TestState.IDLE, ...) on the initial state and verifies the command
is scheduled immediately during construction, alongside the existing
initial-state assertions. Use the StateMachine.builder(...), whileIn, and
getCurrentState symbols so the new coverage clearly exercises the
constructor-side effect behavior in StateMachine.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e736a3d9-c3cc-4f1a-ac1f-ba92d22790c2
📒 Files selected for processing (28)
build.gradledocs/AUDIT.mddocs/DESIGN.mddocs/RESEARCH.mdsrc/main/java/frc/excalib2/control/ControlMode.javasrc/main/java/frc/excalib2/control/CurrentBudget.javasrc/main/java/frc/excalib2/control/Gains.javasrc/main/java/frc/excalib2/control/MotionConstraints.javasrc/main/java/frc/excalib2/device/CANDeviceId.javasrc/main/java/frc/excalib2/device/DeviceConfigs.javasrc/main/java/frc/excalib2/device/ExcaTalonFX.javasrc/main/java/frc/excalib2/device/SignalHub.javasrc/main/java/frc/excalib2/mechanisms/Mechanism.javasrc/main/java/frc/excalib2/mechanisms/MechanismConfig.javasrc/main/java/frc/excalib2/mechanisms/PositionalMechanism.javasrc/main/java/frc/excalib2/mechanisms/RollerMechanism.javasrc/main/java/frc/excalib2/mechanisms/VelocityMechanism.javasrc/main/java/frc/excalib2/sim/MechanismSim.javasrc/main/java/frc/excalib2/statemachine/StateMachine.javasrc/main/java/frc/excalib2/superstructure/Superstructure.javasrc/main/java/frc/excalib2/telemetry/FaultReporter.javasrc/main/java/frc/excalib2/telemetry/Telemetry.javasrc/main/java/frc/excalib2/telemetry/TunableNumber.javasrc/main/java/frc/excalib2/util/AllianceFlip.javasrc/main/java/frc/excalib2/util/Zones.javasrc/test/java/frc/excalib2/statemachine/StateMachineTest.javavendordeps/DogLog.jsonvendordeps/maple-sim.json
Comment on lines
+105
to
+114
| /** Direct torque-current output (FOC), amps. Falls back to voltage scaling in VOLTAGE mode. */ | ||
| public void setAmps(double amps) { | ||
| if (controlMode == ControlMode.TORQUE_CURRENT_FOC) { | ||
| talon.setControl(torqueOut.withOutput(amps)); | ||
| } else { | ||
| // No torque control without FOC — approximate nothing; callers in VOLTAGE mode | ||
| // should use setVolts. Stop instead of guessing. | ||
| talon.setControl(neutral); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Silent stop on setAmps() in non-FOC mode should be logged.
Falling back to neutral instead of the requested amps is a reasonable safety default, but doing so silently could mask real integration bugs (a caller expecting torque control gets a mechanism that just stops, with no diagnostic trail).
🛠️ Proposed fix to surface the fallback
public void setAmps(double amps) {
if (controlMode == ControlMode.TORQUE_CURRENT_FOC) {
talon.setControl(torqueOut.withOutput(amps));
} else {
// No torque control without FOC — approximate nothing; callers in VOLTAGE mode
// should use setVolts. Stop instead of guessing.
+ DogLog.logFault("SetAmpsWrongMode/" + name);
talon.setControl(neutral);
}
}📝 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.
Suggested change
| /** Direct torque-current output (FOC), amps. Falls back to voltage scaling in VOLTAGE mode. */ | |
| public void setAmps(double amps) { | |
| if (controlMode == ControlMode.TORQUE_CURRENT_FOC) { | |
| talon.setControl(torqueOut.withOutput(amps)); | |
| } else { | |
| // No torque control without FOC — approximate nothing; callers in VOLTAGE mode | |
| // should use setVolts. Stop instead of guessing. | |
| talon.setControl(neutral); | |
| } | |
| } | |
| /** Direct torque-current output (FOC), amps. Falls back to voltage scaling in VOLTAGE mode. */ | |
| public void setAmps(double amps) { | |
| if (controlMode == ControlMode.TORQUE_CURRENT_FOC) { | |
| talon.setControl(torqueOut.withOutput(amps)); | |
| } else { | |
| // No torque control without FOC — approximate nothing; callers in VOLTAGE mode | |
| // should use setVolts. Stop instead of guessing. | |
| DogLog.logFault("SetAmpsWrongMode/" + name); | |
| talon.setControl(neutral); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/frc/excalib2/device/ExcaTalonFX.java` around lines 105 - 114,
`ExcaTalonFX.setAmps()` silently commands `neutral` when `controlMode` is not
`TORQUE_CURRENT_FOC`, which hides unexpected torque-control calls. Update
`setAmps()` to emit a warning or error through the existing logging path before
falling back to `neutral`, including the requested amps and the current
`ControlMode` so callers can diagnose misconfiguration. Keep the actual safety
behavior unchanged, but make the fallback visible in the `ExcaTalonFX`
torque-control path.
Comment on lines
+39
to
+40
| * <p>The owning subsystem must call {@link #periodic()} from its own {@code periodic()}. | ||
| */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Self-register periodic() via PeriodicScheduler instead of requiring manual delegation.
Mechanism is explicitly not a WPILib subsystem, yet its 20ms housekeeping (telemetry, disconnect alert, sim stepping) depends on every owning subsystem remembering to call periodic(). If a subsystem author forgets, disconnect alerts, telemetry, and sim updates silently stop.
♻️ Suggested fix
protected Mechanism(MechanismConfig config) {
this.config = config;
@@
}
+
+ PeriodicScheduler.PERIOD.MILLISECONDS_20.add(this::periodic);
}Also applies to: 88-106
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/frc/excalib2/mechanisms/Mechanism.java` around lines 39 - 40,
`Mechanism` currently relies on each owning subsystem manually calling
`periodic()`, which makes its 20ms telemetry, disconnect alert, and sim updates
fragile. Update `Mechanism` so it self-registers its housekeeping task with
`PeriodicScheduler.PERIOD.MILLISECONDS_20.add(...)` instead of depending on
external delegation, and keep the existing `periodic()` logic in that scheduled
task (or a shared helper it calls) so the behavior still runs automatically
without requiring subsystem authors to remember anything.
Source: Coding guidelines
Comment on lines
+61
to
+64
| if (config.cancoderId != null && RobotBase.isReal()) { | ||
| CANcoder cancoder = new CANcoder(config.cancoderId.id(), config.cancoderId.toCANBus()); | ||
| DeviceConfigs.applyVerified(config.name + "/CANcoder", cancoder, config.toCANcoderConfiguration()); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Follower motor and CANcoder aren't covered by fault/disconnect monitoring.
disconnectedAlert only checks motor.connected() (Line 96) — a disconnected follower goes unreported. Similarly, the CANcoder (Line 61-64) is configured but never passed to frc.excalib2.telemetry.FaultReporter.register(...) the way ExcaTalonFX self-registers the leader Talon, so CANcoder faults are invisible to the fault-reporting pipeline.
Also applies to: 96-96
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/frc/excalib2/mechanisms/Mechanism.java` around lines 61 - 64,
The follower motor and CANcoder in Mechanism are created/configured but not
included in fault/disconnect tracking. Update the Mechanism setup so the
follower motor is also monitored beyond motor.connected() in the
disconnectedAlert path, and make sure the CANcoder created in the CANcoder block
is registered with frc.excalib2.telemetry.FaultReporter.register(...) like the
leader ExcaTalonFX. Use the existing Mechanism and disconnectedAlert symbols to
locate the monitoring logic and add both devices to the fault-reporting
pipeline.
Comment on lines
+160
to
+163
| public MechanismConfig continuousWrap() { | ||
| this.continuousWrap = true; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
continuousWrap() contract isn't enforced.
Javadoc states this "Requires {@link #softLimits}" but nothing validates that at build/construction time. If a caller forgets to set soft limits, PositionalMechanism.wrapToRange silently treats limits as ±infinity, defeating the wrap-to-range behavior without any warning.
Consider validating in toTalonFXConfiguration() or the Mechanism constructor, throwing IllegalStateException when continuousWrap is true but either soft limit is null.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/frc/excalib2/mechanisms/MechanismConfig.java` around lines 160
- 163, The continuous-wrap contract in MechanismConfig is not being enforced, so
add a validation in MechanismConfig.toTalonFXConfiguration() or the Mechanism
constructor that checks continuousWrap against the softLimits fields. If
continuousWrap is enabled and either soft limit is missing, throw an
IllegalStateException before building the configuration so
PositionalMechanism.wrapToRange cannot fall back to unbounded limits. Reference
MechanismConfig.continuousWrap() and the softLimits state when implementing the
check.
Comment on lines
+165
to
+169
| /** At-goal / at-speed tolerance. */ | ||
| public MechanismConfig tolerance(Angle tolerance) { | ||
| this.toleranceRotations = tolerance.in(Rotations); | ||
| return this; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
toleranceRotations is reused for two different physical quantities.
tolerance(Angle) stores a position tolerance in rotations, but VelocityMechanism.atSpeed (VelocityMechanism.java Line 21-24) reinterprets the same config.toleranceRotations field as a velocity tolerance in rotations-per-second. Callers configuring a VelocityMechanism must pass an Angle (e.g. Rotations.of(2)) to mean "2 rotations/sec", defeating the unit-safety this config class is designed to provide and inviting silent misconfiguration.
Consider splitting into distinct fields/builders, e.g. positionTolerance(Angle) and velocityTolerance(AngularVelocity), so PositionalMechanism and VelocityMechanism each read a semantically-correct, unit-typed field.
♻️ Suggested direction
- double toleranceRotations = 0.02;
+ double positionToleranceRotations = 0.02;
+ double velocityToleranceRotationsPerSecond = 0.5;
...
- /** At-goal / at-speed tolerance. */
- public MechanismConfig tolerance(Angle tolerance) {
- this.toleranceRotations = tolerance.in(Rotations);
+ /** At-goal tolerance (positional mechanisms). */
+ public MechanismConfig tolerance(Angle tolerance) {
+ this.positionToleranceRotations = tolerance.in(Rotations);
+ return this;
+ }
+
+ /** At-speed tolerance (velocity mechanisms). */
+ public MechanismConfig velocityTolerance(AngularVelocity tolerance) {
+ this.velocityToleranceRotationsPerSecond = tolerance.in(RotationsPerSecond);
return this;
}Also applies to: 53-63
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/frc/excalib2/mechanisms/MechanismConfig.java` around lines 165
- 169, The MechanismConfig.tolerance(Angle) builder is overloading
toleranceRotations for both position and velocity semantics, which breaks unit
safety for VelocityMechanism.atSpeed. Split the configuration into separate,
clearly typed builders/fields such as positionTolerance(Angle) and
velocityTolerance(AngularVelocity), and update PositionalMechanism and
VelocityMechanism to read the appropriate field instead of sharing
toleranceRotations. Keep the existing mechanism-specific symbols
(MechanismConfig, tolerance, VelocityMechanism.atSpeed) aligned with their real
physical units so callers cannot accidentally configure speed tolerance using an
Angle.
Comment on lines
+50
to
+59
| void scan() { | ||
| hardwareFault.refresh(); | ||
| deviceTempFault.refresh(); | ||
| bootDuringEnable.refresh(); | ||
| undervoltage.refresh(); | ||
| update(hardwareAlert, hardwareFault, "HardwareFault"); | ||
| update(tempAlert, deviceTempFault, "DeviceTempFault"); | ||
| update(bootAlert, bootDuringEnable, "BootDuringEnable"); | ||
| update(undervoltageAlert, undervoltage, "Undervoltage"); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Individual refresh() calls violate the batched-refresh convention.
scan() calls .refresh() on each StatusSignal individually instead of relying on the once-per-loop TalonFXMotor.refreshAll() (or the existing SignalHub batching used elsewhere in this library). This duplicates CAN traffic and works against the "refresh once per loop" invariant the rest of the codebase relies on.
♻️ Suggested approach
void scan() {
- hardwareFault.refresh();
- deviceTempFault.refresh();
- bootDuringEnable.refresh();
- undervoltage.refresh();
update(hardwareAlert, hardwareFault, "HardwareFault");
update(tempAlert, deviceTempFault, "DeviceTempFault");
update(bootAlert, bootDuringEnable, "BootDuringEnable");
update(undervoltageAlert, undervoltage, "Undervoltage");
}Register these signals with SignalHub/refreshAll() so they're refreshed alongside the rest of the device signals, and read .getValue() directly here.
As per coding guidelines, "Do not call refresh() on individual motors in periodic code — use TalonFXMotor.refreshAll() instead, which is called once per robot loop in Robot.robotPeriodic()".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/frc/excalib2/telemetry/FaultReporter.java` around lines 50 -
59, The scan() method in FaultReporter is refreshing each StatusSignal
individually, which bypasses the library’s batched refresh flow. Remove the
per-signal refresh() calls for hardwareFault, deviceTempFault, bootDuringEnable,
and undervoltage, and rely on the existing once-per-loop refresh mechanism
(TalonFXMotor.refreshAll() or the SignalHub batching used elsewhere) before
reading the signal values. Keep the update(...) calls in scan() and make them
consume the already-refreshed values directly so FaultReporter stays aligned
with the rest of the telemetry code.
Source: Coding guidelines
Comment on lines
+75
to
+85
| /** Scans all registered devices at most once per second. Call every loop. */ | ||
| public static void poll() { | ||
| double now = Timer.getFPGATimestamp(); | ||
| if (now - lastScanTimestamp < SCAN_PERIOD_SECONDS) { | ||
| return; | ||
| } | ||
| lastScanTimestamp = now; | ||
| for (MonitoredDevice device : DEVICES) { | ||
| device.scan(); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consider registering via PeriodicScheduler instead of manual per-loop calls.
poll() is exactly the "must run every 20ms, not a full subsystem" case. Registering it with PeriodicScheduler.PERIOD.MILLISECONDS_20.add(FaultReporter::poll) (e.g., in a static initializer or Telemetry.init) removes the burden of remembering to call it from robotPeriodic.
As per coding guidelines, "Use PeriodicScheduler.PERIOD.MILLISECONDS_20.add(task) for tasks that must run every 20ms loop but should not be implemented as a full WPILib subsystem".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/frc/excalib2/telemetry/FaultReporter.java` around lines 75 -
85, FaultReporter.poll is still relying on manual per-loop invocation, but it
should be scheduled as a 20ms periodic task instead. Update the registration
path to use PeriodicScheduler.PERIOD.MILLISECONDS_20.add(FaultReporter::poll),
ideally in a static initializer or Telemetry.init, and remove any need to call
poll from robotPeriodic so the existing poll() scan logic runs automatically
every loop.
Source: Coding guidelines
Comment on lines
+38
to
+52
| public TunableNumber(String key, double defaultValue) { | ||
| this.defaultValue = defaultValue; | ||
| if (tuningEnabled) { | ||
| entry = NetworkTableInstance.getDefault() | ||
| .getDoubleTopic("/Tuning/" + key) | ||
| .getEntry(defaultValue); | ||
| entry.set(defaultValue); | ||
| } else { | ||
| entry = null; | ||
| } | ||
| } | ||
|
|
||
| public double get() { | ||
| return entry == null ? defaultValue : entry.get(defaultValue); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Tuning state is latched at construction time, not lazily checked.
Whether an NT entry is created is decided once in the constructor based on the static tuningEnabled flag. Any TunableNumber constructed before enableTuning() runs (e.g., a static final gain field on a subsystem class initialized during class loading, ahead of Robot's constructor) permanently loses live-tuning capability, silently.
🐛 Suggested fix: lazily create the entry on first access
public TunableNumber(String key, double defaultValue) {
this.defaultValue = defaultValue;
- if (tuningEnabled) {
- entry = NetworkTableInstance.getDefault()
- .getDoubleTopic("/Tuning/" + key)
- .getEntry(defaultValue);
- entry.set(defaultValue);
- } else {
- entry = null;
- }
+ this.key = key;
}
public double get() {
- return entry == null ? defaultValue : entry.get(defaultValue);
+ if (!tuningEnabled) {
+ return defaultValue;
+ }
+ if (entry == null) {
+ entry = NetworkTableInstance.getDefault()
+ .getDoubleTopic("/Tuning/" + key)
+ .getEntry(defaultValue);
+ entry.set(defaultValue);
+ }
+ return entry.get(defaultValue);
}(entry and key become non-final fields.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/frc/excalib2/telemetry/TunableNumber.java` around lines 38 -
52, TunableNumber currently latches tuning state in the constructor, so
instances created before enableTuning() never get a live NetworkTables entry.
Update TunableNumber to lazily create the entry on first get() access instead of
deciding in the constructor, and make entry and key non-final so the class can
initialize the /Tuning/ key path when tuning becomes enabled. Keep get() as the
single place that checks tuningEnabled and creates/uses the DoubleTopic entry.
Comment on lines
+38
to
+46
| @Test | ||
| void initialStateEntersAndRunsOnEnter() { | ||
| AtomicInteger entered = new AtomicInteger(); | ||
| StateMachine<TestState> machine = StateMachine.builder("test", TestState.IDLE) | ||
| .onEnter(TestState.IDLE, entered::incrementAndGet) | ||
| .build(); | ||
| assertEquals(TestState.IDLE, machine.getCurrentState()); | ||
| assertEquals(1, entered.get()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add coverage for whileIn bound to the initial state.
No test constructs a StateMachine with whileIn(initialState, ...) to verify the command is scheduled during construction (see the constructor-side-effect concern raised in StateMachine.java). Adding this test would confirm the current behavior and guard against regressions if it's changed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/java/frc/excalib2/statemachine/StateMachineTest.java` around lines
38 - 46, Add a test in StateMachineTest that builds a StateMachine with
whileIn(TestState.IDLE, ...) on the initial state and verifies the command is
scheduled immediately during construction, alongside the existing initial-state
assertions. Use the StateMachine.builder(...), whileIn, and getCurrentState
symbols so the new coverage clearly exercises the constructor-side effect
behavior in StateMachine.
YehudaRothstein
changed the title
…ic factories Team style preference: no static factory methods — construct objects with 'new' and call methods on them. - Gains: new Gains(), new Gains(kP,kI,kD), new Gains(kP..kA) replace Gains.empty()/Gains.pid() - MotionConstraints: 3-arg ctor = trapezoidal, 2-arg ctor = expo (+ withCruiseCap) replace trapezoidal()/expo() statics - CurrentBudget: new CurrentBudget(stator, supply) replaces of() - MechanismConfig: public constructor replaces of(); simRotationalModel / simArmModel instance methods replace MechanismSim.* factory calls in user code - StateMachine: direct construction + chainable declaration methods replace the Builder; initial state's actions fire on first tick - DriveCommands deleted: fieldCentricDriveCommand / brakeCommand / idleCommand / driveToPoseCommand now live on SwerveSubsystem - Tests + ADDING_A_SUBSYSTEM.md updated; all 9 tests green Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The wrapper is named for the abstraction it provides (a motor with its signals and control), dropping the clipped-vendor + Exca-prefix name that was inconsistent with the rest of frc.excalib2. git mv preserves history. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
YehudaRothstein
force-pushed
the
feature/lib-rebuild
branch
from
July 3, 2026 13:58
a2406ba to
47dde9e
Compare
…r doc ref LinearExtension = meters-based positional archetype (onboard MotionMagic), porting ExcaLib v1's LinearExtension: - dynamic arm-mounted gravity (kG*sin(angle)) applied as supplemental feedforward on top of the profile, via new Motor.set*PositionGoal(pos, ff) overloads; also a constant-gravity elevator mode (onboard kG) - meters-based config helpers (softLimitsMeters, toleranceMeters, simElevatorModel) + MechanismSim.elevator (WPILib ElevatorSim) - atGoal trigger, getLength/getVelocity, resetLength, DogLog telemetry - 5 unit tests (goal tracking + soft-limit clamping) ExcaCANcoder was only ever a bullet in the DESIGN.md planned layout (never implemented — fused CANcoders are handled in MechanismConfig + Mechanism); removed the stale reference. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Closes the R-18 follow-up: FOC-mode gains are amps-based and cannot come from voltage SysId. Mechanism gains sysIdQuasistaticTorque/DynamicTorque which drive TorqueCurrentFOC output using the CTRE volts-as-amps idiom (WPILib's SysId API is volts-typed): ramp 5 A/s, step 20 A, clamped by the config's torque peaks, state recorded to the hoot via SignalLogger under <name>/SysIdTorqueState. Motor.forceAmps applies torque-current output regardless of the configured control mode, so a mechanism still running VOLTAGE closed-loop can be characterized for its FOC switch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/DESIGN.md (1)
35-37: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove
waitForAll on CANivorefrom theSignalHubsummary.
SignalHubowns batch refreshes and latency compensation; CANivore time-sync belongs to the swerve drivetrain, so this row should not attribute it toSignalHub. Keep the latency-compensated position note.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/DESIGN.md` around lines 35 - 37, The SignalHub summary currently attributes CANivore time-sync responsibilities that belong elsewhere. Update the SignalHub entry in the design overview to remove the “waitForAll on CANivore” phrase, while keeping the batch refreshes and latency-compensated position description. Use the SignalHub row in the design document as the unique location to edit.
♻️ Duplicate comments (5)
src/main/java/frc/excalib2/device/Motor.java (1)
123-132: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSilent stop on
setAmps()in non-FOC mode is still unlogged.This is the same fallback flagged in a prior review on
ExcaTalonFX.java(nowMotor.java); the silent neutral fallback whencontrolMode != TORQUE_CURRENT_FOCcan mask real integration bugs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/frc/excalib2/device/Motor.java` around lines 123 - 132, The setAmps method in Motor currently falls back to neutral silently whenever controlMode is not TORQUE_CURRENT_FOC, which can hide integration issues. Update the non-FOC branch in setAmps to emit a warning or error through the existing logging path before calling talon.setControl(neutral), and include the current controlMode plus a clear note that torque current is unsupported outside FOC mode.src/main/java/frc/excalib2/mechanisms/Mechanism.java (2)
70-76: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winFollower motor still excluded from disconnect monitoring.
followerMotorconstruction changed to the newMotortype, butdisconnectedAlert(Line 97) still only checks the leadermotor.connected(). A disconnected follower goes unreported, same as previously flagged.Also applies to: 97-97
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/frc/excalib2/mechanisms/Mechanism.java` around lines 70 - 76, The follower motor is still not included in disconnect monitoring, so a disconnected `followerMotor` can be missed. Update the disconnect alert logic in `Mechanism` so the `disconnectedAlert` checks both `motor.connected()` and `followerMotor.connected()` when a follower exists, keeping the existing leader behavior while extending it to the new `Motor` follower instance.
89-107: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
periodic()still requires manual delegation instead of self-registering withPeriodicScheduler.Telemetry, the disconnect alert, and sim stepping all depend on every owning subsystem remembering to call
periodic(). As per coding guidelines, "UsePeriodicScheduler.PERIOD.MILLISECONDS_20.add(task)for tasks that must run every 20ms loop but should not be implemented as a full WPILib subsystem." This was flagged previously and remains unresolved in this segment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/frc/excalib2/mechanisms/Mechanism.java` around lines 89 - 107, Mechanism.periodic() still depends on callers to invoke it manually instead of registering itself for the 20ms loop. Move the housekeeping/telemetry/sim update work out of ad hoc delegation and have Mechanism self-register a task with PeriodicScheduler.PERIOD.MILLISECONDS_20.add(...) from the Mechanism setup path. Keep the existing logic in Mechanism.periodic() (sim.update, disconnectedAlert, DogLog logging, logExtras) but ensure it is scheduled automatically rather than requiring each owning subsystem to call it.Source: Coding guidelines
src/test/java/frc/excalib2/statemachine/StateMachineTest.java (1)
38-50: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for
whileInbound to the initial state under the new deferred-start design.
initialStateEntersOnFirstTickcoversonEnterfor the initial state but nothing verifies that awhileIncommand bound to the initial state is scheduled only on the firstperiodic()/request()call (not at construction), which is exactly the behavior theensureStarted()fix is meant to guarantee.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/frc/excalib2/statemachine/StateMachineTest.java` around lines 38 - 50, Add a test in StateMachineTest that verifies a whileIn binding on the initial state is not scheduled at construction and only starts on the first periodic()/request() via ensureStarted(). Reuse the existing StateMachine<TestState> setup with TestState.IDLE, but bind a whileIn callback for the initial state and assert no action occurs before the first tick, then it begins after machine.periodic() and does not re-trigger on subsequent periodic() calls.src/main/java/frc/excalib2/mechanisms/MechanismConfig.java (1)
153-161: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMeters-based limits/tolerance reuse the rotation-named fields — same unit-unsafety already flagged.
softLimitsMeters()andtoleranceMeters()write meter values straight intoforwardSoftLimitRotations/reverseSoftLimitRotations/toleranceRotations. This is the same generic-field-reuse pattern already flagged fortolerance(Angle)vs.VelocityMechanism.atSpeed(rotations vs. rotations/sec) — now a third physical unit (meters) is silently packed into the same "…Rotations" fields. Nothing stops a caller from mixingsoftLimits(Angle)andsoftLimitsMeters(Distance)on the same config, or from aPositionalMechanismaccidentally reading a meters-tolerance as an angle.♻️ Suggested direction
- Double forwardSoftLimitRotations = null; - Double reverseSoftLimitRotations = null; - boolean continuousWrap = false; - double toleranceRotations = 0.02; + Double forwardSoftLimitRotations = null; // rotations OR meters — same underlying units the mechanism reports + Double reverseSoftLimitRotations = null; + boolean continuousWrap = false; + double toleranceRotations = 0.02;Consider at minimum guarding against mixed usage (e.g.,
IllegalStateExceptionif bothsoftLimitsandsoftLimitsMetersare called on the same config) until the fields are properly split by archetype.Also applies to: 179-184
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/frc/excalib2/mechanisms/MechanismConfig.java` around lines 153 - 161, The meter-based APIs in MechanismConfig are reusing the same rotation-named backing fields, which allows mixed unit configuration and silent misreads. Update softLimitsMeters() and toleranceMeters() to either use separate storage for linear-mechanism values or, at minimum, add a guard that rejects mixing them with softLimits()/tolerance() on the same instance. Use the existing MechanismConfig methods softLimitsMeters, softLimits, and toleranceMeters/tolerance to locate the shared state and enforce one unit system per config.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/ADDING_A_SUBSYSTEM.md`:
- Around line 77-82: Update the Robot loop requirements section to match the
actual behavior in Robot.robotPeriodic() or explicitly label it as a future
target-state template. Refer to Robot.robotPeriodic(), ShiftUtil.update(),
robotContainer.periodic(), PeriodicScheduler.PERIOD.MILLISECONDS_20.run(),
Threads.setCurrentThreadPriority(...), CommandScheduler.getInstance().run(),
Monologue.updateAll(), and TalonFXMotor.refreshAll() so the documented order
aligns with the implementation.
In `@src/main/java/frc/excalib2/auto/Autos.java`:
- Around line 31-39: Wrap the per-auto creation in Autos.buildChooser() so one
bad AutoBuilder.buildAuto(autoName) call cannot abort the whole chooser build.
Catch exceptions around each autoName, skip only the failing auto, and report
the failure through DogLog or the existing error logging path with the auto name
and exception details. Keep SmartDashboard.putData("Auto Chooser", chooser)
reachable even when some autos fail, and preserve the default “Do Nothing”
option.
In `@src/main/java/frc/excalib2/mechanisms/Mechanism.java`:
- Around line 171-192: Add a simulation runtime guard to the torque-current
SysId path: in getTorqueSysIdRoutine and the callers sysIdQuasistaticTorque and
sysIdDynamicTorque, check RobotBase.isSimulation() before creating or running
the routine and fail fast with a clear error/exception. This should prevent
motor.forceAmps(...) from being invoked in sim and make the “Real FOC hardware
only” restriction explicit in the Mechanism SysId torque characterization flow.
In `@src/main/java/frc/excalib2/swerve/SwerveSubsystem.java`:
- Around line 84-96: The field-centric drive code uses several fully qualified
types inline instead of imports, which is inconsistent with the rest of the
class. Update SwerveSubsystem to import the referenced types used by
fieldCentricDriveCommand and the other affected methods (such as DoubleSupplier,
MathUtil, DriveRequestType, Pose2d, and TrapezoidProfile.Constraints), then
replace the fully qualified references at their use sites with the simple names.
- Around line 166-186: configureAutoBuilder leaves autoBuilderAlert stuck on and
hides the real setup error. In SwerveSubsystem.configureAutoBuilder, clear
autoBuilderAlert on the successful path before returning, and in the catch block
log the caught exception details instead of only
DogLog.logFault("AutoBuilderConfigFailed"). Keep the try/catch around
RobotConfig.fromGUISettings and AutoBuilder.configure, but surface the actual
exception message/cause so a failed retry can be diagnosed.
In `@src/test/java/frc/excalib2/mechanisms/LinearExtensionTest.java`:
- Around line 15-64: Add test coverage for the arm-mounted LinearExtension
overload by constructing LinearExtension with gravity angle/feedforward
parameters instead of only new LinearExtension(config). Use the existing
LinearExtensionTest setup to verify the feedforward path in simulation and
explicitly exercise setLengthMeters or setLength on that instance so the sim
control mode interaction is covered. Name the new test around the gravity-aware
constructor and the sim behavior, and ensure it would fail if the constructor’s
configured control mode/unit handling is wrong.
---
Outside diff comments:
In `@docs/DESIGN.md`:
- Around line 35-37: The SignalHub summary currently attributes CANivore
time-sync responsibilities that belong elsewhere. Update the SignalHub entry in
the design overview to remove the “waitForAll on CANivore” phrase, while keeping
the batch refreshes and latency-compensated position description. Use the
SignalHub row in the design document as the unique location to edit.
---
Duplicate comments:
In `@src/main/java/frc/excalib2/device/Motor.java`:
- Around line 123-132: The setAmps method in Motor currently falls back to
neutral silently whenever controlMode is not TORQUE_CURRENT_FOC, which can hide
integration issues. Update the non-FOC branch in setAmps to emit a warning or
error through the existing logging path before calling
talon.setControl(neutral), and include the current controlMode plus a clear note
that torque current is unsupported outside FOC mode.
In `@src/main/java/frc/excalib2/mechanisms/Mechanism.java`:
- Around line 70-76: The follower motor is still not included in disconnect
monitoring, so a disconnected `followerMotor` can be missed. Update the
disconnect alert logic in `Mechanism` so the `disconnectedAlert` checks both
`motor.connected()` and `followerMotor.connected()` when a follower exists,
keeping the existing leader behavior while extending it to the new `Motor`
follower instance.
- Around line 89-107: Mechanism.periodic() still depends on callers to invoke it
manually instead of registering itself for the 20ms loop. Move the
housekeeping/telemetry/sim update work out of ad hoc delegation and have
Mechanism self-register a task with
PeriodicScheduler.PERIOD.MILLISECONDS_20.add(...) from the Mechanism setup path.
Keep the existing logic in Mechanism.periodic() (sim.update, disconnectedAlert,
DogLog logging, logExtras) but ensure it is scheduled automatically rather than
requiring each owning subsystem to call it.
In `@src/main/java/frc/excalib2/mechanisms/MechanismConfig.java`:
- Around line 153-161: The meter-based APIs in MechanismConfig are reusing the
same rotation-named backing fields, which allows mixed unit configuration and
silent misreads. Update softLimitsMeters() and toleranceMeters() to either use
separate storage for linear-mechanism values or, at minimum, add a guard that
rejects mixing them with softLimits()/tolerance() on the same instance. Use the
existing MechanismConfig methods softLimitsMeters, softLimits, and
toleranceMeters/tolerance to locate the shared state and enforce one unit system
per config.
In `@src/test/java/frc/excalib2/statemachine/StateMachineTest.java`:
- Around line 38-50: Add a test in StateMachineTest that verifies a whileIn
binding on the initial state is not scheduled at construction and only starts on
the first periodic()/request() via ensureStarted(). Reuse the existing
StateMachine<TestState> setup with TestState.IDLE, but bind a whileIn callback
for the initial state and assert no action occurs before the first tick, then it
begins after machine.periodic() and does not re-trigger on subsequent periodic()
calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0a74215b-3aa9-41b4-a1c2-1e45149b8476
📒 Files selected for processing (19)
NOTICE.mddocs/ADDING_A_SUBSYSTEM.mddocs/DESIGN.mdsrc/main/java/frc/excalib2/auto/Autos.javasrc/main/java/frc/excalib2/control/CurrentBudget.javasrc/main/java/frc/excalib2/control/Gains.javasrc/main/java/frc/excalib2/control/MotionConstraints.javasrc/main/java/frc/excalib2/device/Motor.javasrc/main/java/frc/excalib2/mechanisms/LinearExtension.javasrc/main/java/frc/excalib2/mechanisms/Mechanism.javasrc/main/java/frc/excalib2/mechanisms/MechanismConfig.javasrc/main/java/frc/excalib2/sim/MechanismSim.javasrc/main/java/frc/excalib2/statemachine/StateMachine.javasrc/main/java/frc/excalib2/swerve/DriveToPose.javasrc/main/java/frc/excalib2/swerve/SwerveSubsystem.javasrc/main/java/frc/excalib2/swerve/vision/LimelightMegaTag2.javasrc/main/java/frc/excalib2/telemetry/FaultReporter.javasrc/test/java/frc/excalib2/mechanisms/LinearExtensionTest.javasrc/test/java/frc/excalib2/statemachine/StateMachineTest.java
Comment on lines
+77
to
+82
| ## 6. Robot loop requirements (already wired in the template) | ||
|
|
||
| `robotPeriodic()` must call, in order: `SignalHub.refreshAll()` → `ShiftUtil.update()` → | ||
| `CommandScheduler.run()` → `FaultReporter.poll()`. Call `SignalHub.optimizeAll()` once at | ||
| the end of robot construction, and `Telemetry.init(...)` in the robot constructor. | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Update the loop-order instructions to match Robot.robotPeriodic()
robotPeriodic() in src/main/java/frc/robot/Robot.java currently runs ShiftUtil.update() → robotContainer.periodic() → PeriodicScheduler.PERIOD.MILLISECONDS_20.run() → Threads.setCurrentThreadPriority(...) → CommandScheduler.getInstance().run() → Monologue.updateAll() → TalonFXMotor.refreshAll(). The guide should reflect that loop, or clearly mark this section as a future target-state template.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/ADDING_A_SUBSYSTEM.md` around lines 77 - 82, Update the Robot loop
requirements section to match the actual behavior in Robot.robotPeriodic() or
explicitly label it as a future target-state template. Refer to
Robot.robotPeriodic(), ShiftUtil.update(), robotContainer.periodic(),
PeriodicScheduler.PERIOD.MILLISECONDS_20.run(),
Threads.setCurrentThreadPriority(...), CommandScheduler.getInstance().run(),
Monologue.updateAll(), and TalonFXMotor.refreshAll() so the documented order
aligns with the implementation.
Comment on lines
+31
to
+39
| public static SendableChooser<Command> buildChooser() { | ||
| SendableChooser<Command> chooser = new SendableChooser<>(); | ||
| chooser.setDefaultOption("Do Nothing", Commands.none()); | ||
| for (String autoName : AutoBuilder.getAllAutoNames()) { | ||
| chooser.addOption(autoName, AutoBuilder.buildAuto(autoName)); | ||
| } | ||
| SmartDashboard.putData("Auto Chooser", chooser); | ||
| return chooser; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
One bad auto file can crash the entire chooser build (and likely robot init).
AutoBuilder.buildAuto(autoName) is called unguarded for every discovered auto. A single malformed .auto file throws, aborting the loop before SmartDashboard.putData (line 37) runs — so the chooser is never published, and the exception propagates out of buildChooser(), undermining the class's own "never crash" design goal.
🛡️ Suggested fix
for (String autoName : AutoBuilder.getAllAutoNames()) {
- chooser.addOption(autoName, AutoBuilder.buildAuto(autoName));
+ try {
+ chooser.addOption(autoName, AutoBuilder.buildAuto(autoName));
+ } catch (Exception exception) {
+ DogLog.logFault("AutoBuildFailed/" + autoName);
+ }
}(Requires a dev.doglog.DogLog import, or substitute the existing fault-reporting mechanism.)
📝 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.
Suggested change
| public static SendableChooser<Command> buildChooser() { | |
| SendableChooser<Command> chooser = new SendableChooser<>(); | |
| chooser.setDefaultOption("Do Nothing", Commands.none()); | |
| for (String autoName : AutoBuilder.getAllAutoNames()) { | |
| chooser.addOption(autoName, AutoBuilder.buildAuto(autoName)); | |
| } | |
| SmartDashboard.putData("Auto Chooser", chooser); | |
| return chooser; | |
| } | |
| public static SendableChooser<Command> buildChooser() { | |
| SendableChooser<Command> chooser = new SendableChooser<>(); | |
| chooser.setDefaultOption("Do Nothing", Commands.none()); | |
| for (String autoName : AutoBuilder.getAllAutoNames()) { | |
| try { | |
| chooser.addOption(autoName, AutoBuilder.buildAuto(autoName)); | |
| } catch (Exception exception) { | |
| DogLog.logFault("AutoBuildFailed/" + autoName); | |
| } | |
| } | |
| SmartDashboard.putData("Auto Chooser", chooser); | |
| return chooser; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/frc/excalib2/auto/Autos.java` around lines 31 - 39, Wrap the
per-auto creation in Autos.buildChooser() so one bad
AutoBuilder.buildAuto(autoName) call cannot abort the whole chooser build. Catch
exceptions around each autoName, skip only the failing auto, and report the
failure through DogLog or the existing error logging path with the auto name and
exception details. Keep SmartDashboard.putData("Auto Chooser", chooser)
reachable even when some autos fail, and preserve the default “Do Nothing”
option.
Comment on lines
+171
to
+192
| /** | ||
| * Torque-current characterization for {@code ControlMode.TORQUE_CURRENT_FOC} gains. | ||
| * | ||
| * <p>WPILib's SysId API is volts-typed, so — per the CTRE idiom — the routine's "volts" | ||
| * are <b>interpreted as amps</b> and driven as {@code TorqueCurrentFOC} output | ||
| * (ramp 5 A/s, step 20 A, clamped by the config's torque peaks). The analyzer's | ||
| * resulting "volt" gains are therefore <b>amp gains</b>: drop them straight into the | ||
| * FOC-mode {@code Gains}. Real FOC hardware only — do not run in simulation. | ||
| */ | ||
| private SysIdRoutine getTorqueSysIdRoutine(SubsystemBase subsystem) { | ||
| if (sysIdTorqueRoutine == null) { | ||
| sysIdTorqueRoutine = new SysIdRoutine( | ||
| new SysIdRoutine.Config( | ||
| Volts.of(5).per(edu.wpi.first.units.Units.Second), // 5 "volts"(=amps) per second ramp | ||
| Volts.of(20), // 20 "volts"(=amps) step | ||
| null, | ||
| state -> SignalLogger.writeString(config.name + "/SysIdTorqueState", state.toString())), | ||
| new SysIdRoutine.Mechanism( | ||
| (Voltage amps) -> motor.forceAmps(amps.in(Volts)), null, subsystem)); | ||
| } | ||
| return sysIdTorqueRoutine; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
No runtime guard against running torque-current SysId in simulation.
The javadoc explicitly warns: "Real FOC hardware only — do not run in simulation," but getTorqueSysIdRoutine/sysIdQuasistaticTorque/sysIdDynamicTorque have no check for RobotBase.isSimulation(). Since motor.forceAmps(...) bypasses effectiveControlMode() and drives TorqueCurrentFOC directly, a developer accidentally running this in sim would silently collect meaningless characterization data instead of getting an explicit error.
🛡️ Proposed guard
private SysIdRoutine getTorqueSysIdRoutine(SubsystemBase subsystem) {
if (sysIdTorqueRoutine == null) {
+ if (RobotBase.isSimulation()) {
+ throw new IllegalStateException(config.name + ": torque-current SysId is real-FOC-hardware only");
+ }
sysIdTorqueRoutine = new SysIdRoutine(📝 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.
Suggested change
| /** | |
| * Torque-current characterization for {@code ControlMode.TORQUE_CURRENT_FOC} gains. | |
| * | |
| * <p>WPILib's SysId API is volts-typed, so — per the CTRE idiom — the routine's "volts" | |
| * are <b>interpreted as amps</b> and driven as {@code TorqueCurrentFOC} output | |
| * (ramp 5 A/s, step 20 A, clamped by the config's torque peaks). The analyzer's | |
| * resulting "volt" gains are therefore <b>amp gains</b>: drop them straight into the | |
| * FOC-mode {@code Gains}. Real FOC hardware only — do not run in simulation. | |
| */ | |
| private SysIdRoutine getTorqueSysIdRoutine(SubsystemBase subsystem) { | |
| if (sysIdTorqueRoutine == null) { | |
| sysIdTorqueRoutine = new SysIdRoutine( | |
| new SysIdRoutine.Config( | |
| Volts.of(5).per(edu.wpi.first.units.Units.Second), // 5 "volts"(=amps) per second ramp | |
| Volts.of(20), // 20 "volts"(=amps) step | |
| null, | |
| state -> SignalLogger.writeString(config.name + "/SysIdTorqueState", state.toString())), | |
| new SysIdRoutine.Mechanism( | |
| (Voltage amps) -> motor.forceAmps(amps.in(Volts)), null, subsystem)); | |
| } | |
| return sysIdTorqueRoutine; | |
| } | |
| /** | |
| * Torque-current characterization for {`@code` ControlMode.TORQUE_CURRENT_FOC} gains. | |
| * | |
| * <p>WPILib's SysId API is volts-typed, so — per the CTRE idiom — the routine's "volts" | |
| * are <b>interpreted as amps</b> and driven as {`@code` TorqueCurrentFOC} output | |
| * (ramp 5 A/s, step 20 A, clamped by the config's torque peaks). The analyzer's | |
| * resulting "volt" gains are therefore <b>amp gains</b>: drop them straight into the | |
| * FOC-mode {`@code` Gains}. Real FOC hardware only — do not run in simulation. | |
| */ | |
| private SysIdRoutine getTorqueSysIdRoutine(SubsystemBase subsystem) { | |
| if (sysIdTorqueRoutine == null) { | |
| if (RobotBase.isSimulation()) { | |
| throw new IllegalStateException(config.name + ": torque-current SysId is real-FOC-hardware only"); | |
| } | |
| sysIdTorqueRoutine = new SysIdRoutine( | |
| new SysIdRoutine.Config( | |
| Volts.of(5).per(edu.wpi.first.units.Units.Second), // 5 "volts"(=amps) per second ramp | |
| Volts.of(20), // 20 "volts"(=amps) step | |
| null, | |
| state -> SignalLogger.writeString(config.name + "/SysIdTorqueState", state.toString())), | |
| new SysIdRoutine.Mechanism( | |
| (Voltage amps) -> motor.forceAmps(amps.in(Volts)), null, subsystem)); | |
| } | |
| return sysIdTorqueRoutine; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/frc/excalib2/mechanisms/Mechanism.java` around lines 171 - 192,
Add a simulation runtime guard to the torque-current SysId path: in
getTorqueSysIdRoutine and the callers sysIdQuasistaticTorque and
sysIdDynamicTorque, check RobotBase.isSimulation() before creating or running
the routine and fail fast with a clear error/exception. This should prevent
motor.forceAmps(...) from being invoked in sim and make the “Real FOC hardware
only” restriction explicit in the Mechanism SysId torque characterization flow.
Comment on lines
+84
to
+96
| public Command fieldCentricDriveCommand(java.util.function.DoubleSupplier x, | ||
| java.util.function.DoubleSupplier y, | ||
| java.util.function.DoubleSupplier omega, | ||
| double maxSpeedMps, double maxOmegaRadPerSec, | ||
| double deadband) { | ||
| SwerveRequest.FieldCentric request = new SwerveRequest.FieldCentric() | ||
| .withDriveRequestType(com.ctre.phoenix6.swerve.SwerveModule.DriveRequestType.Velocity); | ||
| return applyRequest(() -> request | ||
| .withVelocityX(edu.wpi.first.math.MathUtil.applyDeadband(x.getAsDouble(), deadband) * maxSpeedMps) | ||
| .withVelocityY(edu.wpi.first.math.MathUtil.applyDeadband(y.getAsDouble(), deadband) * maxSpeedMps) | ||
| .withRotationalRate(edu.wpi.first.math.MathUtil.applyDeadband(omega.getAsDouble(), deadband) * maxOmegaRadPerSec)) | ||
| .withName("Field Centric Drive"); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Prefer imports over fully-qualified inline types.
java.util.function.DoubleSupplier, edu.wpi.first.math.MathUtil, com.ctre.phoenix6.swerve.SwerveModule.DriveRequestType, edu.wpi.first.math.geometry.Pose2d, and edu.wpi.first.math.trajectory.TrapezoidProfile.Constraints are all referenced fully-qualified in method signatures instead of being imported, inconsistent with the rest of the file which imports Rotation2d etc. directly.
🧹 Suggested cleanup
+import edu.wpi.first.math.MathUtil;
+import edu.wpi.first.math.geometry.Pose2d;
+import edu.wpi.first.math.trajectory.TrapezoidProfile;
+import com.ctre.phoenix6.swerve.SwerveModule;
+import java.util.function.DoubleSupplier;Then drop the fully-qualified prefixes at their use sites.
Also applies to: 114-122
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/frc/excalib2/swerve/SwerveSubsystem.java` around lines 84 - 96,
The field-centric drive code uses several fully qualified types inline instead
of imports, which is inconsistent with the rest of the class. Update
SwerveSubsystem to import the referenced types used by fieldCentricDriveCommand
and the other affected methods (such as DoubleSupplier, MathUtil,
DriveRequestType, Pose2d, and TrapezoidProfile.Constraints), then replace the
fully qualified references at their use sites with the simple names.
Comment on lines
+166
to
+186
| /** Wires PathPlanner's AutoBuilder to this drivetrain. Call once after construction. */ | ||
| public void configureAutoBuilder(PIDConstants translationGains, PIDConstants rotationGains) { | ||
| try { | ||
| RobotConfig config = RobotConfig.fromGUISettings(); | ||
| AutoBuilder.configure( | ||
| () -> getState().Pose, | ||
| this::resetPose, | ||
| () -> getState().Speeds, | ||
| (speeds, feedforwards) -> setControl(autoRequest | ||
| .withSpeeds(speeds) | ||
| .withWheelForceFeedforwardsX(feedforwards.robotRelativeForcesXNewtons()) | ||
| .withWheelForceFeedforwardsY(feedforwards.robotRelativeForcesYNewtons())), | ||
| new PPHolonomicDriveController(translationGains, rotationGains), | ||
| config, | ||
| () -> !AllianceFlip.isBlue(), | ||
| this); | ||
| } catch (Exception exception) { | ||
| autoBuilderAlert.set(true); | ||
| DogLog.logFault("AutoBuilderConfigFailed"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
Alert is never cleared on success; broad catch hides the real failure.
autoBuilderAlert is only ever set to true; if configureAutoBuilder is retried and succeeds it stays lit forever. Catching bare Exception also discards the actual cause (RobotConfig.fromGUISettings() throws a checked exception) — logging the message would speed up debugging a misconfigured PathPlanner GUI settings file.
🩹 Suggested fix
try {
RobotConfig config = RobotConfig.fromGUISettings();
+ autoBuilderAlert.set(false);
AutoBuilder.configure( } catch (Exception exception) {
autoBuilderAlert.set(true);
- DogLog.logFault("AutoBuilderConfigFailed");
+ DogLog.logFault("AutoBuilderConfigFailed: " + exception.getMessage());
}📝 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.
Suggested change
| /** Wires PathPlanner's AutoBuilder to this drivetrain. Call once after construction. */ | |
| public void configureAutoBuilder(PIDConstants translationGains, PIDConstants rotationGains) { | |
| try { | |
| RobotConfig config = RobotConfig.fromGUISettings(); | |
| AutoBuilder.configure( | |
| () -> getState().Pose, | |
| this::resetPose, | |
| () -> getState().Speeds, | |
| (speeds, feedforwards) -> setControl(autoRequest | |
| .withSpeeds(speeds) | |
| .withWheelForceFeedforwardsX(feedforwards.robotRelativeForcesXNewtons()) | |
| .withWheelForceFeedforwardsY(feedforwards.robotRelativeForcesYNewtons())), | |
| new PPHolonomicDriveController(translationGains, rotationGains), | |
| config, | |
| () -> !AllianceFlip.isBlue(), | |
| this); | |
| } catch (Exception exception) { | |
| autoBuilderAlert.set(true); | |
| DogLog.logFault("AutoBuilderConfigFailed"); | |
| } | |
| } | |
| /** Wires PathPlanner's AutoBuilder to this drivetrain. Call once after construction. */ | |
| public void configureAutoBuilder(PIDConstants translationGains, PIDConstants rotationGains) { | |
| try { | |
| RobotConfig config = RobotConfig.fromGUISettings(); | |
| autoBuilderAlert.set(false); | |
| AutoBuilder.configure( | |
| () -> getState().Pose, | |
| this::resetPose, | |
| () -> getState().Speeds, | |
| (speeds, feedforwards) -> setControl(autoRequest | |
| .withSpeeds(speeds) | |
| .withWheelForceFeedforwardsX(feedforwards.robotRelativeForcesXNewtons()) | |
| .withWheelForceFeedforwardsY(feedforwards.robotRelativeForcesYNewtons())), | |
| new PPHolonomicDriveController(translationGains, rotationGains), | |
| config, | |
| () -> !AllianceFlip.isBlue(), | |
| this); | |
| } catch (Exception exception) { | |
| autoBuilderAlert.set(true); | |
| DogLog.logFault("AutoBuilderConfigFailed: " + exception.getMessage()); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/frc/excalib2/swerve/SwerveSubsystem.java` around lines 166 -
186, configureAutoBuilder leaves autoBuilderAlert stuck on and hides the real
setup error. In SwerveSubsystem.configureAutoBuilder, clear autoBuilderAlert on
the successful path before returning, and in the catch block log the caught
exception details instead of only DogLog.logFault("AutoBuilderConfigFailed").
Keep the try/catch around RobotConfig.fromGUISettings and AutoBuilder.configure,
but surface the actual exception message/cause so a failed retry can be
diagnosed.
Comment on lines
+15
to
+64
| class LinearExtensionTest { | ||
| private LinearExtension extension; | ||
|
|
||
| @BeforeEach | ||
| void setup() { | ||
| assertTrue(HAL.initialize(500, 0)); | ||
| MechanismConfig config = new MechanismConfig("Test/Elevator", new CANDeviceId(59)) | ||
| .gains(new Gains(1, 0, 0), new Gains(1, 0, 0)) | ||
| .motion(new MotionConstraints(1.0, 2.0, 0)) // trapezoidal, meters/s | ||
| .rotorToMechanismRatio(10.0) // rotor rot per meter | ||
| .softLimitsMeters(Meters.of(0.0), Meters.of(1.0)) | ||
| .toleranceMeters(Meters.of(0.01)) | ||
| .simElevatorModel(DCMotor.getKrakenX60Foc(1), 10.0, 5.0, 0.02, 0.0, 1.0, 0.0); | ||
| extension = new LinearExtension(config); | ||
| } | ||
|
|
||
| @AfterEach | ||
| void teardown() { | ||
| HAL.shutdown(); | ||
| } | ||
|
|
||
| @Test | ||
| void startsWithNoGoal() { | ||
| assertFalse(extension.atGoal.getAsBoolean()); | ||
| } | ||
|
|
||
| @Test | ||
| void goalIsTrackedInMeters() { | ||
| extension.setLengthMeters(0.5); | ||
| assertEquals(0.5, extension.getGoalMeters(), 1e-9); | ||
| } | ||
|
|
||
| @Test | ||
| void goalClampsToForwardSoftLimit() { | ||
| extension.setLengthMeters(2.0); | ||
| assertEquals(1.0, extension.getGoalMeters(), 1e-9); | ||
| } | ||
|
|
||
| @Test | ||
| void goalClampsToReverseSoftLimit() { | ||
| extension.setLengthMeters(-1.0); | ||
| assertEquals(0.0, extension.getGoalMeters(), 1e-9); | ||
| } | ||
|
|
||
| @Test | ||
| void distanceOverloadMatchesMeters() { | ||
| extension.setLength(Meters.of(0.3)); | ||
| assertEquals(0.3, extension.getGoalMeters(), 1e-9); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Missing coverage for the dynamic-gravity constructor and sim control-mode interaction.
All tests use the constant-gravity constructor (new LinearExtension(config)). None exercise LinearExtension(config, gravityAngleRadians, gravityFeedforward), which would have caught the sim-forced-VOLTAGE vs. configured-FOC-amps unit mismatch flagged in LinearExtension.java. Consider adding a test that verifies the feedforward path under simulation for the arm-mounted overload.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/java/frc/excalib2/mechanisms/LinearExtensionTest.java` around lines
15 - 64, Add test coverage for the arm-mounted LinearExtension overload by
constructing LinearExtension with gravity angle/feedforward parameters instead
of only new LinearExtension(config). Use the existing LinearExtensionTest setup
to verify the feedforward path in simulation and explicitly exercise
setLengthMeters or setLength on that instance so the sim control mode
interaction is covered. Name the new test around the gravity-aware constructor
and the sim behavior, and ensure it would fail if the constructor’s configured
control mode/unit handling is wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
A ground-up rewrite of our internal library as
frc.excalib2— Phoenix-6-native, FOC-first, declarative. No robot-code changes in this PR (the migration is stacked as PR 2); ExcaLib v1 is untouched. Process deliverables included:docs/AUDIT.md→docs/RESEARCH.md→docs/DESIGN.md(approved before implementation) → library, commit by commit, build green throughout.Why (top audit findings this fixes)
Swerve→frc.robot.*)frc.excalib2never importsfrc.robot; robot values enter via configs.simModel(...)(TalonFXSimState + WPILib physics); CTRE swerve sim built inDeviceConfigs: apply + read-back verify + retry + AlertSensorToMechanismRatio/FusedCANcoder); WPILibMeasureAPIStateMachine<S>: default-deny transition table, guards, enter/while/exit, Trigger integration (unit-tested)TunableNumber;FaultReporter→ AlertsNew features (→ RESEARCH.md recommendations)
DeviceConfigsapply+verify+retryMechanismConfig+ Positional / Velocity / Roller archetypes (real+sim gains, budgets, tolerances, sim models)StateMachine<S>engine +Superstructure<G>coordinatorSwerveSubsystemon CTRESwerveDrivetrain+ PathPlanner wheel-force FF + drive/DriveToPosecommands as instance methodsLimelightMegaTag2client (orientation feed, scaled std-devs, reject gate, camera-pose transform)CurrentBudget(stator/supply/lower-time/torque peaks)FaultReporter,TunableNumber,Telemetry(DogLog+SignalLogger)CANDeviceId,AllianceFlip,ZonesTesting evidence
./gradlew buildgreen at every commit (verified before each).simulateJavaboots clean (verified in PR 2 where the library is exercised by real subsystems).build.gradlepins thetesttask to the WPILib JDK — other JDKs' bundled MSVC runtime hard-crasheswpiHal.dllon Windows.Follow-ups / known gaps
No AdvantageKit anywhere. Licenses respected; concepts from unlicensed repos (TRIGONLib, MAutils) re-implemented from scratch — see
NOTICE.md.🤖 Generated with Claude Code
Update: API style pass
Per team preference, the library has no static factory methods — objects are constructed with
newand configured via chainable instance methods (new Gains(...),new MotionConstraints(...),new CurrentBudget(...),new MechanismConfig(...)withsimRotationalModel/simArmModel,new StateMachine<>(...)with no builder, drive commands asSwerveSubsysteminstance methods). All 9 unit tests updated and green.Summary by CodeRabbit
New Features
Bug Fixes
Documentation