Mechanical-Advantage / AdvantageKit

Monorepo for robot framework and tools
GNU General Public License v3.0
152 stars 42 forks source link

Startup exception with sparkmax odometry thread. Index Out of bounds. #77

Closed cosmosified closed 6 months ago

cosmosified commented 6 months ago

i don't have the log here. Our team ran into an issue today with the advanced swerve template with the new invalid value fix.

in SparkmaxOdometryThread:: periodic, it used to iterate over signals and timestampques. now it currently indexes over the size of teh signals, and will throw an index out of bounds exception when trying to offer to the timestamp queue that doesn't exist at index xxxxx. (i believe we kept seeing 4)

We're going to test the following code tomorrow.

private void periodic() { Drive.odometryLock.lock(); double timestamp = Logger.getRealTimestamp() / 1e6; try { for (int i = 0; i < signals.size(); i++) { int finalI = i; signals.get(i).get().ifPresent(signal -> { queues.get(finalI).offer(signal); if (finalI < timestampQueues.size()) timestampQueues.get(finalI).offer(timestamp); }); } } finally { Drive.odometryLock.unlock(); } }

jwbonner commented 6 months ago

Thanks for finding that, I'll fix it shortly. Keep in mind that the code you posted will not guarantee that the queues receive the same number of samples, which is likely to cause exceptions in other parts of the code.

cosmosified commented 6 months ago

yeah i was wonderring about that. Looking at all this, the signaling code using an integer to index on multiple separate queuses..sems odd. since the timestamps are hardcoded for whent he periodic fires.. i was thinking that possibly pairing the data together would help that..

ie Queue<Tuple2<Signal,Timestamp>> .

cosmosified commented 6 months ago

got the stack trace from our programmers today Error at java.base/jdk.internal.util.Preconditions.outOfBounds(Unknown Source): Unhandled exception in Notifier thread: java.lang.IndexOutOfBoundsException: Index 4 out of bounds for length 4 at java.base/jdk.internal.util.Preconditions.outOfBounds(Unknown Source) at java.base/jdk.internal.util.Preconditions.outOfBoundsCheckIndex(Unknown Source) at java.base/jdk.internal.util.Preconditions.checkIndex(Unknown Source) at java.base/java.util.Objects.checkIndex(Unknown Source) at java.base/java.util.ArrayList.get(Unknown Source) at frc.robot.subsystems.drive.SparkMaxOdometryThread.periodic(SparkMaxOdometryThread.java:100) at edu.wpi.first.wpilibj.Notifier.lambda$new$0(Notifier.java:124) at java.base/java.lang.Thread.run(Unknown Source)

jwbonner commented 6 months ago

I pushed a fix and updated the example project on the latest release.

The reason for handling timestamps separately is that they are synchronized between all of the signals for the purpose of the odometry calculation. There are many ways to structure this to log the timestamps as inputs, and the example projects show one possibility. You are welcome to adjust the structure of the code from this starting point, but we may not be able to help with debugging.

cosmosified commented 6 months ago

@jwbonner no worries. I appreciate the quick work