Search code examples
javaandroidmultithreadingsockets

Use multiple service or do all operations in single service, which is preferable?


I am developing one socket based application in which I am continuously updating the status of room like how many lights are on and off. I have seven of this kind of room and I need to update the status for each room.

So my question is should I create a separate service for each room or should I perform all operation in single service? Which way would be the more convenient as far as performance is concerned?

Here is my Service class for Single room.

public class UpdateUiService extends Service 
{
    @Override
    public void onCreate() {
        super.onCreate();
        intent = new Intent(BROADCAST_ACTION);
        try {
            s = new Socket("192.168.1.19,502);
            i = s.getInputStream();
            o = s.getOutputStream();
            System.out.println("connected");
        } catch (UnknownHostException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        } catch (IOException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
    }

    @Override
    public void onStart(Intent intent, int startId) {
        handler.removeCallbacks(sendUpdatesToUI);
        handler.postDelayed(sendUpdatesToUI, 1000); // 1 second

    }

    private Runnable sendUpdatesToUI = new Runnable() {
        public void run() {
            DisplayLoggingInfo();
            handler.postDelayed(this, Integer.parseInt(interval_dinning));
        }
    };
    
    private void DisplayLoggingInfo() {
        try {
            byte[] data1 = new byte[1024], packet1 = 
            { 
                (byte) 0x00,(byte) 0x00,(byte) 0x00, 
                (byte) 0x00,(byte) 0x00,(byte) 0x06, 
                (byte) 0x01,(byte) 0x01,(byte) 0x00,
                (byte) 0x00,(byte) 0x00,(byte) 0x19
            };

            o.write(packet1);
            i.read(data1, 0, 1024);

            byte_to_hex = ConversionMethods.bytesToHex(data1).substring(18, 26);

            char[] arr = byte_to_hex.toCharArray();
            for (int i = 0; i < arr.length - 1; i += 2) {
                char temp = arr[i];
                arr[i] = arr[i + 1];
                arr[i + 1] = temp;
            }

            swapped_result = new String(arr);
            result = ConversionMethods.hexStringToNBitBinary(swapped_result, 32);
            int counter_ = 0;
            for( int i=0; i<result.length(); i++ ) 
            {
                if( result.charAt(i) == '1' )
                {
                    counter_++;        
                }  
            }
            status=Integer.toString(counter_);
            
        } catch (UnknownHostException e) {
            e.printStackTrace();
        } catch (IOException e) {
            e.printStackTrace();
        }
        
        intent.putExtra("counter", String.valueOf(++counter));
        intent.putExtra("status", status);
        sendBroadcast(intent);
    } 
}

And I am starting this service and getting broadcasted intent in activity where I want to show this values. Here is the reference link to update UI from background service.

How do I achieve this using multithreading? My ultimate goal is to read socket and get the status as a result.


Solution

  • Questions

    a) It's not obvious from your code what is starting your service. Is it started once or multiple times.

    Based on current code, it looks like it will send something to ip, read result, send one broadcast and that's it.

    So, the question, do you need to update lights status just once or you need to update them constantly/periodically?

    Ideas

    a) In the case, if you need to update lights statuses just once and it's triggered from UI and updates UI, you will be much better off with AsyncTask which is specifically designed for this.

    And it's up to you whether you want to have 7 concurrent AsyncTask's (if you want to update lights status in parallel) or you can have one AsyncTask which will update lights status serially and report back to UI thread after each of lights were updated.

    b) In the case, if you need continuously track lights status then you are better off with a service. However, you need to have a long running thread in this service. So, you should start a thread in onStart.

    Generally, it should (let say once in 10 seconds) invoke some method which will cause all communication and so on.

    At that method, you can spur X threads (one for each room) and in these threads do all your operations (write to socket, read, parse and etc) or you can execute all of these actions within this first thread. You have here the same choice as with AsyncTask to do this in parallel or serially.

    c) Also, you may want to keep all sockets alive and reused them to not reconnect each 5 seconds when you need to update lights status.

    General comments

    a) You are using onStart() which is deprecated. You should be using onStartCommand()

    b) I understand that it could be a prototype, but this piece of code which you showed have quite low quality. If you won't clean it up, you will have a lot of bugs to chase in the future:

    You have in code:

    • a lot of magic numbers
    • misnamed functions (like DisplayLoggingInfo, which doesn't display anything, but rather read/write socket, do some conversion and send broadcasts)
    • long methods (DisplayLoggingInfo)

    Update 1

    Here is the sample app for you. Please be aware, this is a prototype. You may be interested to add more checks, separate it to more classes and so on.

    MyService.java

    package com.example.servicesample;
    
    import android.app.Service;
    import android.content.Intent;
    import android.os.IBinder;
    import java.lang.Thread;
    import android.support.v4.content.LocalBroadcastManager;
    
    public class MyService extends Service implements Runnable {
        public static final String ROOM_STATUS_BROADCAST = "com.example.room_status_broadcast";
        public static final String ROOM_STATUS_BROADCAST_EXTRA_ROOM_NUMBER = "roomnumber";
        public static final String ROOM_STATUS_BROADCAST_EXTRA_STATUS = "status";
    
        static final int NUM_ROOMS = 7;
        static final int TIME_FOR_A_REST = 5000; //ms
    
        Thread mThread = null;
        Boolean mRunning = false;
    
        @Override
        public void onCreate() {
        }
    
        @Override
        public IBinder onBind(Intent intent) {
            return null;
        }
    
        @Override
        public int onStartCommand(Intent intent, int flags, int startId) {
            start();
    
            return  START_STICKY;
        }
    
        @Override
        public void onDestroy() {
            stop();
        }
    
        private synchronized void start()
        {
            if (mThread != null)
                return;
    
            mRunning = true;
            mThread = new Thread(this);
            mThread.start();
        }
    
        private synchronized void stop()
        {
            if (mThread == null)
                return;
    
            mRunning = true;
    
            try
            {
                mThread.join();
            } catch (InterruptedException e) {}
            mThread = null;
        }
    
    
        public void run()
        {
            while (mRunning)
            {
                for (int i = 0; i < NUM_ROOMS; i++)
                    updateRoomStatus(i);
    
                try
                {
                    Thread.sleep(TIME_FOR_A_REST);
                } catch (InterruptedException e) {}             
            }
        }
    
        Boolean getRoomStatus(int roomNumber)
        {
            // Do real communication here (instea of just assigning true)
            // It makes sense to move all communication to a separate class from here
            Boolean newRoomStatus = true;
    
            return newRoomStatus;
        }
    
        void updateRoomStatus(int roomNumber)
        {
            Boolean newRoomStatus = getRoomStatus(roomNumber);
            broadcastRoomStatus(roomNumber, newRoomStatus);
        }
    
        void broadcastRoomStatus(int roomNumber, Boolean newRoomStatus)
        {
            Intent intent = new Intent(ROOM_STATUS_BROADCAST);
            intent.putExtra(ROOM_STATUS_BROADCAST_EXTRA_ROOM_NUMBER, roomNumber);
            intent.putExtra(ROOM_STATUS_BROADCAST_EXTRA_STATUS, newRoomStatus);
            LocalBroadcastManager.getInstance(this).sendBroadcast(intent);
        }
    
    }
    

    MyActivity.java

    package com.example.servicesample;
    
    import android.os.Bundle;
    import android.app.Activity;
    import android.content.Intent;
    import android.support.v4.content.LocalBroadcastManager;
    import android.util.Log;
    import android.view.Menu;
    import com.example.servicesample.MyService;
    import android.content.BroadcastReceiver;
    import android.content.Context;
    import android.content.IntentFilter;
    
    public class MainActivity extends Activity {
    
        private IntentFilter mIntentFilter = new IntentFilter(MyService.ROOM_STATUS_BROADCAST);
    
    
        private BroadcastReceiver mReceiver = new BroadcastReceiver() {
    
            @Override
            public void onReceive(Context context, Intent intent) {
                MainActivity.this.receivedBroadcast(intent);       
            }
        };
    
        @Override
        protected void onCreate(Bundle savedInstanceState) {
            super.onCreate(savedInstanceState);
            setContentView(R.layout.activity_main);
    
            startMyService();
        }
    
        @Override
        public boolean onCreateOptionsMenu(Menu menu) {
            // Inflate the menu; this adds items to the action bar if it is present.
            getMenuInflater().inflate(R.menu.activity_main, menu);
            return true;
        }   
    
        void startMyService()
        {
            // You can move this code to be executed on a button click or something else
            // It will start a service
            startService(new Intent(this, MyService.class));
        }   
    
        @Override
        protected void onResume()
        {
            super.onResume();
    
            LocalBroadcastManager.getInstance(this).registerReceiver(mReceiver, mIntentFilter);
        }
    
        @Override
        protected void onPause()
        {
            LocalBroadcastManager.getInstance(this).unregisterReceiver(mReceiver);
    
            super.onPause();
        }   
    
         private void receivedBroadcast(Intent i) {
             Integer roomNumber = i.getIntExtra(MyService.ROOM_STATUS_BROADCAST_EXTRA_ROOM_NUMBER, 0);
            Boolean roomStatus = i.getBooleanExtra(MyService.ROOM_STATUS_BROADCAST_EXTRA_STATUS, false);
    
            // Let's do here whatever we want with received status (as example, update UI)
            Log.d("SomeTag", "Room number "+roomNumber.toString() + " got new status " + roomStatus.toString());
         }  
    
    }
    

    AndroidManifest.xml

    <?xml version="1.0" encoding="utf-8"?>
    <manifest xmlns:android="http://schemas.android.com/apk/res/android"
        package="com.example.servicesample"
        android:versionCode="1"
        android:versionName="1.0" >
    
        <uses-sdk
            android:minSdkVersion="8"
            android:targetSdkVersion="16" />
    
        <application
            android:allowBackup="true"
            android:icon="@drawable/ic_launcher"
            android:label="@string/app_name"
            android:theme="@style/AppTheme" >
            <activity
                android:name="com.example.servicesample.MainActivity"
                android:label="@string/app_name" >
                <intent-filter>
                    <action android:name="android.intent.action.MAIN" />
    
                    <category android:name="android.intent.category.LAUNCHER" />
                </intent-filter>
            </activity>
    
            <service android:name=".MyService"/>
    
        </application>
    
    </manifest>