Hi,
Let me start off by saying, the structure looks well done to me in terms of thinking about it in an OO way, which makes coding a lot easier (as I’m sure you know) so most of my comments will probably be pretty nitpicky. Also, props on using git for source control and github to share it. Also, good commenting, be sure, however, when your code gets more intricate, to comment it (this is a case of do as I say, not as I do). I don’t know if you attended a beta presentation or read about the beta, but singletons are a big part of it, and you are already using them.
On to criticisms (I can’t think of a benign word, sorry):
I would reccomend using a seperate package from all the WPILib stuff, I use org.frc3780 for team stuff and com.ograff for personal stuff, so that would be my suggestion for a naming convention.
Since you singleton class constructors are protected, they can still be called by other classes in the same package, and since all of your other classes are in the same package, it kind of defeats the purpose imo. The default code for singletons in the beta use private constructors, and I would suggest using the same convention.
Also, sometimes its useful to preface instance variables with something (I’ve seen it done with _var or m_var)
Here on out I will try to go file by file:
AirCannon.java:
I would make the static variable that holds the air cannon instance private
I would make the instance variable that holds the PWM aircannon private, and initialize it in the constructor, not the variable declaration. Also, I don’t know what MotorController you are using but if it is a Jaguar, or a Victor, I would suggest using the respective class.
For the fire method, when the beta stuff is generally released, I would create a openValve() method and a closeValve() method for the AirCannon class, and then use CommandGroups to make one happen after the other (there are wait commands that can be used). I suggest that, so as to not block the main thread.
ControlBoard.java:
I recommend having instance be private, and only accessible through a getInstance() method.
I really recommend against initializing instance variables at declaration.
I think that the two instance variable should also be private, if another class needs access, use get methods. I would even recommend using private getters if no other class needs access to them, because then you can use lazy instantiation (same idea as a getInstance() method, don’t initialize instance until someone requests it) however that is a habit from iPhone development, but I feel is good practice on an embedded platform.
If getStopToggle() were to be called continuously then it would flip back and forth (the new Operator Interface class will help with this), since no one can hold down a joystick button for the exact right amount of time (20 ms is how often the iterative methods are called IIRC).
Also, getStopToggle does more than just get the stopped state, while polling the button is okay there (however, with the new robot template, that won’t be necessary), I don’t know why you are logging values, since that method only deals with the stop toggle, not the values that its logging. When code works, those values will be right, but if it doesn’t those values could be wrong, and then they will just confuse you.
A lot of what you did there will be dealt with by the OperatorInterface class I think.
ControlMap.java:
I don’t quite understand the reason for this.
Public instance variables tend to be bad practice, and this whole interface thing seems a little unnecessary to me (but to each their own). Either way, I think that instance variable should be implementation specific, not interface wide.
Controller.java:
Private instance variable, possibly with private (or public) getters and lazy instantiation, not instantiated at declaration.
masterControl looks fine for now, good job separating this from the main file, although your naming seems a little unusual to me (class name, and method), but that is just preference.
In the Command Based robot (the templates for next year) I believe you would have a CommandGroup for the main control method that drives and moves on to a fire command when the button is pressed, or maybe a drive command, that gets interrupted by a fire command when the button is pressed.
For the line you use to set the stopped state, direct instance variable access tends to be a bad idea, I would recommend getters and setters.
DriveTrain.java:
Make that static instance variable private.
Initialization done in constructor, or lazily.
Private initializer
Driving can be done with arcadeDrive, just pass it a joystick, might make things simpler.
stop(): I would call self.drive(0,0) that way if you do any logging in your drive method it will get called.
SoftSwitch.java:
I would re-implement toggleState() with getters and setters, instead of direct variable access, also, if you want a one liner for it, that would be setState(!getState());
UglyBetty.java:
Looks fine, it will change for the command based robot a bit.
I encourage you to look through some of the beta presentations, as I reference them a lot. I will also fork this, and make the changes I have suggested with and without beta stuff so you can see what I’m talking about.
Again, really good job, the stuff I mentioned was really pretty particular, and probably mostly personal preference.