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

🐛[BUG] - motor .get_direction() does not work #695

Open
ssejrog opened this issue Sep 5, 2024 · 10 comments
Open

🐛[BUG] - motor .get_direction() does not work #695

ssejrog opened this issue Sep 5, 2024 · 10 comments
Labels
bug Something isn't working

Comments

@ssejrog
Copy link
Contributor

ssejrog commented Sep 5, 2024

Describe the bug
This outputs 2147483647

To Reproduce
Steps to reproduce the behavior:
The example project uses motor = 127 instead of motor.move(), so with that this is the example code.

void opcontrol() {
  pros::MotorGroup mg({1, 3});
  pros::Controller master(E_CONTROLLER_MASTER);
  while (true) {
    mg.move(master.get_analog(E_CONTROLLER_ANALOG_LEFT_Y));
    // Print the motor direction for the motor at index 1. (port 3)
    std::cout << "Motor Direction: " << mg.get_direction();
    pros::delay(2);
  }
}

Expected behavior
Because the motor isn't reversed, I should see 1.

Screenshots
If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

  • OS: [e.g. iOS, Windows, Linux]
  • Version [e.g. 22]

Additional context
Add any other context about the problem here.

@ssejrog ssejrog added the bug Something isn't working label Sep 5, 2024
@WillXuCodes
Copy link
Member

are the motors plugged in

@ssejrog
Copy link
Contributor Author

ssejrog commented Sep 5, 2024

are the motors plugged in

it turns out I was confusing two issues-

I'm having a constructor take a motor group instead of a vector of ints. I was trying to check the direction of the motors so I could copy/paste it to the "motor groups" that are used internally in the code. This didn't work, and then the example code didn't work (when I don't have motors in port 1 or 3), and I made this issue.

I've done more testing now and it seems like something still isn't quite right.

No matter what I do with the directions of 11 and 15, the only thing I see printed is 0. I tried{11, 15}, {-11, 15}, {-11, -15}, {11, -15}.

pros::MotorGroup mg({11, 15});
int dir = mg.get_direction();
using namespace pros;
void opcontrol() {
  std::cout << "Motor Direction: " << dir << "\n";
  printf("\n\n");
  pros::Controller master(E_CONTROLLER_MASTER);

  while (true) {
    mg.move(master.get_analog(E_CONTROLLER_ANALOG_LEFT_Y));
    dir = mg.get_direction();
    std::cout << "Motor Direction: " << dir << "\n";
    pros::delay(2);
  }
}

This is also true of 11 and -11 with a motor and not a motor group, only 0 is printed.

pros::Motor mg(11);
int dir = mg.get_direction();
using namespace pros;
void opcontrol() {
  std::cout << "Motor Direction: " << dir << "\n";
  printf("\n\n");
  pros::Controller master(E_CONTROLLER_MASTER);

  while (true) {
    mg.move(master.get_analog(E_CONTROLLER_ANALOG_LEFT_Y));
    // Print the motor direction for the motor at index 1. (port 3)
    dir = mg.get_direction();
    std::cout << "Motor Direction: " << dir << "\n";
    pros::delay(2);
  }
}

and it is also true of the C functions, only 0 is printed.

void opcontrol() {
  while (true) {
    pros::c::motor_move(-11, pros::c::controller_get_analog(pros::E_CONTROLLER_MASTER, pros::E_CONTROLLER_ANALOG_LEFT_Y));
    printf("Motor Direction: %d\n", pros::c::motor_get_direction(11));
    pros::delay(2);
  }
}

In all of these examples, I confirmed the motors were moving with the joystick. Sorry for not being as thorough initially!

@thiccaxe
Copy link

thiccaxe commented Oct 7, 2024

We can't reproduce this. "2147483647" is when the motor is not plugged in, which you seem to have resolved. But if the motor is moving, it should output -1 or 1. Are you running the latest version of the kernel?

@ion098
Copy link
Contributor

ion098 commented Nov 21, 2024

@thiccaxe @ssejrog
I think the issue here is that negative port numbers do NOT call pros::c::motor_set_reversed, they simply reverse the command. It also looks like the C and C++ API differ here: pros::c::motor_set_reversed is setting a VEXos flag (which is read then by pros::c::motor_get_direction), whereas Motor::set_reversed is simply flipping the sign of the Motor object's port member variable. I think the best solution would be to deprecate pros::c::motor_set_reversed and pros::c::motor_get_reversed, since there's not a good way of getting the C API to match the C++ API.

@djava
Copy link
Contributor

djava commented Nov 21, 2024

I think the best solution would be to deprecate pros::c::motor_set_reversed and pros::c::motor_get_reversed

But then you wouldn't be able to reverse the motor from C? This might be okay if the C functions (i.e. pros::c::motor_move and pros::c::motor_get_position) support negative port numbers (which I'm unsure of), but I don't think we're going to want to break that API in a minor update.

@ion098
Copy link
Contributor

ion098 commented Nov 21, 2024

re @djava

I think the best solution would be to deprecate pros::c::motor_set_reversed and pros::c::motor_get_reversed

But then you wouldn't be able to reverse the motor from C? This might be okay if the C functions (i.e. pros::c::motor_move and pros::c::motor_get_position) support negative port numbers (which I'm unsure of), but I don't think we're going to want to break that API in a minor update.

The C functions do indeed support negative port numbers, so there's not too much of an issue. As far as breaking API goes, pros::c::motor_get_reversed is already arguably broken, so making it a deprecated function that always returns false isn't really breaking anything further imo.

@djava
Copy link
Contributor

djava commented Nov 21, 2024

@ion098

As far as breaking API goes, pros::c::motor_get_reversed is already arguably broken, so making it a deprecated function that always returns false isn't really breaking anything further imo.

Deprecating motor_set_reversed is much more of a concern to me than get.

@ion098
Copy link
Contributor

ion098 commented Nov 21, 2024

@djava

As far as breaking API goes, pros::c::motor_get_reversed is already arguably broken, so making it a deprecated function that always returns false isn't really breaking anything further imo.

Deprecating motor_set_reversed is much more of a concern to me than get.

I think the C API is infrequently used to the extent that this shouldn't be too much of an issue.

Also, it turns out there's even more API madness: the C++ API has a method get_direction which uses both VEXos's flag and the sign of the port number (???), but there's no C++ API equivalent to set_reversed. I'm not sure if this is intentional or just a big oversight, but it's definitely very confusing behavior.

@djava
Copy link
Contributor

djava commented Nov 21, 2024

@ion098
I'm honestly not super opposed to having an array of "reversed" values for each port number that is toggled by pros::c::motor_set_reversed so that we can recreate the functionality (though this is still a bitttt of an API break because now it's persistent across disconnects, but that seems closer to okay). I don't want to just kill the function entirely in a minor update though.

there's no C++ API equivalent to set_reversed

false?

@ion098
Copy link
Contributor

ion098 commented Nov 21, 2024

@djava

there's no C++ API equivalent to set_reversed

false?

I was referring to implementation, the C++ API doesn't set the VEXos flag like the C API does, rather it merely flips the sign of the port stored by Motor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants