[sysid] Failure to analyze AdvantageKit log files

[jwbonner]

The attached log file was produced using AdvantageKit in simulation, with very minor noise added to the voltage samples to ensure reasonable sampling for the dynamic tests (even with duplicate values removed). Trying to run the SysId analysis using the fields below produces the message "Not enough data to run Acceleration Calculation..." on Windows and causes the application to crash on macOS (not tested on Linux).

• Test state: /RealOutputs/SysIdState
• Velocity: /Drive/LeftVelocityRadPerSec
• Position: /Drive/LeftPositionRad
• Voltage: /Drive/LeftAppliedVolts
• Config: simple, radians, velocity/position scaling of 1

The console log for the macOS crash shows:

INFO: Preparing data (Analyzer.cpp:263)
INFO: Preprocessing raw data. (AnalysisManager.cpp:119)
INFO: Copying raw data. (AnalysisManager.cpp:121)
zsh: segmentation fault  /Users/jonah/wpilib/2024/tools/SysId.app/Contents/MacOS/sysid

SysId_AdvantageKit.wpilog.zip

Running with WPILib 2024.1.1 on Windows 10 and macOS 13.5.

[PeterJohnson]

Other than the datalog loading functionality, it appears the SysIdState in this file doesn't use the correct naming. What the tool expects is lowercase-names (e.g. quasistatic-forward, dynamic-reverse), but what's in the file is kQuasistaticForward, kDynamicReverse.

[jwbonner]

Thanks, switching it to the correct names seems to have solved the issue. One aspect which is slightly confusing is that the recordState consumer accepts a State enum rather than a string. I was inadvertently logging the enum directly, which AdvantageKit records using the name (to support replaying it from the log). More generally, someone who accidentally uses .name() instead of .toString() would encounter the same problem.

In the interest of avoiding API changes at this point (like making it a string consumer), I think it would be helpful to have SysId not display any runs with an invalid name. Seeing the list of four runs with a count of samples for each made it appear that the runs were identified correctly (and of course, any sort of feedback beyond crashing would be good on macOS).

BTW, I think this issue is unrelated to #6193. This one involves logs produced directly by AdvantageKit, while that issue is about logs exported by AdvantageScope. Looks like we never opened a corresponding issue for that one.

[jwbonner]

Looking at this a bit more, it seems like the issue is here. Any test state called "none" is rejected, but everything else is accepted by default. It seems like this should be the other way around; only valid test states should be accepted.

[PeterJohnson]

@Oblarg any thoughts here?

[Oblarg]

For now we should reject anything that doesn't have the standard naming. We can become more flexible later when we improve the fit.

[PeterJohnson]

Ok. Currently there's just the 4 valid names? (quasistatic and dynamic / forward and reverse combinations)

[Oblarg]

And none, yeah.