Mechanical-Advantage / AdvantageKit

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

Unhandled ConcurrentModificationException in LogTable.clone method #72

Open steve-carpenter opened 7 months ago

steve-carpenter commented 7 months ago

It appears that this line is occasionally (1 in 5? times) causing a ConcurrentModificationException on initialization. Robot will crash with fatal error, sometimes restart several times until it works.

Stack trace shows: LoggedRobot.startCompetition -> Logger.periodicAfterUser -> LogTable.clone

I'm assuming that that LogTable object passed into the method is being modified while being cloned- which is causing the issue. Does not appear to be happening in simulation. Code is verbatim from example projects & running the latest release.

https://github.com/Mechanical-Advantage/AdvantageKit/blob/5a047d936bfb2a3c7c09b26628114cb19d711731/junction/core/src/org/littletonrobotics/junction/LogTable.java#L100

jwbonner commented 7 months ago

I've never observed this issue on any AdvantageKit example project. Is the code you're running completely unmodified or was it adapted to run on your hardware? The example projects aren't in a state to run on a real robot out of the box. Can you link to the code where you're seeing this happen?

steve-carpenter commented 7 months ago

We've based our code on this example: https://github.com/Mechanical-Advantage/AdvantageKit/tree/main/example_projects/swerve_drive - by that, I mean- we have same configuration in Robot.java & use the AutoLog annotation similarly for our subsystem IO.

steve-carpenter commented 7 months ago

Here is the code snippet from Robot.java

 @Override
    public void robotInit() {
        CrashTracker.logRobotInit();
        try {
            Logger.recordMetadata("Team", "2024-Robot"); // Set a metadata value
            switch (getRuntimeType()) {
                case kRoboRIO, kRoboRIO2: {
                    Logger.addDataReceiver(new WPILOGWriter("/home/lvuser/logs/"));
                    Logger.addDataReceiver(new NT4Publisher()); // Publish data to NetworkTables
                    powerDistribution = new PowerDistribution(1, PowerDistribution.ModuleType.kRev);
                    break;

                }
                case kSimulation: {
                    setUseTiming(false); // Run as fast as possible
                    Logger.addDataReceiver(new NT4Publisher()); // Publish data to NetworkTables
                    break;
                }
            }

            Logger.start();

            drivetrain = Drivetrain.getInstance();
            launcher = Launcher.getInstance();
jwbonner commented 7 months ago

Are you using threads anywhere in your code?

steve-carpenter commented 7 months ago

We're following 254's subsystem manager architecture- so we technically have threads for these Enabled/Disabled loops . Our Logger.processInputs method calls would be called from within the Loop class (thread)

steve-carpenter commented 7 months ago

Subsystems don't get initialized until after Logger.start() in Robot.java FWIW

jwbonner commented 7 months ago

This could definitely be caused by calling Logger methods (especially processInputs) from separate threads. However, the more fundamental issue is that using separate threads for robot logic is not compatible with AdvantageKit since they cannot be replayed in simulation. There are more details in the documentation here. I would recommend either using a single-threaded architecture for subsystems (like WPILib uses by default), or switching to a non-replay logging framework which will probably be a better fit for your needs (particularly WPILib's built-in logging, which can be paired with Monologue for annotation support).

Based on the snippet you posted from Robot, it doesn't look like you've configured AdvantageKit to run log replay. If this is the case, WPILib's built-in logging should cover all of your needs since log replay is the explicit purpose of AdvantageKit.