pidIdx and timeoutMs should have default values

[andrewda]

It's ugly to specify a pidIdx and a timeoutMs every time I want to do something like configSelectedFeedbackSensor() or getSelectedSensorPosition(). These parameters should have default values like pidIdx = 0 (for the primary closed-loop) and timeoutMs = 0 (for no blocking or checking). If this makes sense for you guys, I can open a PR.

[JCaporuscio]

We may be able to do this for C++, but for Java the number of config* methods that would require an overloaded method makes this prohibitive.

I'm not opposed to adding it for one language and not the other, but even with a PR we likely wouldn't integrate it until the off-season.

[SUPERCILEX]

For Java, we could just add one overload apiece with 0 for pidIdx and timeoutMs, would that work?

PS: as a library maintainer myself, I think there should be a greater emphasis on consumer happiness than supplier happiness (though there needs to be a balance of course).

[ozrien]

I'm concerned if we default param timeoutMs to zero, that no one will use a non zero timeoutMs. I think using a nonzero timeoutMs is important when you config everything on robot startup, this way you will get immediate DS error messages on robot boot if a device is not connected/powered. It also helps level network utilization as you config all your devices in your init routines.

But having nonzero timeoutMs for configs in your periodic loops should be avoided since it is less likely to have a device lose power/connectivity during a match, and you can't service the robot mid-match anyway. There is no advantage to blocking config calls because of this. And the general goal is to do all your Config*s on startup, since they are persistent.

Because there is no way the API knows the context of the call, it's not clear to me if it is better to hide these details from inexperienced users, or to force the caller to decide what they want. By not having defaults, new users will see the params, ask "what is this for" and hopefully will read the documentation to understand how best to design their robot software.

But I could be sold on default param for timeoutMs with enough up votes.

[dkt01]

Doesn't the LabVIEW API already default the timeout to 0ms? If I recall correctly, that input is not set as required and the default value is 0. While I agree with ozrien's concern that no one will use a non-zero timeoutMs, I'd argue that consistency among the APIs is more important because this allows the documentation to apply to all languages. My preference is default of 0ms, but requiring a value is fine if that is true on all the APIs.

[andrewda]

I would definitely say that the importance of a clean and simple API far outweighs the concern of having newcomers be confused about what's going on. If the robot doesn't do what the user expects, they will read the documentation.

Another option I see is forcing the timeout to be 1000ms (or some other agreed upon value) on the first call to the Talon if the user sets it as 0 (or doesn't set it). Then accept the 0 for all future calls.

[andrewda]

Just going to bring this suggestion back to light now that we're officially in the off-season. We ended up making timeouts optional for all Phoenix methods we used in our own library, defaulting them to 0. That way we could set an initial timeout of 1000ms for our Talons and then forget about the timeout for our control loops. In the end, it made our code significantly more readable with fewer magic floating 0s or 10s in our methods.

I strongly believe that this makes a lot more sense than requiring timeouts and will result in cleaner and more readable code for everyone who has not taken on the task of making their own library.

Look at some example methods I just came up with:

fun scaleMotorValues() {
  leftMotor.set(0.3.pow(2), 10)
  rightMotor.set(-0.3.pow(2), 10)
}

vs.

fun scaleMotorValues(speed: Double) {
  leftMotor.set(0.3.pow(2))
  rightMotor.set(-0.3.pow(2))
}

Getting rid of those extra numbers at the end makes everything more clear and much less confusing when I don't really need a timeout set.

I can understand the sentiment for wanting forced timeouts in order to lessen the confusion for newcomers, but in my experience having a Talon unplugged or not getting power is usually not the issue. It's also something people can usually see pretty visibly on the robot anyways, so we don't need to make sacrifices from a code-standpoint as the robot is usually one of the first places people look. Again, that being said, I still think an optional timeout is a good idea so those people who do need them to help with debugging are free to have them, but aren't forced to use them in other parts of their code.

[ozrien]

@andrewda Your set example is strange in that set() has no timeout param. This only applies to configs (typically done on boot up) and routines that tare/zero/setup sensor values.

General update on Phoenix 5.7.

During this offseason, we made the following changes to address default values for supplemental params. This also addresses reverting un-set params to default values which is related to this issue in that it also has a default timeout value. These changes were applied in Phoenix 5.7 (also are in the release notes)

• add default timeout of '0' to all existing config* routines.
• add default timeout of '0' to all setStatusFramePeriod routines
• add default timeout of '0' to all setIntegralAccumulator, clearMotionProfileHasUnderrun, clearStickyFaults
• add default timeout of '0' to all get routines. Though note that anything less than 4ms is promoted to 4ms .
• add default pidIdx of '0' to existing routines
• add default slotIdx of '0' to existing routines
• introduce a new configFactoryDefault(timeout) routine that defaults all persistent configs, also default zero for timeout.
• introduce a new configAllSettings(settings, timeout). Default timeout is nonzero (testing will determine). The settings object contains all settings for the device, and constructs with default values for all params. This routine also privately uses configFactoryDefault() first before applying the settings inside settings object to ensure the Talon/Victor/whatever is restored to a clean state before applying the user's settings.

The recommendations moving forward are to use configAllSettings once on boot. Use individual config* routines during runtime if need be. Because configAllSettings has a nonzero timeout, my concern above is addressed. Because the other individual configs have a default timeout of zero, this meets the request of the OP.

Also somewhat related, for legacy applications that use a series of individual config* routines, developer can call configFactoryDefault() first to restore device to factory settings prior to individual calls. This way you never need to manually factory default a device again. This also reduces the number of configs you must call (no longer to call every single one to ensure undesired features get enabled and never disabled).

Rationale

The default value for individual config*s will be zero, as requested in this tracker. However the nonzero default timeout in configAllSettings() means that teams will get error information if the device is not on the bus. This routine should be called once on boot. This also reduces the lines of code that teams have to write, which was also the motivation.

Software documentation will be update accordingly.

[andrewda]

Ah you're right, I forgot which methods actually used a timeout as we've been abstracting it out all year.

Thanks for this update! Appreciate all the work and thought that went into this.