SouthEugeneRoboticsTeam / sertain

Framework for programming FRC robots, inspired by Sertain Legacy and Meanlib
6 stars 0 forks source link

Rewrite MotorController to be abstract and implement specific instances of Talon and Victor motor controllers #62

Open snowsignal opened 4 years ago

snowsignal commented 4 years ago

This PR closes #54.

Here is a short summary of the changes:

These changes will make it much, much easier to implement a simulated motor controller and future types of motor controllers. I haven't run this through a linter yet, so styling could be off. Let me know what you think.

snowsignal commented 4 years ago

Alright, I also added the PhoenixMotorController subclass and moved a bunch of functions that couldn't be generalized for any motor controller into that subclass. The inheritance tree looks like this:

              MotorController ------------------------
              |                                      |
         SimulatedMotorController         PhoenixMotorController
                            --------------------------- | ---------------
                            |                                           |
                  TalonMotorController              VictorMotorController

SimulatedMotorController doesn't exist yet, but I'm showing where it would go.

snowsignal commented 4 years ago

I know this isn’t a priority issue, but it would be nice to get this reviewed + merged soon so I can start fixing the related issues.

mckelveygreg commented 4 years ago

@Jamdan2 Can we try running this branch on the robot soon? @InquisitivePenguin Would you also be able to update this year's competition code to reflect the new API?

gnawinganimal commented 4 years ago

Just a thought: should we change the motorController function to just motor? I feel like a helper function like that should be nice and short.

gnawinganimal commented 4 years ago

Haha just realized there was a problem with this. My bad.

mckelveygreg commented 4 years ago

@Jamdan2 Do we have plans of merging this before competition?

snowsignal commented 4 years ago

@mckelveygreg sorry for the late response, but yes, I can rewrite this year's code to work with the API.

@Jamdan2 You're right, but that's an implementation detail for when we implement simulated motor controllers. For now though, I'll create the ActualTalonMotorController subclass.

gnawinganimal commented 4 years ago

We need to share code between the actual motor controllers because they do almost the exact same thing, but we also need to the simulated and actual motor controllers to be treated as equals by the compiler... I'm gonna be honest I'm really wishing kotlin had rust's impl statement right now

snowsignal commented 4 years ago

Kotlin doesn't need impl because you can used abstract classes directly. We can just use TalonMotorController to work with both simulated and actual talon motor controllers. The fact that the actual Talon and Victor motor controller classes shared a lot of their code could be considered redundant, but it allows for more flexibility and makes more sense from a design perspective.

gnawinganimal commented 4 years ago

That's true. I suppose having duplicate code here is more convenient than avoiding it.