Tpbot/movement service #249

danybony · 2017-01-05T13:53:35Z

Problem

We need the Bot section of our Android app to communicate with the Arduino movement unit.

Solution

First of all we need to detect when a USB device is connected. This is done through a BroadcastReceiver declared in the manifest with the action android.hardware.usb.action.USB_DEVICE_ATTACHED.
It will be triggered only by devices with one of the vendor IDs defined in devices_filter.xml.
At the moment I only added three common vendor IDs generally associated with different versions of Arduino boards. If we will see that some other boards are not recognised, we can list the connected devices' vendor IDs from the BotActivity, and add these to the list later.

Once we detect a new matching device, the BroadcastReceiver starts an Android Service responsible of this communication.
The service (MovementService) does the following:

requests the permission to manage the USB-connected device
initialises the USB serial connection
waits for an activity/other service to bind and forwards all the received messages to the serial connection

In this PR I made the BotActivity bind with the service and added a command pad to that activity as well, so that we can control the bot also from the receiver part. This is for debug only and should be removed once we have the Human-Bot network communication done.

and stop it on USB disconnected

Conflicts: TelepresenceBot/mobile/build.gradle TelepresenceBot/mobile/src/main/java/com/novoda/tpbot/human/HumanActivity.java

Mecharyry · 2017-01-05T14:05:08Z

TelepresenceBot/mobile/src/main/java/com/novoda/tpbot/bot/BotActivity.java

@@ -35,6 +50,68 @@ protected void onCreate(Bundle savedInstanceState) {

        RecyclerView.Adapter adapter = new DirectionAdapter(LayoutInflater.from(this), Collections.<Direction>emptyList());
        directions.setAdapter(adapter);
+
+        ControllerView controllerView = Views.findById(this, R.id.bot_debug_controls);


Added a controller view so that you can send commands straight to the Arduino?

Yes exactly, as I wrote in the description, this is only for debug purposes, while we don't have the network communication from the human controller

tasomaniac · 2017-01-05T15:42:30Z

TelepresenceBot/mobile/src/main/java/com/novoda/tpbot/bot/BotActivity.java

+
+    private MovementService movementService;
+    private boolean bound;
+    private Handler handler;


I am used to have private final Handler handler = new Handler();

Now these are not there anymore, moved into the CommandRepeater creation

tasomaniac · 2017-01-05T15:44:45Z

TelepresenceBot/mobile/src/main/java/com/novoda/tpbot/bot/BotActivity.java

+        }
+    };
+
+    private void startRepeatingCommand(String command) {


I am thinking about encapsulating this command handling in a separate class.
You would have a CommandHandler as a field maybe which has currentCommand inside.
I think it is weird to see String currentCommand inside the Activity.

I like the idea 👍

Done in ae51908, this reduced the duplication of that logic between the two activities

tasomaniac · 2017-01-05T15:45:33Z

TelepresenceBot/mobile/src/main/java/com/novoda/tpbot/bot/BotActivity.java

+        }
+    }
+
+    private Runnable repeatCommand = new Runnable() {


Done in b66e064

tasomaniac · 2017-01-05T15:48:24Z

TelepresenceBot/mobile/src/main/java/com/novoda/tpbot/bot/BotActivity.java

+    private Runnable repeatCommand = new Runnable() {
+        @Override
+        public void run() {
+            sendCommand(BotActivity.this.currentCommand);


These 2 lines can maybe extracted into a method named sendRepeatingCommand(String)
What do you think?

tasomaniac · 2017-01-05T15:52:59Z

TelepresenceBot/mobile/src/main/res/layout/activity_bot.xml

+  android:orientation="vertical"
+  tools:context="com.novoda.tpbot.human.HumanActivity">
+
+  <android.support.v7.widget.RecyclerView xmlns:android="http://schemas.android.com/apk/res/android"


xmlns:android="http://schemas.android.com/apk/res/android" is this redundant?

Good catch, they were leftovers from a previous refactor. Removed in 1f1793c

frapontillo

Minor changes needed for my eyes' sake (I'm old).

frapontillo · 2017-01-05T16:30:07Z

TelepresenceBot/mobile/src/main/java/com/novoda/tpbot/bot/BotActivity.java

+                                       IBinder iBinder) {
+            MovementService.Binder binder = (MovementService.Binder) iBinder;
+            movementService = binder.getService();
+            bound = true;


Rename to boundToMovementService?

frapontillo · 2017-01-05T16:30:24Z

TelepresenceBot/mobile/src/main/java/com/novoda/tpbot/bot/BotActivity.java

+        }
+
+        @Override
+        public void onServiceDisconnected(ComponentName arg0) {


arg0? 🤢

frapontillo · 2017-01-05T16:37:30Z

TelepresenceBot/mobile/src/main/java/com/novoda/tpbot/bot/MovementService.java

+
+    public void startConnection() {
+
+        if (!isSerialStarted) {


I would invert this to return if isSerialStarted.

frapontillo · 2017-01-05T16:37:54Z

TelepresenceBot/mobile/src/main/java/com/novoda/tpbot/bot/MovementService.java

+            usbManager = (UsbManager) getSystemService(Context.USB_SERVICE);
+
+            HashMap<String, UsbDevice> usbDevices = usbManager.getDeviceList();
+            if (!usbDevices.isEmpty()) {


Too many nested layers here 👀

I would also have a quick return above too. Always try to prefer the positive case when possible 😃

frapontillo · 2017-01-05T16:38:23Z

TelepresenceBot/mobile/src/main/java/com/novoda/tpbot/bot/MovementService.java

+
+    private final BroadcastReceiver broadcastReceiver = new BroadcastReceiver() {
+        @Override
+        public void onReceive(Context context, Intent intent) {


Please break this down into smaller methods.

Done in 33eb4f5

frapontillo · 2017-01-05T16:38:46Z

TelepresenceBot/mobile/src/main/java/com/novoda/tpbot/bot/MovementService.java

+                            toaster.popToast("Port is not open");
+                        }
+                    } else {
+                        Log.d("SERIAL", "Port is null");


Extract TAG to constant?

frapontillo · 2017-01-05T16:39:54Z

TelepresenceBot/mobile/src/main/java/com/novoda/tpbot/bot/UsbReceiver.java

+        Intent movementService = new Intent(context.getApplicationContext(), MovementService.class);
+        if (usbConnected(intent)) {
+            context.startService(movementService);
+        } else if(usbDisconnected(intent)) {


Space after if 😄

Mecharyry · 2017-01-05T16:49:41Z

TelepresenceBot/mobile/src/main/AndroidManifest.xml

@@ -32,6 +34,22 @@
      android:icon="@mipmap/ic_launcher"
      android:label="@string/bot" />

+    <receiver android:name="com.novoda.tpbot.bot.UsbReceiver">
+      <intent-filter>


Does it need to be in this order? Can we group the <intent-filter> together?

Mecharyry · 2017-01-05T16:53:10Z

TelepresenceBot/mobile/src/main/java/com/novoda/tpbot/bot/MovementService.java

+
+            HashMap<String, UsbDevice> usbDevices = usbManager.getDeviceList();
+            if (!usbDevices.isEmpty()) {
+                boolean keep = true;


The usage wasn't immediately clear when I read this, is there a better name?

Mecharyry · 2017-01-05T16:54:57Z

TelepresenceBot/mobile/src/main/java/com/novoda/tpbot/bot/MovementService.java

+        }
+    };
+
+    private UsbSerialInterface.UsbReadCallback mCallback = new UsbSerialInterface.UsbReadCallback() {


onDataReceievedListener?

Yeah. mCallback 😷

Mecharyry · 2017-01-05T17:00:37Z

TelepresenceBot/mobile/src/main/java/com/novoda/tpbot/human/ControllerListener.java

@@ -3,7 +3,7 @@
 import com.novoda.notils.logger.simple.Log;
 import com.novoda.tpbot.Direction;

-interface ControllerListener {


If this is being used by both I would move it to the more generic package. I know that it will move again in the future but it shows our intent clearly when looking at the structure of the app.

Done in 2739722

Mecharyry · 2017-01-05T17:02:08Z

TelepresenceBot/mobile/src/main/java/com/novoda/tpbot/human/HumanActivity.java

@@ -15,13 +15,11 @@
 public class HumanActivity extends AppCompatActivity {

    private static final String LAZERS = String.valueOf(Character.toChars(0x1F4A5));
-    private static final long COMMAND_REPEAT_DELAY = TimeUnit.MILLISECONDS.toMillis(100);
+    private static final long COMMAND_FADING_DELAY = TimeUnit.MILLISECONDS.toMillis(100);


Maybe it is worth moving this internally to the debug view? Likely will be the same whether ever we use it at this point.

Yes, good point

Mecharyry · 2017-01-05T17:03:12Z

TelepresenceBot/mobile/src/main/res/layout/activity_bot.xml

+  android:orientation="vertical"
+  tools:context="com.novoda.tpbot.human.HumanActivity">
+
+  <android.support.v7.widget.RecyclerView


I'm thinking of replacing this with the DebugView in my branch, it will make the process of building the debug version slightly faster.

Can we do it once this is merged?
So that changes for a single PR are smaller

…riable

Mecharyry

Nice work on this 👍 Good to see the progress. We should try to extract out some of those if statements to make the usb connection class easier to read.

Mecharyry · 2017-01-05T17:08:03Z

TelepresenceBot/mobile/src/main/java/com/novoda/tpbot/bot/MovementService.java

+                usbManager.requestPermission(device, pendingIntent);
+                break;
+            } else {
+                connection = null;


Should these be set to null on each iteration of the loop? I assumed this would happen if we go through all of the usbDevices and we do not find a supporting device?

tasomaniac

LGTM 🎉

danybony · 2017-01-09T09:04:08Z

@frapontillo @Mecharyry all comments addressed (including @tasomaniac ones, but he already approved 👅 )

Mecharyry · 2017-01-09T09:05:37Z

TelepresenceBot/mobile/src/main/java/com/novoda/tpbot/bot/MovementService.java

+    private static final String SERIAL_TAG = "SERIAL";
+
+    private static final String ACTION_USB_PERMISSION = "com.novoda.tpbot.USB_PERMISSION";
+    private static final List<Integer> SUPPORTED_VENDOR_IDS = Arrays.asList(9025, 10755, 4292); // TODO: read from devices_filter.xml


Is this going to be done in a future PR?

Mecharyry

Nice job! 🎉

danybony added 10 commits December 17, 2016 12:29

Create Movement service

d9ec542

Connect Human Activity to Movement service

151a49f

Add BroadcastReceiver for USB attach-detach

f611073

Start service on USB connected

62bceaa

and stop it on USB disconnected

Add usbSerial dependency

23ec51c

Update Directions with raw command indication

f824be4

Implement serial movement in MovementService

e00f2a6

Merge branch 'tpbot-feature-branch' into tpbot/movement_service

e4de079

Conflicts: TelepresenceBot/mobile/build.gradle TelepresenceBot/mobile/src/main/java/com/novoda/tpbot/human/HumanActivity.java

Move connection to the movementservice to BotActivity

f3c6e40

Add controls to bot activity for debug purposes

5afe6ae

danybony added the TpBot label Jan 5, 2017

Mecharyry reviewed Jan 5, 2017

View reviewed changes

tasomaniac reviewed Jan 5, 2017

View reviewed changes

danybony added 3 commits January 5, 2017 17:09

Make repeatCommand and serviceConnection final

b66e064

Extract commands repeating logic to dedicated class

ae51908

remove redundant xml namespaces

1f1793c

frapontillo suggested changes Jan 5, 2017

View reviewed changes

Mecharyry reviewed Jan 5, 2017

View reviewed changes

Rename boundToMovementService field and some params

d392ace

Mecharyry reviewed Jan 5, 2017

View reviewed changes

Group together BroadcastReceiver intent filter actions

a79f83a

Mecharyry reviewed Jan 5, 2017

View reviewed changes

Return fast if serial started in startConnection()

570da18

Mecharyry reviewed Jan 5, 2017

View reviewed changes

Return fast if no devices are connected and remove unclearly named va…

5489d1c

…riable

Mecharyry suggested changes Jan 5, 2017

View reviewed changes

Mecharyry reviewed Jan 5, 2017

View reviewed changes

tasomaniac approved these changes Jan 6, 2017

View reviewed changes

danybony added 7 commits January 9, 2017 09:32

Move COMMAND_FADING_DELAY into SelfDestructingMessageView

427749d

Move shared classes from human to controls package

2739722

Rename onDataReceievedListener

e68ca35

Reformat UsbReceiver

c732d0b

Extract logging tag to constant

fae922a

Extract setupSerialPort method

33eb4f5

Set connection and device to null only if no supported devices found

a500af9

Mecharyry reviewed Jan 9, 2017

View reviewed changes

Mecharyry approved these changes Jan 9, 2017

View reviewed changes

frapontillo approved these changes Jan 9, 2017

View reviewed changes

frapontillo merged commit 2a59746 into tpbot-feature-branch Jan 9, 2017

frapontillo deleted the tpbot/movement_service branch January 9, 2017 09:10

Tpbot/movement service #249

Tpbot/movement service #249

Conversation

danybony commented Jan 5, 2017

Problem

Solution

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

Choose a reason for hiding this comment

frapontillo left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mecharyry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tasomaniac left a comment

Choose a reason for hiding this comment

danybony commented Jan 9, 2017

Choose a reason for hiding this comment

Mecharyry left a comment

Choose a reason for hiding this comment