Add simulator for NXT #14

mandaiy · 2016-10-30T11:20:28Z

This commit adds a feature of simulating NXT robots.
Users of the simulator can use NxtMachineSimulator by instantiating that class.
The simulator class allows users to create simulatable version of sensors/motors by the same way
as normal NxtMachine.

Load store

Remove timeout from ICommunicator#write and ICommunicator#read

This PR enables us to check that the code base sent as a PR can be compiled correctly $ gradlew check works properly (and this means tests are green) Additionally, I changed the settings of repositories (drive/drivecommand). This makes following changes: when the test fails, merge button will be disabled. when owners (PILE members) push some code to develop or master, the patch will be merged into the branch after testing. If the test fails, merge process will be canceled. (I've not checked it yet, though...) Issue #8

- Add the build status for `develop` - Fix a typo

This commit adds a feature of simulating NXT robots. Users of the simulator can use NxtMachineSimulator by instantiating that class. The simulator class allows users to create *simulatable* version of sensors/motors by the same way as normal NxtMachine.

mandaiy · 2016-10-30T11:23:13Z

According to #12, I added a simulator feature for NXT robots.
One thing I misunderstood is that users manipulate robots via Sensors/Motors.
So I implemented some simulatable sensors/motors.
Stay tuned for Ev3 and Pile version.

TODO: add test cases, general controllers.

Please feel free to leave any opinions.

mandaiy · 2016-10-30T12:02:22Z

Added a simple use-case in drive.
You can run a program on your emulator.

tiwanari · 2016-10-31T04:59:56Z

One thing I misunderstood is that users manipulate robots via Sensors/Motors.
So I implemented some simulatable sensors/motors.

I see. The workaround looks good to keep loose couplings :)

makotoshimazu

These comments are based on my comment below.
#12 (comment)

makotoshimazu · 2016-11-01T14:26:54Z

.../java/com/pileproject/drivecommand/machine/device/input/simulation/SimulatedColorSensor.java

@@ -0,0 +1,38 @@
+package com.pileproject.drivecommand.machine.device.input.simulation;


I feel these objects are just for mocking. What do you think about moving to com.pileproject.drivecommand.mock.machine.device.input to clarify which classes are available for testing?

Hmm, I don't know which is better to be honest.
@tiwanari might have opinions on this.

So, at first, let me confirm options. They are;

com.pileproject.drivecommand.machine.device.input.mock (as followed the naming discussion)

com.pileproject.drivecommand.mock.machine.device.input

right?

Anyway, I agree with @amiq11's idea in the case we use these classes just for testing because users can easily understand where these mocks are placed.
Let's regard the mock package as a small repository for mocks (It may help some developers who implement a communication for other robots).

If some classes, especially *Controllers, in this PR are general enough (i.e., it can be used not only for such testing/mocking but also for other purposes), we shouldn't move them into the mock package.

makotoshimazu · 2016-11-01T14:27:16Z

.../java/com/pileproject/drivecommand/machine/device/input/simulation/SimulatedColorSensor.java

+import com.pileproject.drivecommand.machine.device.port.InputPort;
+import com.pileproject.drivecommand.model.ProtocolBase;
+
+public class SimulatedColorSensor extends ColorSensor {


I feel Mock is more common word for this purpose. How about MockColorSensor?

makotoshimazu · 2016-11-01T14:29:37Z

.../java/com/pileproject/drivecommand/machine/device/input/simulation/SimulatedColorSensor.java

+    private int illuminance;
+
+    public SimulatedColorSensor(InputPort port, ProtocolBase protocol) {
+        super(port, protocol);


Is super necessary?

It's necessary because the superclass does not have a default constructor and then the subclasses must call one of its superclass's constructors.
Welcome to Java!

makotoshimazu · 2016-11-01T14:31:01Z

.../java/com/pileproject/drivecommand/machine/device/input/simulation/SimulatedColorSensor.java

+    private float green;
+    private float blue;
+
+    private int illuminance;


Adding m before each members for keeping the naming convention?

Yeah yeah gotcha gotcha...

makotoshimazu · 2016-11-01T14:33:48Z

src/main/java/com/pileproject/drivecommand/model/com/NullCommunicator.java

@@ -0,0 +1,29 @@
+package com.pileproject.drivecommand.model.com;


How about moving this class to com.pileproject.drivecommand.mock.model?

Same as the first reply

makotoshimazu · 2016-11-01T14:34:34Z

src/main/java/com/pileproject/drivecommand/model/com/NullCommunicator.java

+/**
+ * Null object of {@link ICommunicator}.
+ */
+public class NullCommunicator implements ICommunicator {


Let's rename this to MockCommunicator if you agree with the "mock" prefix.

makotoshimazu · 2016-11-01T14:42:14Z

src/main/java/com/pileproject/drivecommand/model/nxt/NxtMachineSimulator.java

+import com.pileproject.drivecommand.machine.device.port.InputPort;
+import com.pileproject.drivecommand.machine.device.port.OutputPort;
+
+public class NxtMachineSimulator extends NxtMachine {


I think this class could targets not only NxtMachine but all classes derived from MachineBase.
We can move this class to com.pileproject.drivecommand.mock.model, for example, and can also implement MockMachine inheriting MachineBase.

Once I thought exactly same thing as you thought.
But users of this library might use only one of the machines in this library and
in that case the corresponding mocking class has to be on the line of inheritance of the machine.
So we'd better to provide mocks for the every machines.

In my understanding, MachineBase should be the interface exactly for such purpose when we designed this library. User's program should handle MachineBase in their codes except for the factory class (currently it's NxtController, right?).

But users of this library might use only one of the machines in this library

I know there is such case, but this library should assume what I mentioned above. Additionally, I'm wondering if there is benefit when users directly use the derived classes like NxtMachine.

Your opinion seems right.
One thing which made me confused was that NxtController takes NxtMachine as its constructor argument, which seems to have to be MachineBase.
And now I feel that the controller's name is misleading as it implies general enough to be able to control any nxt machines.

and NxtController is not a factory class. now MachineProvider is responsible for that.

I'm not familiar with the actual codes, but I think NxtController should take NxtMachine because NxtController binds each port, which is Nxt(Input|Output)Port, with each device like motors and sensors.
Also, why is MachineProvider separated with the corresponding controller? In my understanding, the controller class shapes the machine. In other words, User's programs can define the model of his/her machine by implementing the controller class (ex: the machine should have two motors, and two sensors). This means MachineBase itself without controller is useless for the user's code (drive in our case). That's why I thought "(currently it's NxtController, right?).".

then the mock class has to be the sub class of the corresponding concrete class doesn't it?
if not, NxtController cannot take mocking instance.

and technically machines and controllers are tied up in the provider class. it's just because for the simplicity of the implementation.

Okay... let me summarize your comments.

How about making a common Mock class

Yeah, it's a reasonable way. But, what use cases do you imagine? I think we should refer some use cases (or a purpose) for each case (a common Mock class / Mock classes derived from *Machines).

I feel "Mock classes derived from *Machines" is a case that we, drive developers, come up with at first because we have separated apps and these classes will be used in the corresponding apps which require a concrete machine class as an argument for their controllers (these controllers have some naming problems as @Myusak mentioned. please see the next section).

The name, NxtController

Some problems of the name;

It doesn't mention about the form of NXT but, in drive, the class specifies the form/sensors/motors as @amiq11 mentioned (these controllers specify the forms). So the name is too general for the purpose from a viewpoint Maybe, we should rename it like NxtCarController if we introduce it in this library.

It indeed says this class controls an NXT. So, the class should take a NxtMachine or one of its children. This is a case in which we need such mocking classes, not a general mocking class.

In short...

I think this is a very complicated problem and you both are right. It depends on situations.

What we should discuss

What cases do you suppose with the 2 options? (i.e., what is the purpose of each option?)

Should we choose one of them? Or can we choose both?

How much should we do in this library?

Assuming all cases is impossible

Are some design changes necessary ? (In particular, as for arguments etc.)

makotoshimazu · 2016-11-01T14:45:45Z

src/main/java/com/pileproject/drivecommand/model/nxt/NxtNullProtocol.java

+import java.io.IOException;
+import java.util.Map;
+
+public class NxtNullProtocol extends NxtProtocol {


This also can be moved to com.pileproject.drivecommand.mock.model.

Same as the first reply

tiwanari

I left some comments. We need more discussions.

tiwanari · 2016-11-02T06:30:33Z

.../java/com/pileproject/drivecommand/machine/device/input/simulation/SimulatedColorSensor.java

@@ -0,0 +1,38 @@
+package com.pileproject.drivecommand.machine.device.input.simulation;


Anyway, I agree with @amiq11's idea in the case we use these classes just for testing because users can easily understand where these mocks are placed.
Let's regard the mock package as a small repository for mocks (It may help some developers who implement a communication for other robots).

If some classes, especially *Controllers, in this PR are general enough (i.e., it can be used not only for such testing/mocking but also for other purposes), we shouldn't move them into the mock package.

tiwanari · 2016-11-02T06:30:54Z

src/main/java/com/pileproject/drivecommand/model/nxt/NxtMachineSimulator.java

+import com.pileproject.drivecommand.machine.device.port.InputPort;
+import com.pileproject.drivecommand.machine.device.port.OutputPort;
+
+public class NxtMachineSimulator extends NxtMachine {


Okay... let me summarize your comments.

How about making a common Mock class

Yeah, it's a reasonable way. But, what use cases do you imagine? I think we should refer some use cases (or a purpose) for each case (a common Mock class / Mock classes derived from *Machines).

I feel "Mock classes derived from *Machines" is a case that we, drive developers, come up with at first because we have separated apps and these classes will be used in the corresponding apps which require a concrete machine class as an argument for their controllers (these controllers have some naming problems as @Myusak mentioned. please see the next section).

The name, NxtController

Some problems of the name;

It doesn't mention about the form of NXT but, in drive, the class specifies the form/sensors/motors as @amiq11 mentioned (these controllers specify the forms). So the name is too general for the purpose from a viewpoint Maybe, we should rename it like NxtCarController if we introduce it in this library.

It indeed says this class controls an NXT. So, the class should take a NxtMachine or one of its children. This is a case in which we need such mocking classes, not a general mocking class.

In short...

I think this is a very complicated problem and you both are right. It depends on situations.

What we should discuss

What cases do you suppose with the 2 options? (i.e., what is the purpose of each option?)

Should we choose one of them? Or can we choose both?

How much should we do in this library?

Assuming all cases is impossible

Are some design changes necessary ? (In particular, as for arguments etc.)

tiwanari · 2017-06-28T10:56:06Z

Oh, because of #19, this PR has a lot of diffs now...

@mandaiy Please take a look on this.

tiwanari and others added 30 commits June 16, 2014 00:50

added design document file

c944080

Added test for LineSensor.getSensorValue()

51361e6

Merge branch 'master' of ssh://git@bitbucket.org/mity/drivecommand.git

9db92a6

renamed folder (lib to libs) for auto-loading on eclipse

2def442

moved ICommand to command package

bcff8bd

added class diagram overview and some details

d1ec4e7

renamed folder

d87ce33

renamed folder

cb4cd4a

README.md edited online with Bitbucket

9fef007

arranged overview

2c45056

modified package structure

a86d1e6

arranged package and moved some files to them

d1e41d4

added end-to-end/integration test folder

ff16c32

added GyroSensorTest and RemoteControlReciever Test

66f4d33

improved tests and passed them

d0e9c22

added ColorSensorTest and passed it

e303c9d

added motor test but still writting it

9f792b9

applied test policy to tests

a0497b0

added BuzzerTest

7af626a

added Buzzer and passed its unit test

eb5a91a

added javadocs to CommandBase

b74d135

added SoundSensorTest and SoundSensor

d027a79

added TouchSensorTest and TouchSensor

e401cf6

added Servomotor and Led

63eed73

added MachineTest

4753973

added javadoc to Machine

50b3ff1

added CommandFactoryTest

ddcfe78

added Machine End to End Test

443fadf

added end-to-end test but i don't know it is good

05f3dd6

changed class name MachineEndToEndTest.java

0a9f0a5

tiwanari and others added 15 commits February 21, 2016 23:44

add more expected exception tests in MachineBaseTest

b981610

add more tests and placeholders for tests

48010c7

add more placeholder tests

48c8718

add more tests for protocols

22ee4b4

Merge pull request #3 from PileProject/load_store

98bae32

Load store

Remove timeout from ICommunicator#write and ICommunicator#read

4e3efa0

Merge pull request #6 from myusak/develop

2860782

Remove timeout from ICommunicator#write and ICommunicator#read

update LICENSE

e3c93c1

Update README for release (#7)

66ed542

modify README a little

ddf8688

Add ML URL to README.md

7083b06

Add a status bar linked to TravisCI

dd4e78d

Update README.md (#10)

f02863a

- Add the build status for `develop` - Fix a typo

Add simulator for NXT

06183e7

This commit adds a feature of simulating NXT robots. Users of the simulator can use NxtMachineSimulator by instantiating that class. The simulator class allows users to create *simulatable* version of sensors/motors by the same way as normal NxtMachine.

mandaiy added enhancement wip labels Oct 30, 2016

mandaiy assigned tiwanari and makotoshimazu Oct 30, 2016

makotoshimazu reviewed Nov 1, 2016

View reviewed changes

tiwanari reviewed Nov 2, 2016

View reviewed changes

tiwanari force-pushed the develop branch 4 times, most recently from 847b676 to 1f05cc7 Compare December 23, 2016 03:53

mandaiy closed this Jun 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add simulator for NXT #14

Add simulator for NXT #14

mandaiy commented Oct 30, 2016

mandaiy commented Oct 30, 2016

mandaiy commented Oct 30, 2016

tiwanari commented Oct 31, 2016 •

edited

Loading

makotoshimazu left a comment

makotoshimazu Nov 1, 2016 •

edited

Loading

mandaiy Nov 1, 2016

tiwanari Nov 2, 2016

tiwanari Nov 2, 2016 •

edited

Loading

makotoshimazu Nov 1, 2016

mandaiy Nov 1, 2016

makotoshimazu Nov 1, 2016

mandaiy Nov 1, 2016

makotoshimazu Nov 1, 2016

mandaiy Nov 1, 2016

makotoshimazu Nov 1, 2016

mandaiy Nov 1, 2016

makotoshimazu Nov 1, 2016

mandaiy Nov 1, 2016

makotoshimazu Nov 1, 2016 •

edited

Loading

mandaiy Nov 1, 2016

makotoshimazu Nov 1, 2016

mandaiy Nov 1, 2016

mandaiy Nov 2, 2016

makotoshimazu Nov 2, 2016 •

edited

Loading

mandaiy Nov 2, 2016

mandaiy Nov 2, 2016

tiwanari Nov 2, 2016

makotoshimazu Nov 1, 2016

mandaiy Nov 1, 2016

tiwanari left a comment

tiwanari Nov 2, 2016 •

edited

Loading

tiwanari Nov 2, 2016

tiwanari commented Jun 28, 2017

		@@ -0,0 +1,38 @@
		package com.pileproject.drivecommand.machine.device.input.simulation;

		@@ -0,0 +1,29 @@
		package com.pileproject.drivecommand.model.com;

Add simulator for NXT #14

Add simulator for NXT #14

Conversation

mandaiy commented Oct 30, 2016

mandaiy commented Oct 30, 2016

mandaiy commented Oct 30, 2016

tiwanari commented Oct 31, 2016 • edited Loading

makotoshimazu left a comment

Choose a reason for hiding this comment

makotoshimazu Nov 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiwanari Nov 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makotoshimazu Nov 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makotoshimazu Nov 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

How about making a common Mock class

The name, NxtController

In short...

What we should discuss

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiwanari left a comment

Choose a reason for hiding this comment

tiwanari Nov 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

How about making a common Mock class

The name, NxtController

In short...

What we should discuss

tiwanari commented Jun 28, 2017

tiwanari commented Oct 31, 2016 •

edited

Loading

makotoshimazu Nov 1, 2016 •

edited

Loading

tiwanari Nov 2, 2016 •

edited

Loading

makotoshimazu Nov 1, 2016 •

edited

Loading

makotoshimazu Nov 2, 2016 •

edited

Loading

The name, `NxtController`

tiwanari Nov 2, 2016 •

edited

Loading

The name, `NxtController`