mcoffin / rogue

OSGi container (karaf distribution) for FRC
Apache License 2.0
0 stars 0 forks source link

API - Fix #14 #18

Closed WillYingling closed 2 years ago

WillYingling commented 9 years ago

I left out a manipulator, because I couldn't think of anything that wasn't game-specific or covered in RobotComponent

mcoffin commented 9 years ago

You'll also need to add package <...> statements and move things to the mcoffin.rogue package somewhere, preferably mcoffin.rogue.api.

mcoffin commented 9 years ago

Also please fix your indent formatting as per Google Java Style.

mcoffin commented 9 years ago

Now that your package has changed, you'll have to move these to the right directory in the project (i.e. not WillYinging/rogue/..., but mcoffin/rogue/... instead).

The other comment I had is that are you SURE this is how you want to interact with the drive code? It feels like this could get VERY manual and actually be kind of a PITA if we write the API this way.

Honestly, you're probably going to need 2 separate APIs: one for mechanum, and one for regular old drive. Then the apis would look something like the following:

Mechanum

trait MechanumDrive {
  def setDriveVector(magnitude: Double, direction: Double, turnRate: Double)
}

Regular

trait Drive {
  def setDrive(magnitude: Double, turnRate: Double)
}

Now, I could be wrong and this could be the worst way to control these two drive types, but just give it some thought and report back!

WillYingling commented 9 years ago

I think you're right about separating them, that makes more sense. I also separated put the encoder functions into a new file, since those can be used or not used for both Drives, or a manipulator.

mcoffin commented 9 years ago

This is a code review for a fix for #14. Not sure why waffle didn't pick up on that.

mcoffin commented 9 years ago

@WillYingling make sure you take a look at the comments on the outdated diffs above as some of them still apply (most notably the renaming and the re-thinking of the interface).