Update Commands.java Add looseSequence() method

[tom131313]

added Commands.looseSequence() Comparable to Commands.sequence() to create a SequentialCommandGroup

Each command is run individually by proxy. The requirements of each command are only for the duration of that command and are not required for the entire group process. This allows the possibility of a subsystem default command to run within the sequential group.

Note that the name looseSequence refers to a loose coupling of commands or loosening of the subsystem requirements for the duration of the group. The sequence is still determined by the order of the parameters.

[Oblarg]

Needs unit tests and ports to other languages, but I think this is clearly a good feature - maybe we should extend to the other command group types?

[Starlight220]

I'm not a fan of the naming; "loose" is loosely defined (pun intended). We already have proxyAll, so implementation should use that -- if the reduction of sequence(proxyAll(...)) to looseSequence (or whatever we name this) is significant enough to warrant another factory here.

[Oblarg]

I think this is a common enough composition to warrant it, especially because we want to be absolutely sure that if a user is attempting a loose sequence, they reliably proxy all of the commands in the sequence. The biggest proxy footgun is the potential self-cancel, and this cannot encounter that. Also, looseSequence(...) (while vague) is a better problem-space identifier than sequence(proxyAll(...)) imo.

I like "loose", personally, but am flexible on naming.

Agree on using proxyAll for implementation (forgot it existed).

[Starlight220]

proxySequence?

[Oblarg]

I think I prefer loose to proxy; neither is perfect. I'll let others weigh in.

[KangarooKoala]

Personally, I would be okay with loose, but I would prefer proxy since loose doesn't immediately come to mind and proxy is already used in commands.

[tom131313]

We can play with other words to describe the loose proxy situation.

Root words or derivatives that may fit better such as independent releasing non-blocking unblocked separated unconstrained unwrap all with sequence

[Oblarg]

@tom131313 I like separatedSequence a lot!

[tom131313]

This should have an override for Set<Command> as well

It looks like in the current WPILib only the defer/DeferredCommand has Set. Is that parameter type being added to SequentialCommandGroup (and others) too?

[spacey-sooty]

This should have an override for Set<Command> as well

It looks like in the current WPILib only the defer/DeferredCommand has Set. Is that parameter type being added to SequentialCommandGroup (and others) too?

Huh interesting I would say they should be or defer should have it removed for consistencies sake but thats a seperate pr problem

[Gold856]

DeferredCommand accepts a Set<Subsystem> to prevent teams from forgetting about requirements. No command in WPILib accepts a Set<Command>, so var-args is fine.

[KangarooKoala]

Overall, good work!

Just don't want us to forget that this still needs unit tests and ports to other languages (including robotpy/robotpy-commands-v2 after this is merged). I'm also still a little iffy about the name- People might think there's some time gap between the commands- but it's still pretty good. The paragraph in the doc comment helps a ton, though- For the (few) people that do read it, there isn't any ambiguity. It might be nice though to add a brief note in the first sentence of the doc comment since some things only show the first sentence: (This particular image is from a local javadoc build in the list of available commands- I don't remember what most IDEs display on their tooltips)

We already have proxyAll

That's in #5606, which isn't merged yet.

[Oblarg]

Re: naming, we could also try disjointSequence. A bit more formal/math-y, but also fewer timing connotations

[Starlight220]

How about proxyingSequence (on the weight of repeatingSequence)?

I think it's better to not bring more terms into the mess when we have plenty of terms already. The relation to proxy and sequence needs to be clear.

[tom131313]

How about proxyingSequence (on the weight of repeatingSequence)?

I think it's better to not bring more terms into the mess when we have plenty of terms already. The relation to proxy and sequence needs to be clear.

We do already have a good term that we can use but in a negative way. So another name suggestion and cautionary tale:

Commands can be executed in a sequence by a few ways such as a command triggers another command or timing paces a sequence such as press button "A" to schedule a command then press button "B" to schedule the next command or watch the clock to schedule a command then later on the clock schedule the next command.

The SequentialCommandGroup (sequence) was introduced to both pace commands one after another AND bestow special group properties regarding the default command blocking and the interrupt behavior of the group as a whole. This special sequencing was correctly name SequentialGroupCommand but the sequence name was incorrect and should have been called groupSequence. We may have to live with that misnomer but we can call the new, pure sequence ungroupedSequence.

The cautionary tale, of course, is the interrupt behavior of the ungrouped (being paced the same as if chained by triggers or clocking) is profoundly different than the group behavior. I've start testing the parallel version and that is difficult but seems to require the same caution - the group interrupt behavior is a lot different than not grouped.

[tom131313]

add a brief note in the first sentence of the doc comment since some things only show the first sentence

I did this. I could have removed the <p> as that seems to do the same thing in VSCode but I assumed you meant more words were needed before the cr/lf. And you'll notice a major overhaul to the whole method.

[tom131313]

maybe we should extend to the other command group types

I tried an ungroupedParallel even though it seemed dangerous to me. It was worse than I thought and even my simple examples didn't work right. I might have misunderstood. What did you mean by other command group types?

[Oblarg]

@Starlight220 While I agree we shouldn't add too many new identifiers, I think a single new one here is appropriate because of the fundamentally abstract/complicated nature of proxy.

The ideal situation would be one where disjointSequence (or whatever we call it) will hopefully be usable by new users without needing to do a deep dive into proxyCommand et al. It could be that this is not a reasonable to expect, and we'd be better off just referring people to the deeper documentation and committing to a consistent (if very solution-space-oriented) name. But when possible I like to replace solution-space terms with "friendlier" problem-space terms, and I think this is a good candidate.

It's a matter of weighing discoverability against absolute consistency.

@tom131313 I did indeed mean parallel compositions - particularly the ones other than race where the commands do not all end at the same time.

[tom131313]

parallel compositions

The problem was if the two (or more) parallel commands have a subsystem in common, then the results are bizarre, unexpected and seemingly wrong. I can add my ungroupedParallel (or disjointParallel) if we can live with expecting the user nearly perfectly keeping the same subsystem off more than one branch of a Parallel. I feel pretty sure my students will fail this test. I see no simple way to help the students succeed with this requirement.

[Oblarg]

@tom131313 You could enforce the disjoint requirements at construction-time, probably? This also suggests disjoint is the right naming paradigm.

We also should probably enforce that the proxied commands all have at least one requirement for all of these, or else people might double-compose them and that can break badly.

[tom131313]

enforce the disjoint requirements

disjointSequence() and disjointParallel() are committed. I'll do repeatedly and deadline next. At first glance it's not obvious to me why race couldn't or shouldn't have the same disjoint treatment with proxyAll.

[tom131313]

parallel compositions

Adding the proxyALL to the parallel construction including deadline adds surprisingly little value, I think. If the commands are the simplest possible - single command and not a composite - then the proxy works to allow the default command to activate before the end of the parallel.

Otherwise a composite command has to be wrapped in the disjointSequence because the proxy doesn't propagate and the default isn't going to activate without a proxy all the way done a nest.

And if the disjointSequence is used to warp the parallel commands, no further proxy is needed and parallel works exactly the same as disjointParallel.

We can cover the case of adding proxy to a simple command for parallel but it seems like a lot of complication for a possibly rarely used instance. And it may lead people to believe any command including any composite will be handled which it will NOT be. Composite commands have to be further wrapped by the user and that is far from obvious (took me a few hundred test cases to figure this out).

I see my comments on the parallel being of less utility than expecetd apply to sequence so I'm in favor of abandoning new code and then write an article describing how to get groups to honor the default command. I have test cases for good examples to share.

[tom131313]

The RepeatCommand has a fatal race (it always loses) with the ProxyCommand that is being used to disjointSequence so the repeated results are wrong. I added a delay to the RepeatCommand between the initialize and the execute to "prove" my conjecture. Seems to work but I have no idea what I'm doing so this is a case of testing-in quality which, of course, is frowned upon.

Next step?

  @Override
  public void execute() {
    if (m_ended) {
      m_ended = false;
      m_command.initialize();
      m_delayFirstExecuteIterations = 0; //<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
    }
    if (m_delayFirstExecuteIterations < 3) { //<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
            m_delayFirstExecuteIterations++; //<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
            return; //<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
        }//<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
    m_command.execute();
    if (m_command.isFinished()) {
      // restart command
      m_command.end(false);
      m_ended = true;
    }
  }

[KangarooKoala]

The RepeatCommand has a fatal race (it always loses) with the ProxyCommand that is being used to disjointSequence so the repeated results are wrong.

Could you elaborate more on this? (What is the code being run (including any added or changed method definitions), what is the expected behavior, and what is the actual behavior?)

[tom131313]

The RepeatCommand has a fatal race (it always loses) with the ProxyCommand that is being used to disjointSequence so the repeated results are wrong.

Could you elaborate more on this? (What is the code being run (including any added or changed method definitions), what is the expected behavior, and what is the actual behavior?)

This or equivalents fail: repeatingSequence(cmd1.asProxy(), cmd2.asProxy(), cmd3.asProxy()); Expected to run cmd1 until finished then run cmd2 until finished and then run cmd3. That is the result for the first time through but on the repeat the result is cmd1 and cmd2 run together and then when finished cmd3 runs.

Maybe it happens because the Proxied command (say cmd1 or cmd2) isn't quite ready to respond yet or it takes an extra iteration for that status to flow back through ProxyCommand to RepeatCommand for when the RepeatCommand checks its status. There isn't quite enough time for the repeat between the initialize() and the execute() for things to work right.

[KangarooKoala]

Thanks for noticing that bug! It turns out to be a fairly involved bug, and I think the cleanest fix would be #6593.

This is what happens during the call to RepeatCommand.execute() as it repeats:

• (1) SequentialCommandGroup.initialize() is called, during which:
  • (1a) ProxyCommand(cmd1).initialize() is called.
  • This schedules cmd1, but the schedule is deferred because we're in the command scheduler run loop.
• (2) SequentialCommandGroup.execute() is called, during which:
  • (2a) ProxyCommand(cmd1).execute() is called, which does nothing.
  • (2b) ProxyCommand(cmd1).isFinished() is checked and returns true, causing steps 2c and 2d to occur.
  • isFinished() checks !cmd1.isScheduled(), which is true because the schedule was deferred (so the command wasn't scheduled yet).
  • (2c) ProxyCommand(cmd1).end(false) is called, which does nothing.
  • (2d) ProxyCommand(cmd2).initialize() is called.
  • This schedules cmd2, but this schedule is also deferred because we're in the command scheduler run loop.
• (3) SequentialCommandGroup.isFinished() is checked and returns false.

Note that both cmd1 and cmd2 were queued to be scheduled, leading to the behavior you saw (cmd1 and cmd2 running simultaneously).

[tom131313]

We also should probably enforce that the proxied commands all have at least one requirement for all of these, or else people might double-compose them and that can break badly.

I added this check for restriction by changing the proxyAll() (one of which I had found in github). I included my modified version in this PR Commands.java. (deadline() had to have a tweak, also.) Possibly proxyAll() should always have the check for subsystem requirements. If not, then the version I put in Commands.java could be renamed to something like proxyAllWithRequirements() and, of course, change the new disjoint methods to reference proxyAllWithRequirements().

I believe this satisfies all the comments on this PR except possibly the validation scheme that registers then unregisters composed commands. Changing that is a daunting task for me to do elegantly so I'll leave that for others again to decide how to do it.

I think a complete set of Java disjoint methods is provided and they work as I have tested them. This should give you a good idea if you want to pursue including them in WPILib.

[The repeatingDisjointSequence is not well implemented pending correction of the underlying command class.]