[cmd] Add Commands.choose (aka SendableChooserCommand)

[Starlight220]

No description provided.

[Alextopher]

I implemented something similar recently.

The Consumer<Sendable> rubs me the wrong way (so does hard coding SmartDashboard like I did). It's a good way to add this change within the existing APIs. I'm just not a fan of how this singular idea SendableChooserCommand requires putting 2 things onto a Dashboard for complete functionality.

Command chooser = Commands.choose(
   chooser -> SmartDashboard.putData("Drive Command Chooser", chooser),
   tankDrive, arcadeDrive, curvatureDrive
);
SmartDashboard.putData("Drive Command", chooser);

I wonder if this change could increase in scope and if SendableChooserCommand could be a new, unique, widget (setSmartDashboardType()), on the same footing as "Command" or "String Chooser".

var chooser = new SendableChooserCommand(tankDrive, arcadeDrive, curvatureDrive);
// Does nothing unless sent to a Dashboard
SmartDashboard.putData("Drive Command", chooser);

[Starlight220]

Why do you need to publish the command to the dashboard?

[Alextopher]

Dashboards support starting and stopping commands that you upload.

https://docs.wpilib.org/en/stable/docs/software/dashboards/shuffleboard/advanced-usage/shuffleboard-commands-subsystems.html#displaying-commands

https://docs.wpilib.org/en/stable/docs/software/dashboards/glass/command-based-widgets.html

[Starlight220]

Sure. That's separate functionality, and isn't needed for SendableChooserCommand.

The primary use case for SendableChooser is selecting a command for auto, and the SendableChooser needs to be published for selections to be possible; there's no need (and practically no benefit) to put the command on the dashboard as well.

[KangarooKoala]

#7340 might be resolved by this, though the discussion of the impl in that issue is different from the impl in this PR. (Hence why I'm avoiding the auto-linking words for now)

[Starlight220]

Wow I forgot I wrote this. This PR does what I was thinking of in that issue.

Is this missing something?

[Daniel1464]

I think there should be an overload that accepts a string and a command vararg in addition to the sendable chooser lambda overload.

[Starlight220]

What would that string be used for, and what's the motivation for that overload?

[Daniel1464]

What would that string be used for, and what's the motivation for that overload?

Commands.choose(
      "AutoCommandChoice",
       commands
)

Commands.choose(
      (chooser) -> SmartDashboard.putData("AutoCommandChoice", chooser),
      commands
)

[Starlight220]

It shouldn't be coupled to SmartDashboard.

Accepting an absolute NT key would be good, though.

[KangarooKoala]

There doesn't appear to be a standard way to add a sendable to an arbitrary NT key, unfortunately. Maybe fix the merge conflicts for this PR and make an issue to add that capacity later?

[rzblue]

The c++ version needs a test- it doesn't compile currently.

[rzblue]

This doesn't compile

[KangarooKoala]

Tried fixing this locally, and while some issues could be fixed, a fundamental issue is that SelectCommand requires a std::function (which must be copyable), but SendableChooser is not copyable, so the lambda storing it can't be copyable either.

[Starlight220]

shared_ptr it?

Thanks for looking into this!
Apparently the template errors were very inaccurate.

[KangarooKoala]

Here's a patch that makes tests pass:

diff --git a/wpilibNewCommands/src/main/native/include/frc2/command/Commands.h b/wpilibNewCommands/src/main/native/include/frc2/command/Commands.h
index 7dea6f2c9..787497590 100644
--- a/wpilibNewCommands/src/main/native/include/frc2/command/Commands.h
+++ b/wpilibNewCommands/src/main/native/include/frc2/command/Commands.h
@@ -183,14 +183,14 @@ template <std::convertible_to<CommandPtr>... CommandPtrs>
 [[nodiscard]]
 CommandPtr Choose(std::function<void(wpi::Sendable*)> publish,
                   CommandPtrs&&... commands) {
-  frc::SendableChooser<std::string_view> chooser;
-  ((void)chooser.AddOption(commands.GetName(), commands.GetName()), ...);
-  publish(&chooser);
-  return Select(
-      [sendableChooser = std::move(chooser)]() mutable {
-        return sendableChooser.GetSelected();
+  auto chooser = std::make_shared<frc::SendableChooser<std::string_view>>();
+  ((void)chooser->AddOption(commands.get()->GetName(), commands.get()->GetName()), ...);
+  publish(chooser.get());
+  return Select<std::string_view>(
+      [sendableChooser = chooser]() mutable {
+        return sendableChooser->GetSelected();
       },
-      (std::pair{commands.GetName(), std::move(commands)}, ...));
+      std::pair{std::string_view{commands.get()->GetName()}, std::move(commands)}...);
 }
 
 /**
diff --git a/wpilibNewCommands/src/test/native/cpp/frc2/command/SendableChooserCommandTest.cpp b/wpilibNewCommands/src/test/native/cpp/frc2/command/SendableChooserCommandTest.cpp
index 0d419d855..25e67eb7a 100644
--- a/wpilibNewCommands/src/test/native/cpp/frc2/command/SendableChooserCommandTest.cpp
+++ b/wpilibNewCommands/src/test/native/cpp/frc2/command/SendableChooserCommandTest.cpp
@@ -23,7 +23,7 @@ using namespace frc2;
 using SendableChooserTestPublisherFunc = std::function<void(wpi::Sendable*)>;
 using SendableChooserTestArgs =
     std::pair<std::function<frc2::CommandPtr(SendableChooserTestPublisherFunc)>,
-              std::vector<std::string_view>>;
+              std::vector<std::string>>;
 
 class SendableChooserCommandTest
     : public CommandTestBaseWithParam<SendableChooserTestArgs> {
@@ -50,7 +50,7 @@ TEST_P(SendableChooserCommandTest, OptionsAreCorrect) {
 }
 
 namespace utils {
-static const frc2::CommandPtr CommandNamed(std::string_view name) {
+static frc2::CommandPtr CommandNamed(std::string_view name) {
   return frc2::cmd::Print(name).WithName(name);
 }
 
@@ -58,17 +58,17 @@ static const std::vector<SendableChooserTestArgs> OptionsAreCorrectParams() {
   SendableChooserTestArgs empty{[](SendableChooserTestPublisherFunc func) {
                                   return frc2::cmd::Choose(func);
                                 },
-                                std::vector<std::string_view>()};
+                                std::vector<std::string>()};
 
   SendableChooserTestArgs duplicateName{
       [](SendableChooserTestPublisherFunc func) {
         return frc2::cmd::Choose(func, CommandNamed("a"), CommandNamed("b"), CommandNamed("a"));
-      }, make_vector<std::string_view>("a", "b")};
+      }, make_vector<std::string>("a", "b")};
 
   SendableChooserTestArgs happyPath{
       [](SendableChooserTestPublisherFunc func) {
         return frc2::cmd::Choose(func, CommandNamed("a"), CommandNamed("b"), CommandNamed("c"));
-      }, make_vector<std::string_view>("a", "b", "c")};
+      }, make_vector<std::string>("a", "b", "c")};
 
   return make_vector<SendableChooserTestArgs>(empty, duplicateName, happyPath);
 }

Good thinking about the shared_ptr, I completely forgot about that. Just as for posterity, here's the fixes:

• Use a shared_ptr to allow copying the lambda (necessary for use in a std::function for SelectCommand)
• Use commands.get()->GetName(), since CommandPtr doesn't actually have a GetName() method
• Specify the type parameter of Select() because type deduction was being weird
• Fix the parameter pack expansion in a function argument list (before, it was a fold expression over the sequencing operator ,)
• Change SendableChooserTestArgs to use std::string instead of std::string_view so that it matches the return type of optionsSubscriber.get() in the CHECK_EQ() macro.
• Make CommandNamed() return a non-const CommandPtr because Choose() needs non-const CommandPtrs.

So yeah, some of the errors were poorly wording/misleading, others just were a bit tricky to figure out because there were so many.

[Starlight220]

Wow -- thank you very much!
I'll apply your patch tonight and finish this up.