Standardize controllers

tiwanari commented 7 years ago

This PR standardizes the MachineController to make it has the union of its children's methods.

Background

We talked over the design of controllers again and again offline, and we had two options;

Make controllers for each machine (Nxt, Ev3, Pile, etc.) so that they have their specific methods (e.g., Pile Robot doesn't have ColorSensor, thus it doesn't have getColorSensorValue or something like that) and keep the MachineController as it is.
Make the common MachineController that has the union of methods of these controllers.

They actually have pros & cons and our opinions conflicted. But finally, at present, we decided to choose the latter.

Pros & Cons of the former

Pros

Straightforward way and thus readable. This is very important for developers.
They can be easily tested.

Cons

There will be many duplications of codes because these controllers use our library's MachineBase that provides the same interface to control machines (so, we will have very similar codes in blocks like ForwardSecBlock in each build variant) but we currently can't make the most of it because of the design.
It doesn't follow the design of our library. But we can fix that of our library easily.

Pros & Cons of the latter

Pros

It follows the design of our library.
It commonizes the blocks through MachineController interface. Thus, it reduces the duplicated codes not only *.java but also *.xml.
Easy to maintain because the codes are unified.

Cons

It is difficult to test each variant.
Not straightforward way.
Users can't know what kinds of methods each controller has, and make it worse, calling wrong methods lead to RuntimeError not CompileError.
Poor extensibility. We should add/remove a method to/from MachineController itself if new machines with new methods. So, maybe we should make an another layer for other machines something like MachineController -> CarController, DroneController (i.e., not use the MachineController as the current NxtController's parent).

Why we chose the latter

This was a very difficult problem for us but we took the pros of the latter. Maintaining duplicated codes *.java and *.xml is very annoying and we can easily change our mind when a new type of machine comes if we keep the unified codes clean.

makotoshimazu commented 7 years ago

I'm taking a look. just a moment...

tiwanari commented 7 years ago

@amiq11 Thanks!

This is a wip PR. So please look over it and give any comment:)

tiwanari commented 7 years ago

Notes for me;

I should...

[x] deal with the thresholds in BlockPreferences
[x] deal with MotorProperty and SensorProperty in CarControllerBase and NxtCarController
[x] make a package for car-formed robots.

tiwanari commented 7 years ago

NOTE: I checked that we can test each child controller (e.g., NxtCarControllerTest for NxtCarController)

tiwanari commented 7 years ago

Hey @amiq11 , @myusak ,

Though I know I should add more detailed changes into this PR (e.g., some Javadocs, more blocks we will use), I'm almost fixing the design of this PR. So, please look over it again and give me comments.

I want to discuss some stuff;

What do you think about the CarControllerBase layer?
- This may be too much to do here
- There are blocks that use it by downcasting
- Should I have to move them to a specific package something like com.pileproject.drive.programming.visual.block.car?
What do you think about the BlockPreferencesSchema?
- I gave up spreading them to all variants because my previous suggestion (making getter methods for thresholds) was not so good.

I may miss something. Please let me know.

tiwanari commented 7 years ago

So far, I received thoughtful comments to keep the design (packages) and codes clean 👍 I'll apply them to codes :)

Is there any other opinion about the designs? If not, I will concentrate on cleaning up codes and ask you to review again later 🚀

tiwanari commented 7 years ago

Chages;

I made machine package for Controllers and MachineProviders.
I made *.block.(sequence|selection).car package for *Blocks that use downcast from MachineController to CarControllerBase.

I want to leave the ControllerBuilder improvement to other PR.

tiwanari commented 7 years ago

Hey @myusak, @amiq11

Now, I think this PR is on the real reviewing stage.

tiwanari commented 7 years ago

Hey @amiq11 @myusak,

If there are no more concerns, I will merge this :)

tiwanari commented 7 years ago

I applied your comments and fix codes a little.

tiwanari commented 7 years ago

I want to merge this around the end of tomorrow. Is it okay? Please let me know if you have more opinions or are too busy to check.

tiwanari commented 7 years ago

Hey @amiq11,

WDYT? Let me know your opinion.

tiwanari commented 7 years ago

I didn't receive any objection to merging so far. So, let me go ahead :)

mandaiy commented 7 years ago

Hey, don't skip the commit message and leave it to the github. At least you should add a summary that describes what classes are changed and why so that people do not have to come to this pull request.

tiwanari commented 7 years ago

Thank you for the comment. I didn't care about it and very sorry about that. I added the detail and used force commit (disable the protection of develop branch temporary). I promise to keep it in mind.

PileProject / drive