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

[DRAFT] nonbreakingly rename Subsystem to Resource #6620

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Oblarg
Copy link
Contributor

@Oblarg Oblarg commented May 13, 2024

Per recent discussion, this should be a strict improvement for readability and correctness-of-use. No deprecations to avoid freaking anyone out.

@Oblarg Oblarg requested review from a team as code owners May 13, 2024 18:14
@Oblarg Oblarg marked this pull request as draft May 13, 2024 18:15
.gitignore Outdated Show resolved Hide resolved
@Oblarg Oblarg force-pushed the rename-subsystem-to-resource branch 2 times, most recently from 11a091d to 93caf71 Compare May 13, 2024 19:34
Comment on lines 33 to 40
default void periodic() {}

/**
* This method is called periodically by the {@link CommandScheduler}. Useful for updating
* resource-specific state that needs to be maintained for simulations, such as for updating
* {@link edu.wpi.first.wpilibj.simulation} classes and setting simulated sensor readings.
*/
default void simulationPeriodic() {}
Copy link
Member

Choose a reason for hiding this comment

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

These seem more appropriate for subsystems than for a more generic "resource" type. In my mind, resources are really just for the ownership model for commands, not periodic things in and of themselves. Periodic code with a resource can be done just by calling the periodic() function in robotPeriodic(), and the same for the simulation periodic function. Though I have had students tell me that "it's just easier" to use a subsystem to get access to a periodic method that runs automatically, instead of them writing the one line of code themselves 🤷

@sciencewhiz
Copy link
Contributor

sciencewhiz commented May 14, 2024

I'm not seeing how calling it Resource is strictly better. I think it trades one set of problems for another.

For example, all of the following seem like the could logically be called a Resource: Drive Motor, Swerve Module, Swerve Drive, but only the latter really makes sense to be a Subsystem, and is probably the right choice.

I'm also worried that changing from the more tangible Subsystem to the more abstract Resource will hurt younger programmers. While it's true that calling it a Subsystem sometimes causes them to chose allocation of components to Subsystems that makes their job harder later, it comes with the benefit that it's easier to explain. I'm worried that the abstract term Resource could lead to paralysis, which is worse.

Along the lines of what @SamCarlberg said, I wonder if it wouldn't be better if we provided something that provides some of the conveniences of Subsystems (ie periodic) without deconfliction in the scheduler. Then the documentation could talk about how to choose one or the other. Right now people are told that a Subystem means resource deconfliction, but there isn't a description of what they should do if they didn't need that.

@@ -14,14 +14,14 @@
*
* <p>This class is provided by the NewCommands VendorDep
*/
public abstract class SubsystemBase implements Subsystem, Sendable {
public abstract class SubsystemBase implements Resource, Sendable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is not renaming SubsystemBase intentional? It seems like that's what most people would use in their code, so I'm not seeing the benefit of the name change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the fence about renaming SubsystemBase, since the Sendable stuff doesn't necessarily make sense to scope at the Resource level.

@Oblarg
Copy link
Contributor Author

Oblarg commented May 14, 2024

@sciencewhiz

I don't think there's any way to do more than this without making the changes breaking. In 2027 we'll have a chance to fix this properly, but this seems like the best compromise until then.

All of the things you list can be valid resources, and if there's a "paralysis" issue with the new name it's because newer programmers are now not considering this because the current name is misleading, not because it's not a valid way to structure a robot program. Students already have to decide how large their resource blocks are, and already make mistakes doing this - changing the name makes the problem more visible, which is a good thing. I don't think we can say without context what is "probably the right choice" for most mechanisms, for most students - right now, the decision is usually made arbitrarily based on inappropriate connotations from a misleading classname.

@SamCarlberg The periodic blocks seem reasonable to have in Resource because it's typical to have some internal periodic implementation maintaining the Command contract (e.g. a resource running a PID loop).

@Oblarg Oblarg force-pushed the rename-subsystem-to-resource branch from 93caf71 to 846e948 Compare May 14, 2024 12:04
@SamCarlberg
Copy link
Member

The periodic blocks seem reasonable to have in Resource because it's typical to have some internal periodic implementation maintaining the Command contract (e.g. a resource running a PID loop).

I see that as orthogonal in the best case, and bad practice at worst. Subsystems tend to not always use PID loops (stopping a motor just writes 0 volts, instead of controlling to 0 RPM), and PID loops should be handled by the commands or the methods they call. Other helpful periodic functions to run would be LED buffer copies (eg a command writes to a buffer, and the subsystem periodically writes the buffer to the DIO pin) or data logging. But those aren't a necessary function of the requirement semantics, just an internal state management tool.

It seems to me like Subsystem is overloaded to mean both "a thing that with exclusive ownership" and "a thing that can periodically manage its own state". Maybe we should have separate Requireable and Periodic interfaces instead of jamming it all into Resource; commands require Requireable things, and if you want an automatically-registered periodic function, make your subsystem class additionally implement Periodic. Or if you only need a periodic thing without requirement semantics, just implement Periodic without Requireable.

I am still unsure about how positive a change away from the Subsystem name will be. It's very easy to understand that a subsystem in code corresponds to a physical subsystem or mechanism on a robot. A generic Resource or Requireable (or whatever) name doesn't have that physical analogy. Maybe we keep the Subsystem interface as a helpful alias to make that connection still clear, but allow Resource to be used for those non-mechanical systems that still need the ownership model.

@Oblarg
Copy link
Contributor Author

Oblarg commented May 14, 2024

@SamCarlberg I view the periodic blocks themselves as basically obsolete; addPeriodic is the proper utility for registering periodic tasks. They're not touched here out of convenience and to minimize breakages.

I don't think Subsystem is a helpful alias for Resource - the point of Resource is not necessarily to represent entire mechanisms, and in fact the structure of code should never be expected to exactly match the physical structure of the robot. It's extremely important for programmers of embedded systems to learn how to distinguish between the structure of the physical problem-space and the control problem-space, and we shouldn't use terminology that blends these confusingly. The idea of a library class that is meant to represent a generic physical robot mechanism is itself kind of confused, because there is no clear contract that entails - so the Subsystem name is misleading, even if it seems intuitively clear.

Teaching students to structure their code based on "Subsystems" in the way the documentation currently does teaches them to think about the problem in a fundamentally confused manner, where one looks for a library class that exactly represents a familiar primitive from another problem-space and builds outwards from there. This does not work in general and leads to poorly-structured code.

I think there is place in a future v3 command library for a Subsystem class that properly represents aggregations of resources, but it is not realistic to implement here.

@jasondaming
Copy link
Member

jasondaming commented May 14, 2024

To me more important than the actual change here is the change in the documentation to properly describe what reasoning you are trying to push with this change. That is the part where the discussion can really happen. It is hard to determine the merit of this name change without properly going through the how do we use it, teach it, and give examples for it.

You say above you don't like how subsystem is taught, that is fine but we need to understand what you want to replace that with

@sciencewhiz
Copy link
Contributor

@SamCarlberg I view the periodic blocks themselves as basically obsolete; addPeriodic is the proper utility for registering periodic tasks. They're not touched here out of convenience and to minimize breakages.

Right now there's zero examples using addPeriodic, and the only reference in frc-docs is in reference to running things at faster rates, not for replacing Subsystem's periodic. If the WPILib examples and documentation aren't using the "proper" method, I can't imagine how teams are expected to.

@Oblarg
Copy link
Contributor Author

Oblarg commented May 14, 2024

There aren't any examples for addPeriodic, which is why this PR doesn't remove addPeriodic.

The point of this PR is to be minimally-invasive while renaming the class to something that isn't actively-misleading re: what it actually does and is useful for. The examples we do have are split between describing what the class does in code, and describing a disconnected conceptual parsing of the class that vaguely relates to what it does in code.

Changing the name nonbreakingly like this keeps the value of the misguided documentation approximately at what it was, while making a better description seamlessly more-discoverable and giving us room to improve it as we go.

@PeterJohnson
Copy link
Member

PeterJohnson commented May 14, 2024

The Subsystem name is indeed confusing for teams because they think of Subsystem in terms of system terminology, not command scheduler execution. It's often a 1:1 mapping, but is not 1:1 often enough to be a source of confusion as teams map out their code structure. Some other name to capture the command scheduler mutex concept would likely help with this, but it's a little debatable whether Resource is the right name (over e.g. Requirement, Requirable, HardwareMutex, etc).

One reason I don't like the Resource name is I fear it may further conflate the existing confusion on sensors--it's perfectly fine for any piece of robot code to access a sensor with no command mutexing required or desired, but Resource is so generic that teams may want to wrap sensors into Resources.

It's perhaps worth noting that any object (or even any unique identifier) can serve as the actual "mutex" for the command scheduler. It could literally be just an Object (in Java at least). The reasons to have an interface for this concept seem to boil down to (1) having it named something is useful from a self-documenting perspective, (2) it's a good place for user-friendly helper functions to create commands that implicitly require the object itself as the mutex, (3) it encourages grouping of the hardware objects being mutex'ed into the user's concrete implementation of the interface.

It's potentially arguable whether point (3) is a sufficient reason why this class needs to be an interface/base class, rather than a concrete class that is just contained in a subsystem side-by-side with the hardware items. The main downside of the concrete class approach would be that (as with multithread mutexes when used in this way) the user would need to be consistent with which resource is used with which hardware items, otherwise the protection is lost.

If it is an interface/base class, is the eventual code structure concept going to be "subsystem" classes that contain "resource" classes that actually implement the hardware interfaces? Or is the idea that "resource" classes wholly replace the purpose that "subsystem" classes currently serve, just with a different name?

Either way, I appreciate the effort to not break existing code. In my opinion, this needs to be a gradual transition as long as we're still calling it "commands v2". Commands "v3" could do a hard break.

@Oblarg
Copy link
Contributor Author

Oblarg commented May 14, 2024

per @SamCarlberg's suggestions I've moved the loop and default command functionality out of Resource, since arguably those imply a commitment to more than just mutexing and helper functions (@PeterJohnson's reduction of what the class does is pretty accurate i think)

@TheTripleV
Copy link
Member

I also really disagree with Resource. It's an generic name that can be applied to nearly anything. I would be much more comfortable with a name like Requireable or RequireableByACommand.

If there's definitely a commandsv3 planned for 2027, I'd rather make this change at the same time. There's not really a point in forcing a transition over 2 years just to break the name again.

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.

None yet

7 participants