purduesigbots / pros

Source code for PROS kernel: open source C/C++ development for the VEX V5 microcontroller
https://pros.cs.purdue.edu
Other
259 stars 76 forks source link

✨PROS4: Base Class for Motors #532

Closed noam987 closed 1 year ago

noam987 commented 1 year ago

Summary:

Reworks the inheritance between motors and motor groups by adding a new BaseMotor virtual class.

Motivation:

525

References (optional):

Closes #525 Closes #507 by deciding that this should NOT be intended behavior Closes #490 as it doesn't make sense in the current architecture. Motor groups are vectors of ports, not of motor objects. We have added indexable methods for all non-movement methods to target any one specific motor in a motor group.

Test Plan:

djava commented 1 year ago

I think I prefer the name AbstractMotor to BaseMotor, feels less likely to be confusing. I could see people think its like for their robot base (occasionally used synonym for chassis/drivetrain). other opinions?

Richard-Stump commented 1 year ago

This was tested and confirmed to work on real hardware. Indexed setters/getters were tested, vectorized setters/getters were tested, and polymorphism was tested.

Next we need to decide whether to make the methods in Motor and MotorGroup virtual or not.

djava commented 1 year ago

We want to encourage users to build wrappers that use our classes, not create subclasses that extend them.

Why? I don't think it should really matter to us how users choose to build functionality on top of PROS.

As far as I'm concerned, the argument is really if you're okay with incurring the runtime cost of virtual dispatch on all motor function calls to allow for the possibility of people extending the motor class. Personally, I probably wouldn't take that cost, considering that extending AbstractMotor is already an option? Not totally sure though.