flamingchickens1540 / ares-luna-2020 Goto Github PK

Hello! I am Christopher Marley, a co-lead programmer of FRC Team 4681, Murphy's Law, and I have been assigned your repository as part of the Chief Delphi "2020 Code Review Party". Over the next two weeks, I will be plumbing the depths of your code and compiling everything in this one issue (so your emails aren't 'pinged' too much).
Quick disclaimer, our team uses TimedRobot, so forgive me if my knowledge of command based is lacking. I did do a lot of research on the features, so I should be up to speed.
If you have any questions at all, reply to this issue or directly email me at [email protected].
I'm very surprised there have been no issues opened in this repo... cool!

Very clean code overall. Really good documentation through comments as well. Kudos for removing or documenting unused methods and commands (My team really should work on that).

Localization Manager

• I love the way you compiled all localization and state checkers for subsystems (Shooter, Hood, and Drivetrain) at the same place. This seems way more intuitive than having every method in their own subsystem.
• L290 Using LEDs to give visual feedback to the driver is a thing we tried this season as well. It is better than looking at the dashboard in order to understand if the target is found.
• I'd like to know how much it helped to 3D transform to the inner port. Did it have a huge accuracy impact that can outweigh the FPS loss due to using solvePNP?

Robot Container

General

This part seems like the only bloated part of the robot code. You might wanna have a different place to set controller bindings and just pass a reference to the method in RobotContainer.java.

• Using a different controller layout for testing is something I feel stupid for not thinking of.
• As far as I understand, you had a lot of code laying around for command selector even though you just used the 6 cell autonomous. I get it if you just left them there in order to use it in the champs so not a big deal.

IntakeRun

Just out of curiosity, is using velocity control on intake necessary? I believe that it doesn't really put any noticeable load on RIO but still seems a bit overengineered to me. I also saw that you guys had a percent output method for intake that you didn't use.

Ramsete Config

The same goes for what I said about localization stuff. Having configuration stuff not bloat the subsystem is a great way to write clean code.

Robot

Even though we are using command-based as well, we are logging lots of stuff in Robot.java periodic methods. Your implementation seems to clear the clutter. Another thing I feel stupid for not thinking of myself.

Indexer

• L33 since TODOs show up as problems in VSCode it would drive me mad always having a TODO that you can't do anything with. Just a heads up for programmers' mental health.😁
• Queue Sequence Sequencing commands using commands' methods is a lot more readable than using SequentialCommandGroup's sequence,race etc. methods. Here's a quick example of what I mean from our code: super(new VisionTurnCG(shooter, drive, led), new SetShooterRPMPF(2900, shooter, false).withTimeout(2).raceWith(new RunHopper("sync", hopper)).andThen(() -> { drive.arcadeDrive(0.8, 0); Timer.delay(1); }).andThen(() -> drive.tankDriveVolts(0, 0)));

I didn't have time to go through all the custom libraries you used. That's pretty much all I got to say about your code. Clean code, good use of command base structure and custom libraries. If I missed some important parts of the code or you want feedback on certain parts of the code, you can let me know by replying to this issue or writing to me at [email protected]. Good luck!