[commands] Replace throwing default command validation with warning

[Starlight220]

There could be use cases for it, and the validation is definitely not worthy of crashing the code -- very few things are.

[sciencewhiz]

What's the use case for this? If you want a command to always run that doesn't require a subsystem, can't you just start it?

Typically we throw for config issues and warn for runtime issues, and this seems to be the former.

Given loop overrun prints, we have to assume that warnings won't be seen, and this seems more likely to hide problems then open use cases to me.

[virtuald]

This seems like a bad idea, aren't there assumptions elsewhere that this would break?

[Oblarg]

This doesn't break anything and the amount of schedule overhead is small. The cost of a team accidentally crashing on the field is large.

[PeterJohnson]

Without a use case, I'm not seeing the value in removing validation that to date has not been an issue for teams.

[sciencewhiz]

This doesn't break anything and the amount of schedule overhead is small. The cost of a team accidentally crashing on the field is large.

I don't understand this. As it would crash on startup, it would only affect a team on the field that majorly restructured their code and deployed in queue right before putting the robot on the field.

[ThadHouse]

Also, people already don't read the console. At least if code is crashing it will either get found and fixed quickly, posted by the team asking for help, or getting CSA help at an event. Warnings are just going to be ignored until they cause a real problem.

[Starlight220]

I don't understand this. As it would crash on startup, it would only affect a team on the field that majorly restructured their code and deployed in queue right before putting the robot on the field.

A few things:

majorly restructured

From experience, changing default commands is a fairly small change.

deployed in queue right before putting the robot on the field

Is deploying in queue something we want to punish teams for?

Given that for most teams competition is a stressful time full of rushed development, I think that crashing code over not-necessarily-wrong behavior isn't a fitting reaction. As for the "mistakes" this validation guards against, let's see what would happen if they weren't caught:

• (command doesn't require subsystem): command would be continuously scheduled while it runs; this is not that different from any other requirement issue, is basically what whileActiveContinuous did, and tbh there would be very little difference from correct behavior.
• (command finishes immediately) the command would be scheduled and end every iteration; exactly the same as whileActiveContinuous(InstantCommand). This isn't wrong behavior! If already, this would be a very intuitive way to stop motors etc. In any case, this is UB.
• (command is not interruptible) this might be the most problematic of them, but still not so bad: it would "lock" the subsystem, not allowing any other command to be scheduled on it. This isn't very terrible behavior, and imo fairly easy to diagnose. It also is something that can be achieved rather easily through other means (which we do not validate).

And now let's compare to the other times we throw:

• resource conflicts: (on motor controllers) this causes massively disruptive behavior, has hardware implications that can possibly cause injury or other damage, and no way for this to be correct behavior or recover from it.
• NPEs (Java): this is more of a language issue than anything else, and it's such a common mistake outside of robotics that it's expected.

I'm of the opinion that the validation here should be removed completely, but I'm fine with warning on the non-UB checks to help debugging.

[PeterJohnson]

Isn’t allowing it near certain to cause a resource conflict though? Since running any other command on the same subsystem would instantly conflict.

[Starlight220]

What are you referring to? Wrongly-managed requirements are a general problem which we have no way to enforce; checking (and throwing!) here feels very out of place and disproportionate.

[virtuald]

Is deploying in queue something we want to punish teams for?

In short, yes.

Teams really shouldn't be deploying code in queue without testing it. These sorts of errors can be easily caught in simulation.

[Starlight220]

I agree with the idea of "teams shouldn't deploy in queue", but I argue that it's not always practical and shouldn't be driving design decisions. However, I think that's an agree-to-disagree situation and I don't think that delving into it as part of this PR would be productive.

---

To avoid going in circular discussion, let's do this differently, step by step. I see that the objections are circling around the requirements check, so we can get back to it later.

First, do we agree to remove the isFinished check? Calling isFinished before initialize is unsafe and has undefined semantics, and violations of the check (ie default commands that end immediately) aren't wrong behavior.

(react with :+1: to agree, comment with objections)

[ThadHouse]

I’m more inclined to agree with that, because I do agree that could cause issues, but that should be a separate pr. We also need to figure out what breaks if a default command does finish, because I suspect there might be some things in the scheduler that are not to happen when that happens.

As for the requirements change, it’s not really going in circle, most of us disagree with it. It’s a check that definitely should be there, and an explicit crash, because nobody checks for warnings anyway and the just get ignored.

[Starlight220]

I'm reducing this PR to only remove the isFinished check. This doesn't break anything, the default command will simply be scheduled again in the following iteration.

[virtuald]

I agree with the idea of "teams shouldn't deploy in queue"

That isn't what I said. I said "Teams really shouldn't be deploying code in queue without testing it". These cases can be hit in simulation.

I offer a middle ground:

Because it's so easy to accidentally crash in Python, in RobotPy we have some places where we will check to see if the FMS is connected when something unexpected occurs. If the FMS is connected, we emit a warning. If the FMS is not connected, we crash the program so that the user notices it -- since as Thad points out, everyone ignores warnings.

However, as I stated earlier, I'm not sure what the ramifications are of allowing this obviously incorrect code to continue executing on the field, so I'm not sure this middle ground is a good idea either (but keep it in mind for the future).

[Starlight220]

I think that middle ground solution should be introduced at a larger scope and not here.

I've reduced the scope of this PR to only remove the isFinished check. There don't seem to be any objections to that, so can we merge this? We can discuss the requirements check at another time.