Request: Prefer protected over private for class members

[fletch3555]

Using "private" scope prevents extending classes, causing teams to have to re-implement the class in it's entirety.

To give a more concrete example, I'm working with the MecanumDrive class, but need to configure it to use closed-loop control for the TalonSRX objects. Since DriveCartesian calls .Set(<value>), not .Set(ControlMode::Velocity, <value>), I need to override that method. When doing so, I copy/paste the method body into the subclass and recompile. However, all members of the MecanumDrive class are private, most importantly the SpeedController objects.

I propose using protected scope instead. This should apply to any class that is likely to be used and overridden by end users, including motor controllers, sensors, etc.

I was testing C++ specifically, but I imagine the same problem would occur in Java as well.

[fletch3555]

Is there a reason this was closed without comment? I don't see anything in https://github.com/wpilibsuite/allwpilib/blob/master/CONTRIBUTING.md that my request needs to follow any particular format. At the very least, a simple comment explaining what I did wrong would be appreciated.

[calcmogul]

My phone was in my pocket when this PR was closed, so I didn't actually intend to close it. My bad.

[calcmogul]

Composition would be preferred over inheritance for providing customization points. By making these classes take joystick inputs and return kinematic results, the user can pass them to whatever they want, not just motors (for example, it makes closed loop velocity control trivial). The alternative is the ClosedLoopMotor PR, but I dislike how it composes and there's a lot of conversion factors for normalization that don't need to exist.

It would be a breaking change done over the next two years, but makes more powerful abstractions possible in a much cleaner, linear fashion (swerve drive, closed loop MIMO control, etc.).

[fletch3555]

That's a new one. Never seen anyone "butt-close-github-issues" before. 😆

I like the idea of composition, but as you said, that's breaking and would require a couple years to phase it in. Changing private to protected would not be breaking and could even be released this year (assuming another release was planned).

I'm willing to put together the PR for it, and will even cover both java and C++ sides, but would appreciate help building a list of classes this would be beneficial for. I'm happy with leaving it to just the "drive" classes for now if that's reasonable?

[Starlight220]

Just an idea : having the motor output to a lambda, so there is no need for overriding.

[auscompgeek]

Given that the kinematics classes are in WPILib now, do those solve your usecase @fletch3555?

[calcmogul]

The drive class use-case mentioned here would be resolved by the suggestions made in #1691.

[lidor51]

I've discussed with @prateekma in here on some things related to the HolonomicDriveController & SwerveControllerCommand, among them is making the fields of HolonomicDriveController protected instead of private (and also adding another c'tor to HolonomicDriveController, so it can get any DriveController that implements the "calculate" method).

Why I think you should make this move:
If WPILib see the teams only as a consumers of their code (especially for the "full-solutions" like HolonomicDriveController etc.) , so you should keep the encapsulation as is. But, I don't believe it should be the case. WPILib shouldn't look of the the teams as a consumer, but meant to be an educational tool, that rather than read and understand WPILib's code, also need let the students the option to continue to develop and build upon the great tools WPILib is providing.

I believe that the price of not doing that, can be worst:

• Duplicating and changing manually parts of WPILib's code. That's forking is a bad habit, teams can (and will) miss out important updates in WPILib's side.
• Just don't using WPILib tools. Depends on team's reason, that's not necessary bad. But I do believe that many teams would be glad to continue develop upon your lib, and use it as a wide base for their code, and right now it's not possible in some packages.

*Just to be clear I'm pro the great things that WPILib is doing, an believe it should, and easily can be better in being something all kind of teams are getting advantage of.

[PeterJohnson]

One of the biggest issues with making currently private functions/data protected is it would make the implementation part of the public interface. Right now we have the ability to change around class internals without that being an API breaking change. With protected, changing the internals would also be a breaking change, perhaps even a subtle one, as class internals often make unstated assumptions that external interfaces do not (e.g. holding a lock or not, or partially initialized internal data). Protected data in particular is widely considered to be a code smell. Composition in general is much cleaner than inheritance except where desired--IMO a number of WPILib classes should really be marked final to make composition a requirement.

[Starlight220]

I'll point to what I said in #3536. When team and WPILib functionality becomes mix-and-match, teams don't always have the knowledge to build the puzzle correctly. WPILib takes that responsibility by making elements private or final.

[PeterJohnson]

Also see brief discussion on final in #1851, where we decided to not apply it globally, but rather more sparingly.

[truher]

hey everybody, I can add my voice to the list of folks who wish for nonprivateness.

alternatively, and better, stuff like holonomicdrivecontroller could be more fully dependency-injected. use of statics like chassis speeds.fromfieldrelative are particularly tough to work around.

maybe think of phases of rework: start with deprivatizing and move to (constructor) injection over time.

?

[calcmogul]

WPILib used to use inheritance a lot in the 2008-era, but we've migrated almost entirely to composition because it improves component cohesion and reduces coupling with vendor and team code we don't control. Cohesive components are easier to test and maintain, and loosely coupled code can be refactored easier without breaking many users. This video summarizes my thoughts on composition vs inheritance: https://www.youtube.com/watch?v=hxGOiiR9ZKg.

Exposing all our internal implementation details as protected increases coupling with team code, so we can't refactor internals without breaking teams. Even if we say "hey, protected members are subject to change", teams are going to use them anyway and complain when they change.

We've refactored WPILib over time into more composable pieces so teams can swap implementation details and do the plumbing themselves if they don't want what the default wrappers do. We consider class customization points via inheritance or higher-order function callbacks carefully because they tend to increase internal complexity and make the library harder to maintain. If the user coordinates the interactions instead, they can understand, instrument, and troubleshoot their code better. We prefer teams do their own plumbing unless there's really only one sensible composition combination, and teams won't need visibility into that code (e.g., ProfiledPIDController).

RamseteCommand is an example of a well-intentioned abstraction backfiring. RamseteCommand abstracts away the composition of RamseteController and velocity PIDControllers, but that's not what users typically mess up. Instead, users have misconfigured sensors, misconfigured motors, and unit conversion issues. Teams need access to the plumbing to log and troubleshoot those problems, which is currently only possible if they copy-paste the internals. Logging the internals with Sendable would be a band-aid for a leaky abstraction that teams realistically need to peel away to get their robot working. This is why we provide an example without RamseteCommand.

stuff like holonomicdrivecontroller could be more fully dependency-injected

We already inject the PID controllers in the constructor and provide getters for them. All HolonomicDriveController does is plumb them, so I'm not sure what else one could customize without having effectively rewritten the entire class anyway. Was the need for customization due to some bug that needs fixing?

[truher]

I agree about the virtue of composition. I think the question is what to do between now and when WPILib fully embraces that idea. Teams could cut and paste, or they could depend on protected members, either way seems bad, though in different ways. I can appreciate that in the former case the badness is experienced by the teams themselves, whereas the latter case burdens WPIlib folk with the complaining.

I encourage my students to investigate how the library code they use actually works, and experiment with it in small ways, and that practice does seem somewhat opposed to some aspects of software design that aim to improve maintainability. I would personally favor reducing the surface area to maintain by eliminating things like RamseteCommand or ProfiledPidSubsystem.

The specific issue with HolonomicDriveController is that we wanted to adjust the angle used in ChassisSpeeds.fromFieldRelativeSpeeds to correct for sensor lag and main loop time. Maybe we could just change the currentPose passed to calculate() but maybe that would have more effects than we wanted? The barrier was actually the use of the ChassisSpeed static methods; better for that to be some sort of injected calculator.

[calcmogul]

I think the question is what to do between now and when WPILib fully embraces that idea.

Well, making stuff protected is a public API change, so that won't happen until the offseason. At that point, we can just refactor things properly. We just need a list of what to refactor and why. An example of the kinds of refactors we've done in the past are providing static inverse kinematic functions on the drive classes so teams can call that instead and insert their own stuff before the motor set calls.

WPILib is like a standard library for FRC. Keep in mind that what we include should be useful for the majority of teams, but we can't be everything to everyone. It's expected that teams will write some custom stuff for unique use cases.

I can appreciate that in the former case the badness is experienced by the teams themselves, whereas the latter case burdens WPIlib folk with the complaining.

In the former case, the main benefit to teams is that upstream internal changes won't break their code without warning. The latter case is bad for both parties because we either:

1. Treat internals as part of the public API, so evolution/improvement on it slows down. We'd also have to provide changelogs for what used to be implementation details, which is too much work.
2. Keep treating implementation details as private but mark them protected. Teams would get broken often by internal changes, and we wouldn't provide changelogs or migration guides for them.

I encourage my students to investigate how the library code they use actually works, and experiment with it in small ways, and that practice does seem somewhat opposed to some aspects of software design that aim to improve maintainability.

Encouraging students to understand the library internals is a good thing in an educational context (although hopefully not required to use the library well). There's just a cost to providing large amounts of configuration. It creates more work for library maintainers and makes the intended API usage less clear for users.

I would personally favor reducing the surface area to maintain by eliminating things like RamseteCommand or ProfiledPidSubsystem.

I agree. PIDCommand and ProfiledPIDSubsystem in particular don't offer much benefit over the modern pattern of inline commands and subsystem command factories. That's going to be an uphill battle though because many teams and mentors are entrenched in antiquated Java/OOP design patterns (i.e., classes/inheritance are the primary method of abstraction, even in situations where functions make more sense).

The specific issue with HolonomicDriveController is that we wanted to adjust the angle used in ChassisSpeeds.fromFieldRelativeSpeeds to correct for sensor lag and main loop time. Maybe we could just change the currentPose passed to calculate() but maybe that would have more effects than we wanted?

I don't have all the context for your situation, but sensor lag and other delays sound like something that would negatively affect odometry, so fixing the odometry directly seems better than tweaking the controller to deal with bad input. This reminds me of https://www.chiefdelphi.com/t/whitepaper-swerve-drive-skew-and-second-order-kinematics/416964, so you could see if that fixes the problem.

[truher]

re: maintaining the private pattern but doing more refactoring prior to 2024, that sounds great.

re: delay correction, yeah, that thread, and the one below [1], were indeed the inspiration for this change. i initially expected the magnitude of delay to be similar to the main loop time, assuming the bulk of the issue was simply not accounting for the evolution of the pose during the next period, but adding more delay produced better results. There was actually a Mythbusters moment where the student working the problem kinda anchored on the main-loop-time expectation, even though the results seemed progressively better at larger numbers, and I had to remind them, "if it's worth doing, it's worth overdoing," i.e. finding the optimum requires finding the other side of the hill you're climbing.

[1] https://www.chiefdelphi.com/t/field-relative-swervedrive-drift-even-with-simulated-perfect-modules/413892/16