search

ramsay-t / ShefRobot

Wrapper classes for Java code to use Lego robots.
3 stars 1 forks source link

Motor refactor #17

Closed Robadob closed 9 years ago

Robadob commented 9 years ago

Fixes #12

I'm going to leave this pull request for you to merge Ramsay

Addition/Changes include:

Notes:

ramsay-t commented 9 years ago

It looks good, my only concern is the close() method. I can’t remember exactly how it was supposed to work but there was some subtly in the way the Robot.close() called the Motor.close() and vice versa - they have to clear some attribute first otherwise they will enter an infinite mutual recursion.

We should really write some tests. I might even use if for test generation research :)

On 7 Nov 2015, at 15:20, Robert Chisholm notifications@github.com wrote:

Fixes #12 https://github.com/ramsay-t/ShefRobot/issues/12 I'm going to leave this pull request for you to merge Ramsay

Addition/Changes include:

Changed PortManager to be generic, so that Action-Argument/Return pairs could be used. Adjusted close() so that it calls stop() and waits for the motor to stop, previously asynchronous rotations were causing the motors to not disconnect cleanly, requiring robot restarts. Added functionality for setSpeed() getSpeed() getMaxSpeed() isStalled() isMoving() getTachoCount() resetTachoCount() rotate(degrees, async) rotateTo(degrees, async) All functionality should be documented Notes:

I've tested all the added functionality so there should be no huge bugs. The methods like getSpeed() have been implemented so that they return synchronously with other calls to the Motor, otherwise a call to getSpeed() directly after setSpeed() returns the old speed. Didn't add them into a sub-package in the end, due to protected not allowing sub-packages access. It may be useful to add an asynchronous Motor.sleep(duration) method, so students can do more complex multi motor timings. You can view, comment on, or merge this pull request online at:

https://github.com/ramsay-t/ShefRobot/pull/17 https://github.com/ramsay-t/ShefRobot/pull/17 Commit Summary

Initial motor refactor, commiting so that it can be tested on other pc. Finished Robot refactoring, added lots of methods and documented. File Changes

A ShefRobot/LargeMotor.java https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-0 (17) A ShefRobot/MediumMotor.java https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-1 (18) M ShefRobot/Motor.java https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-2 (321) M ShefRobot/PortManager.java https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-3 (14) M ShefRobot/Robot.java https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-4 (48) M ShefRobot/Sensor.java https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-5 (6) A ShefRobot/util/Pair.java https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-6 (37) Patch Links:

https://github.com/ramsay-t/ShefRobot/pull/17.patch https://github.com/ramsay-t/ShefRobot/pull/17.patch https://github.com/ramsay-t/ShefRobot/pull/17.diff https://github.com/ramsay-t/ShefRobot/pull/17.diff — Reply to this email directly or view it on GitHub https://github.com/ramsay-t/ShefRobot/pull/17.

Robadob commented 9 years ago

my only concern is the close() method. I can’t remember exactly how it was supposed to work but there was some subtly in the way the Robot.close() called the Motor.close()

Well I haven't changed Robot.close(), and I only changed Motor.close() because I had to keep restarting the Robot after using asynchronous rotations.

I tried adding something that would leave it waiting for the action to complete, however it didn't seem to be able to tell the difference between asynchronous rotate and Motor.forward(), so i just added a call to Motor.stop() and then wait for it to actually stop.

On Sat, Nov 7, 2015 at 4:18 PM, Ramsay Taylor notifications@github.com wrote:

It looks good, my only concern is the close() method. I can’t remember exactly how it was supposed to work but there was some subtly in the way the Robot.close() called the Motor.close() and vice versa - they have to clear some attribute first otherwise they will enter an infinite mutual recursion.

We should really write some tests. I might even use if for test generation research :)

On 7 Nov 2015, at 15:20, Robert Chisholm notifications@github.com wrote:

Fixes #12 https://github.com/ramsay-t/ShefRobot/issues/12 I'm going to leave this pull request for you to merge Ramsay

Addition/Changes include:

Changed PortManager to be generic, so that Action-Argument/Return pairs could be used. Adjusted close() so that it calls stop() and waits for the motor to stop, previously asynchronous rotations were causing the motors to not disconnect cleanly, requiring robot restarts. Added functionality for setSpeed() getSpeed() getMaxSpeed() isStalled() isMoving() getTachoCount() resetTachoCount() rotate(degrees, async) rotateTo(degrees, async) All functionality should be documented Notes:

I've tested all the added functionality so there should be no huge bugs. The methods like getSpeed() have been implemented so that they return synchronously with other calls to the Motor, otherwise a call to getSpeed() directly after setSpeed() returns the old speed. Didn't add them into a sub-package in the end, due to protected not allowing sub-packages access. It may be useful to add an asynchronous Motor.sleep(duration) method, so students can do more complex multi motor timings. You can view, comment on, or merge this pull request online at:

https://github.com/ramsay-t/ShefRobot/pull/17 < https://github.com/ramsay-t/ShefRobot/pull/17> Commit Summary

Initial motor refactor, commiting so that it can be tested on other pc. Finished Robot refactoring, added lots of methods and documented. File Changes

A ShefRobot/LargeMotor.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-0> (17) A ShefRobot/MediumMotor.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-1> (18) M ShefRobot/Motor.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-2> (321) M ShefRobot/PortManager.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-3> (14) M ShefRobot/Robot.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-4> (48) M ShefRobot/Sensor.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-5> (6) A ShefRobot/util/Pair.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-6> (37) Patch Links:

https://github.com/ramsay-t/ShefRobot/pull/17.patch < https://github.com/ramsay-t/ShefRobot/pull/17.patch> https://github.com/ramsay-t/ShefRobot/pull/17.diff < https://github.com/ramsay-t/ShefRobot/pull/17.diff> — Reply to this email directly or view it on GitHub < https://github.com/ramsay-t/ShefRobot/pull/17>.

— Reply to this email directly or view it on GitHub https://github.com/ramsay-t/ShefRobot/pull/17#issuecomment-154719491.

Robadob commented 9 years ago

If you check the actual change, its more of an addition.

I'm going to start refactoring the existing sensors soon.

On Sat, Nov 7, 2015 at 4:26 PM, Robert Chisholm robadob@robadob.org wrote:

my only concern is the close() method. I can’t remember exactly how it was supposed to work but there was some subtly in the way the Robot.close() called the Motor.close()

Well I haven't changed Robot.close(), and I only changed Motor.close() because I had to keep restarting the Robot after using asynchronous rotations.

I tried adding something that would leave it waiting for the action to complete, however it didn't seem to be able to tell the difference between asynchronous rotate and Motor.forward(), so i just added a call to Motor.stop() and then wait for it to actually stop.

On Sat, Nov 7, 2015 at 4:18 PM, Ramsay Taylor notifications@github.com wrote:

It looks good, my only concern is the close() method. I can’t remember exactly how it was supposed to work but there was some subtly in the way the Robot.close() called the Motor.close() and vice versa - they have to clear some attribute first otherwise they will enter an infinite mutual recursion.

We should really write some tests. I might even use if for test generation research :)

On 7 Nov 2015, at 15:20, Robert Chisholm notifications@github.com wrote:

Fixes #12 https://github.com/ramsay-t/ShefRobot/issues/12 I'm going to leave this pull request for you to merge Ramsay

Addition/Changes include:

Changed PortManager to be generic, so that Action-Argument/Return pairs could be used. Adjusted close() so that it calls stop() and waits for the motor to stop, previously asynchronous rotations were causing the motors to not disconnect cleanly, requiring robot restarts. Added functionality for setSpeed() getSpeed() getMaxSpeed() isStalled() isMoving() getTachoCount() resetTachoCount() rotate(degrees, async) rotateTo(degrees, async) All functionality should be documented Notes:

I've tested all the added functionality so there should be no huge bugs. The methods like getSpeed() have been implemented so that they return synchronously with other calls to the Motor, otherwise a call to getSpeed() directly after setSpeed() returns the old speed. Didn't add them into a sub-package in the end, due to protected not allowing sub-packages access. It may be useful to add an asynchronous Motor.sleep(duration) method, so students can do more complex multi motor timings. You can view, comment on, or merge this pull request online at:

https://github.com/ramsay-t/ShefRobot/pull/17 < https://github.com/ramsay-t/ShefRobot/pull/17> Commit Summary

Initial motor refactor, commiting so that it can be tested on other pc. Finished Robot refactoring, added lots of methods and documented. File Changes

A ShefRobot/LargeMotor.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-0> (17) A ShefRobot/MediumMotor.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-1> (18) M ShefRobot/Motor.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-2> (321) M ShefRobot/PortManager.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-3> (14) M ShefRobot/Robot.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-4> (48) M ShefRobot/Sensor.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-5> (6) A ShefRobot/util/Pair.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-6> (37) Patch Links:

https://github.com/ramsay-t/ShefRobot/pull/17.patch < https://github.com/ramsay-t/ShefRobot/pull/17.patch> https://github.com/ramsay-t/ShefRobot/pull/17.diff < https://github.com/ramsay-t/ShefRobot/pull/17.diff> — Reply to this email directly or view it on GitHub < https://github.com/ramsay-t/ShefRobot/pull/17>.

— Reply to this email directly or view it on GitHub https://github.com/ramsay-t/ShefRobot/pull/17#issuecomment-154719491.

Robadob commented 9 years ago

My only concern is whether I need to do anything fancy to be thread-safe when reading return values on the motor's getters (because there might be a tiny chance of it reading at the same time as it's being written).

On Sat, Nov 7, 2015 at 4:27 PM, Robert Chisholm robadob@robadob.org wrote:

If you check the actual change, its more of an addition.

I'm going to start refactoring the existing sensors soon.

On Sat, Nov 7, 2015 at 4:26 PM, Robert Chisholm robadob@robadob.org wrote:

my only concern is the close() method. I can’t remember exactly how it was supposed to work but there was some subtly in the way the Robot.close() called the Motor.close()

Well I haven't changed Robot.close(), and I only changed Motor.close() because I had to keep restarting the Robot after using asynchronous rotations.

I tried adding something that would leave it waiting for the action to complete, however it didn't seem to be able to tell the difference between asynchronous rotate and Motor.forward(), so i just added a call to Motor.stop() and then wait for it to actually stop.

On Sat, Nov 7, 2015 at 4:18 PM, Ramsay Taylor notifications@github.com wrote:

It looks good, my only concern is the close() method. I can’t remember exactly how it was supposed to work but there was some subtly in the way the Robot.close() called the Motor.close() and vice versa - they have to clear some attribute first otherwise they will enter an infinite mutual recursion.

We should really write some tests. I might even use if for test generation research :)

On 7 Nov 2015, at 15:20, Robert Chisholm notifications@github.com wrote:

Fixes #12 https://github.com/ramsay-t/ShefRobot/issues/12 I'm going to leave this pull request for you to merge Ramsay

Addition/Changes include:

Changed PortManager to be generic, so that Action-Argument/Return pairs could be used. Adjusted close() so that it calls stop() and waits for the motor to stop, previously asynchronous rotations were causing the motors to not disconnect cleanly, requiring robot restarts. Added functionality for setSpeed() getSpeed() getMaxSpeed() isStalled() isMoving() getTachoCount() resetTachoCount() rotate(degrees, async) rotateTo(degrees, async) All functionality should be documented Notes:

I've tested all the added functionality so there should be no huge bugs. The methods like getSpeed() have been implemented so that they return synchronously with other calls to the Motor, otherwise a call to getSpeed() directly after setSpeed() returns the old speed. Didn't add them into a sub-package in the end, due to protected not allowing sub-packages access. It may be useful to add an asynchronous Motor.sleep(duration) method, so students can do more complex multi motor timings. You can view, comment on, or merge this pull request online at:

https://github.com/ramsay-t/ShefRobot/pull/17 < https://github.com/ramsay-t/ShefRobot/pull/17> Commit Summary

Initial motor refactor, commiting so that it can be tested on other pc. Finished Robot refactoring, added lots of methods and documented. File Changes

A ShefRobot/LargeMotor.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-0> (17) A ShefRobot/MediumMotor.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-1> (18) M ShefRobot/Motor.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-2> (321) M ShefRobot/PortManager.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-3> (14) M ShefRobot/Robot.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-4> (48) M ShefRobot/Sensor.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-5> (6) A ShefRobot/util/Pair.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-6> (37) Patch Links:

https://github.com/ramsay-t/ShefRobot/pull/17.patch < https://github.com/ramsay-t/ShefRobot/pull/17.patch> https://github.com/ramsay-t/ShefRobot/pull/17.diff < https://github.com/ramsay-t/ShefRobot/pull/17.diff> — Reply to this email directly or view it on GitHub < https://github.com/ramsay-t/ShefRobot/pull/17>.

— Reply to this email directly or view it on GitHub https://github.com/ramsay-t/ShefRobot/pull/17#issuecomment-154719491.

ramsay-t commented 9 years ago

Call it “eventually consistent” and ignore it, thats the 21st century solution :)

On 7 Nov 2015, at 16:28, Robert Chisholm notifications@github.com wrote:

My only concern is whether I need to do anything fancy to be thread-safe when reading return values on the motor's getters (because there might be a tiny chance of it reading at the same time as it's being written).

On Sat, Nov 7, 2015 at 4:27 PM, Robert Chisholm robadob@robadob.org wrote:

If you check the actual change, its more of an addition.

I'm going to start refactoring the existing sensors soon.

On Sat, Nov 7, 2015 at 4:26 PM, Robert Chisholm robadob@robadob.org wrote:

my only concern is the close() method. I can’t remember exactly how it was supposed to work but there was some subtly in the way the Robot.close() called the Motor.close()

Well I haven't changed Robot.close(), and I only changed Motor.close() because I had to keep restarting the Robot after using asynchronous rotations.

I tried adding something that would leave it waiting for the action to complete, however it didn't seem to be able to tell the difference between asynchronous rotate and Motor.forward(), so i just added a call to Motor.stop() and then wait for it to actually stop.

On Sat, Nov 7, 2015 at 4:18 PM, Ramsay Taylor notifications@github.com wrote:

It looks good, my only concern is the close() method. I can’t remember exactly how it was supposed to work but there was some subtly in the way the Robot.close() called the Motor.close() and vice versa - they have to clear some attribute first otherwise they will enter an infinite mutual recursion.

We should really write some tests. I might even use if for test generation research :)

On 7 Nov 2015, at 15:20, Robert Chisholm notifications@github.com wrote:

Fixes #12 https://github.com/ramsay-t/ShefRobot/issues/12 I'm going to leave this pull request for you to merge Ramsay

Addition/Changes include:

Changed PortManager to be generic, so that Action-Argument/Return pairs could be used. Adjusted close() so that it calls stop() and waits for the motor to stop, previously asynchronous rotations were causing the motors to not disconnect cleanly, requiring robot restarts. Added functionality for setSpeed() getSpeed() getMaxSpeed() isStalled() isMoving() getTachoCount() resetTachoCount() rotate(degrees, async) rotateTo(degrees, async) All functionality should be documented Notes:

I've tested all the added functionality so there should be no huge bugs. The methods like getSpeed() have been implemented so that they return synchronously with other calls to the Motor, otherwise a call to getSpeed() directly after setSpeed() returns the old speed. Didn't add them into a sub-package in the end, due to protected not allowing sub-packages access. It may be useful to add an asynchronous Motor.sleep(duration) method, so students can do more complex multi motor timings. You can view, comment on, or merge this pull request online at:

https://github.com/ramsay-t/ShefRobot/pull/17 < https://github.com/ramsay-t/ShefRobot/pull/17> Commit Summary

Initial motor refactor, commiting so that it can be tested on other pc. Finished Robot refactoring, added lots of methods and documented. File Changes

A ShefRobot/LargeMotor.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-0> (17) A ShefRobot/MediumMotor.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-1> (18) M ShefRobot/Motor.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-2> (321) M ShefRobot/PortManager.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-3> (14) M ShefRobot/Robot.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-4> (48) M ShefRobot/Sensor.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-5> (6) A ShefRobot/util/Pair.java < https://github.com/ramsay-t/ShefRobot/pull/17/files#diff-6> (37) Patch Links:

https://github.com/ramsay-t/ShefRobot/pull/17.patch < https://github.com/ramsay-t/ShefRobot/pull/17.patch> https://github.com/ramsay-t/ShefRobot/pull/17.diff < https://github.com/ramsay-t/ShefRobot/pull/17.diff> — Reply to this email directly or view it on GitHub < https://github.com/ramsay-t/ShefRobot/pull/17>.

— Reply to this email directly or view it on GitHub https://github.com/ramsay-t/ShefRobot/pull/17#issuecomment-154719491.

— Reply to this email directly or view it on GitHub https://github.com/ramsay-t/ShefRobot/pull/17#issuecomment-154721315.