allwpilib
allwpilib copied to clipboard
wpilibsuite
[DRAFT] nonbreakingly rename `Subsystem` to `Resource`
Per recent discussion, this should be a strict improvement for readability and correctness-of-use. No deprecations to avoid freaking anyone out.
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.
@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).
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.
@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.
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
@SamCarlberg I view the
periodicblocks themselves as basically obsolete;addPeriodicis 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.
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.
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.
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)
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.
