Code Review!!

[Anupam Goli]

So now that building (for us) is officially over, I'd like to do a massive code Review. Now, unfortunately there are only two Java programmers on our team (actually two programmers in general), and so I'd like someone who is outside of the two of us to check the code out, see if we did anything fatal. Of course, only we can check our math, but i'm looking to see if i made any stupid mistakes or if i need to improve an algorithm a little bit. Here is the URL to our google code project:
http://code.google.com/p/2011deployable-1002/

Leave comments and Enjoy!

P.s. I think there should be an entire subsection of a forum dedicated to this type of thing. It would be quite useful.

[Jeanne Boyarsky]

Minor nit: The URL to checkout on http://code.google.com/p/2011deploya...ource/checkout has a space in it so you can't copy/paste it.

Things that jump out as possible problems:

1. In the drive class, if a CAN timeout happens, you print the error and move on. What should happen in this case. If you can't drive, you are kind of doomed. But is there anything you can do to partially recover? Drive with one motor maybe? (I don't think the CAN network precludes this depending on which fail.) If that's the case, you'd need null checks around the CANJaguar calls so some can work.
2. We didn't extend RobotDrive directly, so I'm not sure about this one. But isn't while(true) dangerous? It looks like it would force you to reboot the robot after each match. Whereas while (!isEnabled()) would end the loop when you disable the bot.
3. fibers[current].run(); looks strange. The fibers/components implement the Runnable interface in Java. The idea of which is to use it for threads which run in the background. To use them, one typically calls something like "new Thread(fibers[current]).start()". This creates a new thread and starts it in the background. Whereas fibers[current].run() just call the run method once synchronously. So you aren't using any of the benefits of threading. Which clearly works fine. But implementing Runnable adds confusion. At least to someone who works with Java regularly .
4. If you were using threads to run things in the background, you might have a "CPU starvation" problem. The run methods do their work constantly instead of sleeping/waiting/yielding to let other threads run. If you were to switch to threads this is something important to be aware of. Due to #3, it's a point of theoretical interest.

Also one Java coding conventions. I wouldn't change it at this point, but you asked for a code review so you are getting one from a real Java programmer .

1. Class names should begin with uppercase

And some things I particularly liked:

1. Good package structure/design
2. I like how you handled the minibot and have a check for whether the game is almost over
3. It was easy to read and figure out what was going on.

Good luck!

[Anupam Goli]

Well, based on our drive format of choice currently (currently we are doing tank, and if you noticed we had a software switch to go from meccanum to Tank). The robot would not be able to move really well anyway, and wouldn't the Jags that were declared up until the exception was thrown be running?

2. I'm sure that there should be while(isEnabled()) instead of a while(true), while loops with boolean true are normally used for software written for running on actual computers, no embedded systems. I'll change that so I don't do something fatal.

3. We were planning on using something we call "minithreading" during autonomous; basically, we have the compressor run as a background process (it will only check the switch once every 30 seconds orso) and have the arm and drive being constantly processed.

4. I understand about CPU Starvation, but we shuld get good results for teleop in this setup. In autonomous, things are going to be different if we implement actual threading. (two possiblities: implement threading or have everything written as a step function to increment constantly).

I normally stick to Java convention, this time around i forgot for a few seconds. (Also, I hate having instance fields at the bottom of the class)

Thanks for your review and help. I will amend all of what needs to be amended now.

[Ether]

Originally Posted by Wing

we have the compressor run as a background process (it will only check the switch once every 30 seconds orso)

What switch are you referring to?

[Anupam Goli]

Originally Posted by Ether

What switch are you referring to?

The pressure switch on the compressor.

We considered using a threading system. The extension of Runnable is just a remnant of it. We ultimately removed it due to the time management problems we might have met. We might revive it if we found a way to implement it.

[Ether]

Originally Posted by Wing

The pressure switch on the compressor.

How much pressure can build up if the compressor runs uninterrupted for 30 seconds.

Originally Posted by Ether

How much pressure can build up if the compressor runs uninterrupted for 30 seconds.

More than what is legal. We're going to change that.

[Jeanne Boyarsky]

> and wouldn't the Jags that were declared up until the exception was
> thrown be running?
Yes. Although I suspect the main code would reference one and through a null pointer stopping the whole program after that. As you noted, this is a point of mainly obscure interest because moving would be so poor anyway.

> but we shuld get good results for teleop in this setup
Right. Because there isn't threading.

[jhersh]

When using CAN, threading will improve your bus utilization by approximately 3x if using the Black Jag Bridge. Potentially more if using the 2CAN. The reason is there is more bus bandwidth than that consumed talking to a single Jag. That means that you can have messages in flight to up to about 3 jags at a time, thus reducing the overall time needed to update all of your CAN Jags. Only by using multiple threads can you actually send more than one message at a time.

Remember that you can only ever have one message TO THE SAME JAG at a time. All parallel messages must be to different Jags. If you send more than one message to the same Jag at the same time, the messages will simply be serialized by the library (i.e. things will still work, you just won't see a performance improvement).

-Joe