Skip to content

Conversation

@abbbbbby
Copy link
Contributor

@abbbbbby abbbbbby commented Oct 1, 2016

No description provided.

@riftEmber
Copy link

Forgot to register command

@riftEmber
Copy link

Fix usage of Timer here too


private Config config;
private int times;
private Timer timer;

Choose a reason for hiding this comment

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

not initialized

import org.teamtators.rotator.operatorInterface.LogitechF310;
import org.teamtators.rotator.operatorInterface.RumbleType;

public class RumblyRumble extends CommandBase implements Configurable<RumblyRumble.Config> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just call this OIRumble or JoystickRumble

case LEFT:
setRumble(RumbleType.kLeftRumble, value);
break;
case RIGHT:
Copy link
Contributor

Choose a reason for hiding this comment

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

break?

@riftEmber
Copy link

Do we want a logger?


static class Config {
public int maxTimes;
public float onTime;

Choose a reason for hiding this comment

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

you're in double territory now and you better act like it

@abbbbbby abbbbbby force-pushed the add-rumblyrumble branch 2 times, most recently from 2b3cf4d to bdc828d Compare October 7, 2016 02:53
@Override
public void configure(OIRumble.Config config) {
this.config = config;
timer = robot.timer();

Choose a reason for hiding this comment

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

This should be created in the constructor. In this scope there isn't even a "robot"

private boolean rumbling;

public OIRumble(CoreRobot robot) {
super("RumblyRumble");

Choose a reason for hiding this comment

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

Should have the same name as the command

}

@Override
public void configure(OIRumble.Config config) {

Choose a reason for hiding this comment

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

I don't think you need to specify that it's an OIRumble.Config, just Config should work

public int maxTimes;
public double onTime;
public double offTime;
public LogitechF310 joystick;
Copy link

@riftEmber riftEmber Oct 8, 2016

Choose a reason for hiding this comment

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

How does one specify a LogitechF310 in config? This should probably just be name of the joystick, and then you can get the right one in configure

logger.info("Rumbling {} times", config.maxTimes);
times = 0;
config.joystick.setRumble(config.type, config.value);
rumbling = false;

Choose a reason for hiding this comment

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

You're setting rumbling to false after starting it rumbling on the line before

@Override
protected boolean step() {
if(rumbling && timer.hasPeriodElapsed(config.onTime)) {
config.joystick.setRumble(config.type, 0.0f);

Choose a reason for hiding this comment

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

0 will do

}

protected void finish() {
config.joystick.setRumble(config.type, 0.0f);

Choose a reason for hiding this comment

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

also just a 0 here works

if(rumbling && timer.hasPeriodElapsed(config.onTime)) {
config.joystick.setRumble(config.type, 0.0f);
rumbling = false;
times ++;

Choose a reason for hiding this comment

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

Autoformat all your files

@vpzomtrrfrt vpzomtrrfrt dismissed riftEmber’s stale review October 15, 2016 01:49

It appears to be dealt with

@vpzomtrrfrt
Copy link
Contributor

Appears to depend on #119

@Override
public void configure(Config config) {
this.config = config;
if (config.joystick == "driver") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Once #121 is merged, this should use that

private AbstractOperatorInterface operatorInterface;

public OIRumble(CoreRobot robot) {
super("IORumble");

Choose a reason for hiding this comment

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

wrong

@Override
public void configure(Config config) {
this.config = config;
if (config.joystick == "driver") {

Choose a reason for hiding this comment

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

should use .equals() method, not ==

this.config = config;
if (config.joystick == "driver") {
joystick = operatorInterface.driverJoystick();
} else if (config.joystick == "gunner") {

Choose a reason for hiding this comment

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

And what if they ask for a joystick that doesn't exist?

@abbbbbby abbbbbby force-pushed the add-rumblyrumble branch 2 times, most recently from 82707e4 to ce994bf Compare October 18, 2016 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants