Improper documentation for SysId test state

[binex-dsk]

While there is a slight mention of the general format of the SysId test states for logging, the names are not explicitly mentioned or explained. The only way we found the proper names for the test-mode log variable was through a screenshot of the log analysis tool itself:

The names of each of the test modes should be explicitly documented here.

[jasondaming]

Were you intending to have a link in the last sentence?

Why do you think the names need to be documented? General use of SysID doesn't require knowing the names, see this example. Are you writing your own new SysIdRoutine clone? If so that seems beyond the scope of the general documentation and more something that looking through the code should be an expectation.

[binex-dsk]

I wasn';t intending on putting down a link. We were using a custom SysId routine with CTRE logging. Paging @gabeStuk he will have more info.

[gabeStuk]

Based on the documentation given and the testing I did, loading a log into sysid with 3rd party logging requires a method passed into the SysIdRoutine.Config() to log the name of the test. The names quasistatic-forward, quasistatic-backward, dynamic-forward and dynamic-backward were unintuitive and it took me a long time to figure out that the SysIdRoutineLog.State enum values' names were not the string that needed to be logged. Therefore, I'd love either the documented demonstration or implementation of an enum method to convert between the SysIdRoutineLog.State value from the Config callback to the string that the sysid application uses to tell what test the dats is from.

For context, here is the SysIdRoutine logging method I am talking about. We used CTRE's SignalLogger:

private SysIdRoutine sysIdRoutine = new SysIdRoutine(
            new SysIdRoutine.Config(null, null, null, this::logSysIDState),
            new SysIdRoutine.Mechanism(
                    volts -> setControl(voltRequest.withVoltage(volts.in(Units.Volts))),
                    null,
                    this));

    private void logSysIDState(State state) {
        if (state != State.kNone) {
            switch (state) {
                case kDynamicForward:
                    SignalLogger.writeString("test-mode", "dynamic-forward");
                    break;
                case kDynamicReverse:
                    SignalLogger.writeString("test-mode", "dynamic-reverse");
                    break;
                case kQuasistaticForward:
                    SignalLogger.writeString("test-mode", "quasistatic-forward");
                    break;
                case kQuasistaticReverse:
                    SignalLogger.writeString("test-mode", "quasistatic-reverse");
                    break;
                default:
                    break;
            }
        }

Something like state.getTestName(), or a part of the docs showing code similar to this to demonstrate that you can't use state.name() and log "kQuasistaticForward" is what we're asking for.

[sciencewhiz]

See wpilibsuite/allwpilib#6273 for related issue

[sciencewhiz]

Something like state.getTestName(), or a part of the docs showing code similar to this to demonstrate that you can't use state.name() and log "kQuasistaticForward" is what we're asking for.

Doesn't state.toString() work?

[gabeStuk]

Ah yes, you are correct

[gabeStuk]

My entire complaint is nullified

[gabeStuk]

Other than that the docs should say that