-
Notifications
You must be signed in to change notification settings - Fork 613
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
base: main
Are you sure you want to change the base?
Conversation
11a091d
to
93caf71
Compare
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() {} |
There was a problem hiding this comment.
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 🤷
I'm not seeing how calling it For example, all of the following seem like the could logically be called a I'm also worried that changing from the more tangible 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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 |
93caf71
to
846e948
Compare
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 I am still unsure about how positive a change away from the |
@SamCarlberg I view the I don't think 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 |
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 |
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 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 One reason I don't like the 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 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 |
I also really disagree with 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. |
Per recent discussion, this should be a strict improvement for readability and correctness-of-use. No deprecations to avoid freaking anyone out.