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

Have a SubsystemGroup class to facilitate more compact commands #67

Open
haydenheroux opened this issue May 23, 2022 · 2 comments
Open

Comments

@haydenheroux
Copy link
Member

Why should the Library of Gongolerium include this feature?

It is currently considered best practice to pass references to subsystems as arguments to commands which require those subsystems to function.

image
https://docs.wpilib.org/en/stable/docs/software/commandbased/commands.html

However, passing subsystems increases the number of arguments to each command, which obscures the true purpose of providing arguments to a command: altering the functionality of the command via alterations in the argument(s). Especially in autonomous programming (see BackupAndShootHighThenLeaveTarmac) where numerous subsystems are commonly used within each command, each constructor has the opportunity to become less readable as more functionality is added.

In order to combat this problem, it would be useful to supply a group of subsystems as an argument under a single alias. While it is technically possible to instantiate an array to store a set of subsystems and pass this array's name as an argument, this solution is not standardized and does not have the opportunity to hold any functionality (e.g. error checking, logging, etc.). As such, I think it is a good idea to wrap a group of subsystems under a class, whose functionality I will detail below.

How should this feature be implemented?

I have already implemented a "working" example; see https://gist.github.com/haydenheroux/bd2226b9c0994de7415404abec66e5c3 for source code with tests.

The SubsystemGroup class should have three methods: a constructor, an add method, and a get method. These methods are essentially the same set of methods a user would have with a normal array/List/Vector/etc.

SubsystemGroup(Subsytem...) -> can throw exception, no return value
subsystemGroupInstance.add(Subsystem) -> can throw exception, no return value [1]
subsystemGroupInstance.get(Class) -> takes the name of a class [2] which extends Subsystem/SubsystemBase returns an instance of that class, if it is stored within

[1]: For convenience, an official implementation may choose to return the instance of the SubsystemGroup itself, since chaining methods could make code more readable. I had initially wanted to implement this but I forgot.
[2]: Ok, this is more complex than it sounds. See the example code I posted for how I chose to implement this. Essentially, if ExampleSubsystem extends the Subsystem/SubsystemBase class, this method should take ExampleSubsystem (not an instance of ExampleSubsystem!) as an argument, such as subsystemGroupInstance.get(ExampleSubsystem) // Returns an instance of ExampleSubsystem or nothing

Exceptions / Errors / Edge Cases

I threw exceptions (specifically some custom ImmutabilityExceptions) to ensure that multiple subsystems of the same class/type were not stored. If it were the case that there was multiple subsystems of the same type stored within the group, there is no method by which a user could determine which subsystem would be used. I made the arbitrary rule to only allow one of each type of subsystem within the SubsystemGroup because there should not be any reason to use multiple subsystems of the same type since they each should represent distinct mechanisms on the robot (... unless we could the EndgameArm case from this year). I do not know of any other edge cases which could cause an error or unexpected behavior.

image
caught on adding m_shooter2 is due to m_shooter1 (of the same type as m_shooter2) already being present within the SubsystemGroup when there was an attempt to add m_shooter2 to the SubsystemGroup.

@haydenheroux
Copy link
Member Author

haydenheroux commented May 23, 2022

The result of the get method could be implemented as an Option enum, to robustly handle the case where the Subsystem is not within the group.

@kylecorry31
Copy link
Member

Good idea, I'll plan on adding this

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

No branches or pull requests

2 participants