Fix command-based examples to not use globals
virtuald opened this issue · 13 comments
I'm not sure what the right approach is, but globals are generally a Bad Thing and we shouldn't be teaching students to do things this way. And it leads to problems with the simulation (see robotpy/robotpy-wpilib#275)
Example badness: https://github.com/robotpy/examples/blob/master/command-based/oi.py#L6
I don't even know what a global is in python, but I have working code that doesn't use it that employs most commandCode functions. If necessary, I can adapt it to work as an example, however, it probably won't be "pythonic" or whatever. https://github.com/RVRProgramming/Command-Python-Code-2017
As you probably know, in most modern programming, globals are generally considered Bad Things and should be avoided -- though, obviously there are exceptions.
A global variable in python doesn't necessarily require using the global keyword, any variable declared at module scope can be considered a global. For example, https://github.com/RVRProgramming/Command-Python-Code-2017/blob/607ac03ca70f706d1714716058c2b5ed70402b81/subsystems/driveBase.py#L21 is a global variable.
When using RobotPy's testing framework, globals are evil because the framework needs to recreate the Robot class and anything that the robot touches. For example, in your drivebase class above, there's no easy way for the testing framework to recreate/reinitialize your RobotDrive object. It's important for the framework to reinitialize your robot class and any corresponding wpilib objects so that each time you run a test your tests are starting from a clean/guaranteed slate, and that prior test runs won't affect any future runs.
Ahh, I see. Well, you run into a fundamental problem with the command code framework itself in that case, as it requires only using one instance of a subsystem to work properly for most situations. Creating a single instance like it is seen there is necessary to prevent conflicting commands.
It's certainly possible to not do that -- just don't create the variables in module scope, create them elsewhere (such as in MyRobot) and pass the variables around. The RobotPy gearsbot example is written in that style by passing the MyRobot object to each subsystem/command: in this command it accesses the subsystem via the robot object
However, you're right that it's different from the style encouraged in the Java command framework examples, which use static singleton instances (which is a type of global) for accessing the various pieces.
Christ this is getting more complicated than I thought. While that IS an option in some cases, it becomes exceedingly hard to prevent import loops using that method, at least in my experience. I much prefer that method, and that is how it is supposed to be done in Java. I will have to revisit that option another time with my code and see if I overlooked something. There is a good chance I did, but I don't feel like looking right now, ill report back some other time.
In the gearsbot example: If the commands and subsystems aren't importing each other (and for the most part they aren't), an import loop is impossible.
However, I agree that it's not ideal, and passing the robot object around to everyone is just lots of annoying boilerplate, which is why this issue exists -- to find a better way.
Of course, this is complicated by the fact that I rather dislike the command framework, and discourage people from using it in favor of using Magicbot instead, so I'm not particularly interested in spending energy fixing usage of it.
As a user of the command framework, as you discover ways to make the command stuff better, make fixes and pull requests! :)
We are also fans of the command based paradigm, so maybe we can figure out a Right Way (TM) to handle these in a pythonic manner. Would all issues be fixed by avoiding module-level variables and pre-robotInit subsystem initialization? We have a programmer meeting tonight, so we can brainstorm about that.
@FRC2539 yes, keeping in mind that static class-level variables are really just a special form of module-level variables
Still open to fixes on this. 👍
We've been busy getting ready for an off-season competition tomorrow, so this has been put on the back-burner. It is on the list of things we might tackle before build season, but we have questions.
It seems like we can't go more than three files in WPILib without running into a static class-level variable. That suggests these have value and allow interesting behaviors, and therefore are not always the wrong choice.
Our concern is that it could trigger a bug. We have read through the ticket that spawned this one, but couldn't figure out what bug we're attempting to fix. Could you help us understand what bug exists in the current command-based example, and why?
I don't like the various WPILib static instances either, and they can potentially cause all sorts of problems if they aren't cleaned up properly. However, we have functionality built in that cleans up all the static state when a test is ended. I would love for all of that to go away, but it can't.
The problem is resetting state when running the pyfrc tests. As I'm sure you know, the way the tests work are:
- Start test
- Create instance of the user's robot class
- Run startCompetition loop
- Stop instance of user's robot class
- Stop test
As stated above:
When using RobotPy's testing framework, globals are evil because the framework needs to recreate the Robot class and anything that the robot touches. For example, in your drivebase class above, there's no easy way for the testing framework to recreate/reinitialize your RobotDrive object. It's important for the framework to reinitialize your robot class and any corresponding wpilib objects so that each time you run a test your tests are starting from a clean/guaranteed slate, and that prior test runs won't affect any future runs.
It's definitely possible to work around this -- and in fact, the current examples do this by just overwriting the previous global when the robot code starts up. However, it's just ugly, and IMHO shows students a bad example. We should do better.
The approach that gearsbot uses (passing a robot instance around and copying things from it) introduces a lot of potential boilerplate. I like the variable-injection approach of magicbot, and perhaps it would be appropriate to use here. Other ideas are welcome too.