Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨PROS4: Base Class for Motors #532

Merged
merged 11 commits into from
Feb 23, 2023
Merged

Conversation

noam987
Copy link
Contributor

@noam987 noam987 commented Feb 21, 2023

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:

  • It compiles
  • Make a motor and motor group and test out some methods

Copy link
Contributor

@Richard-Stump Richard-Stump left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me so far. Just a couple things that I noticed, but I couldn't find any functions that were missing nor any spots in the C-API where the port's sign wasn't checked.

include/pros/motors.hpp Outdated Show resolved Hide resolved
include/pros/motors.hpp Outdated Show resolved Hide resolved
@noam987 noam987 changed the title ✨Base Class for Motors ✨PROS4: Base Class for Motors Feb 21, 2023
@djava
Copy link
Contributor

djava commented Feb 21, 2023

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
Copy link
Contributor

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.

  • Argument for making them virtual: Leaves flexibility for future use either by us or users.
  • Argument against making them virtual: We want to encourage users to build wrappers that use our classes, not create subclasses that extend them.

@djava
Copy link
Contributor

djava commented Feb 22, 2023

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.

@Richard-Stump Richard-Stump merged commit 8809460 into develop-pros-4 Feb 23, 2023
@WillXuCodes WillXuCodes deleted the pros4/base-motor branch March 8, 2023 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants