Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite command controller articles to show composition #2684

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Gold856
Copy link
Contributor

@Gold856 Gold856 commented Aug 1, 2024

Control subsystems are full of footguns and are poor abstractions since they don't save any typing, yet they actively bypass the command mutex system. Control commands are also poor abstractions as they are easy to mess up, yet hide internal details away from the user, making debugging harder. They also don't save any typing.

@Gold856 Gold856 force-pushed the remove-control-subsystems branch 2 times, most recently from 23d3351 to 7cac095 Compare August 1, 2024 04:31
Copy link
Collaborator

@sciencewhiz sciencewhiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather then remove, the docs need to be replaced with what the new preferred pattern is.

@Gold856 Gold856 force-pushed the remove-control-subsystems branch 3 times, most recently from c55c89c to be47e1a Compare August 2, 2024 23:52
@sciencewhiz sciencewhiz added this to the 2025 milestone Aug 3, 2024
@Gold856 Gold856 changed the title Remove references to control subsystems Rewrite command controller articles to show composition Aug 17, 2024
:sync: Java

.. remoteliteralinclude:: https://raw.githubusercontent.com/wpilibsuite/allwpilib/v2024.3.2/wpilibjExamples/src/main/java/edu/wpi/first/wpilibj/examples/armbotoffboard/subsystems/ArmSubsystem.java
.. remoteliteralinclude:: https://raw.githubusercontent.com/wpilibsuite/allwpilib/main/wpilibjExamples/src/main/java/edu/wpi/first/wpilibj/examples/drivedistanceoffboard/subsystems/DriveSubsystem.java
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a full 40 character commit hash instead of main, as that will keep the RLI from breaking if there's future changes. Inspector will be able to update the commit hash

@@ -94,7 +28,7 @@ What does a ``TrapezoidProfileSubsystem`` look like when used in practice? The
.. tab-item:: C++ (Header)
:sync: C++ (Header)

.. remoteliteralinclude:: https://raw.githubusercontent.com/wpilibsuite/allwpilib/v2024.3.2/wpilibcExamples/src/main/cpp/examples/ArmBotOffboard/include/subsystems/ArmSubsystem.h
.. remoteliteralinclude:: https://raw.githubusercontent.com/wpilibsuite/allwpilib/main/wpilibcExamples/src/main/cpp/examples/DriveDistanceOffboard/include/subsystems/DriveSubsystem.h
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto about hash

@@ -103,139 +37,12 @@ What does a ``TrapezoidProfileSubsystem`` look like when used in practice? The
.. tab-item:: C++ (Source)
:sync: C++ (Source)

.. remoteliteralinclude:: https://raw.githubusercontent.com/wpilibsuite/allwpilib/v2024.3.2/wpilibcExamples/src/main/cpp/examples/ArmBotOffboard/cpp/subsystems/ArmSubsystem.cpp
.. remoteliteralinclude:: https://raw.githubusercontent.com/wpilibsuite/allwpilib/main/wpilibcExamples/src/main/cpp/examples/DriveDistanceOffboard/cpp/subsystems/DriveSubsystem.cpp
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto about hash

### Control Algorithm Commands

There are commands for various control setups:
There are many different ways to implement these controllers in the command framework for various control setups:

- ``PIDCommand`` uses a PID controller. For more info, see :ref:`docs/software/commandbased/pid-subsystems-commands:PIDCommand`.
- ``PIDController`` can be integrated into a ``Subsystem``. For more info, see :ref:`docs/software/commandbased/pid-subsystems-commands:PID Control in Command-based`.

- ``TrapezoidProfileCommand`` tracks a trapezoid motion profile. For more info, see :ref:`docs/software/commandbased/profile-subsystems-commands:TrapezoidProfileCommand`.
- ``TrapezoidProfile`` tracks a trapezoid motion profile. For more info, see :doc:`/docs/software/commandbased/profile-subsystems-commands`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section lists controller command classes, and should be removed once all the controller command classes are phased out. In the meantime, an admonition pointing to the other pages might be helpful (but I'm not entirely convinced it's needed); it shouldn't be an item in a list of command classes.

@Gold856 Gold856 force-pushed the remove-control-subsystems branch 2 times, most recently from 326d05f to 62d15a8 Compare August 28, 2024 08:08
@sciencewhiz
Copy link
Collaborator

There's conflicts to resolve

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants