3
\$\begingroup\$

I made an android app which utilizes SSDP for automatic device discovery.

Because I didn't need the whole UPnP functionality, I wrote my own independent SSDP component.

It works well, but I feel like I might've made the implementation way more complex than it needs to by adding too much indirection/abstraction.

I can't articulate the problem well enough, but it just seems too hard to understand overall.

InitSSDPComponent - called upon application being launched

private void InitSSDPComponent() {
    try {
        // Acquire multicast lock to broadcast udp packets to SSDP address
        WifiManager wm = (WifiManager) getSystemService(Context.WIFI_SERVICE);
        WifiManager.MulticastLock multicastLock = wm
                .createMulticastLock("multicastLock");
        multicastLock.acquire();

        ssdpComponent = new SSDPMainComponent(getApplicationContext());
        // Start receiving M-SEARCH and NOTIFY messages sent by other
        // devices
        ssdpComponent.StartReceivingSSDPMessages();

        // Start receiving device information sent directly from other
        // devices
        // upon sending M-SEARCH messages from this device
        ssdpComponent.StartReceivingSSDPDeviceInfo();

        // Start sending M-SEARCH messages to SSDP broadcasting address
        // periodically
        ssdpComponent.StartPeriodicMSearch();

        // Start sending NOTIFY messages to SSDP broadcasting address
        // periodically
        ssdpComponent.StartPeriodicNotify();

    } catch (IOException e) {
        Log.e("MainActivity", "onCreate IOException", e);
    }
}

Basically two separate UDP sockets are created: A socket called ListenerSocket is bound to the multicast address of 239.255.255.250:1900, which listens for M-SEARCH messages sent from other SSDP compliant devices. Upon ListenerSocket receiving an M-SEARCH message, a response containing this device's information is sent back to the requested device. Another one called MulticastSocket creates a UDP socket on any random available port, then sends out M-SEARCH messages to the broadcasting address so that other devices listening for M-SEARCH messages can respond with the listener's device information (Basically what ListenerSocket does).

In short, ListenerSocket is for other devices to discover this device and MulticastSocket is for this device to discover other devices.

So my SSDPMainComponent.java looks something like this

public class SSDPMainComponent {
    private SSDPMulticastSocket multicastSocket;
    private SSDPListenerSocket listenerSocket;
    private boolean isMulticastSocketEnabled = false;
    private static final long M_SEARCH_BROADCASTING_INTERVAL = 60000; // In milliseconds, every 60 seconds
    private static final long NOTIFY_BROADCASTING_INTERVAL = 850000; // In milliseconds, every 850 seconds
    private static final String ROOT_DEVICE_XML_URL_PATH = "/localService/xml/rootdevice.xml";
    private static URL ROOT_DEVICE_URL;
    Thread socketInitThread;

    public SSDPMainComponent(Context context) throws IOException {
        ROOT_DEVICE_URL = new URL("http://" + NetworkUtils.getLocalIpAddress(context)
                + ":" + RestfulWebServerConstants.WEB_SERVER_PORT 
                + ROOT_DEVICE_XML_URL_PATH);
        socketInitThread = new Thread(new Runnable() {
            @Override
            public void run() {
                try {
                    //Socket need to be created on a separate thread.
                    multicastSocket = new SSDPMulticastSocket();
                    listenerSocket = new SSDPListenerSocket();
                } catch (IOException e) {
                    Log.e("SSDPMainComponent", "SSDPMainComponent IOException", e);
                }
            }
        });
        socketInitThread.start();
        try {
            socketInitThread.join();
        } catch (InterruptedException e) {
            Log.e("SSDPMainComponent", "SSDPMainComponent InterruptedException", e);
        }
        ctx = context;
    }

    public void StartPeriodicMSearch() {
        new Timer().scheduleAtFixedRate(new TimerTask() {
            @Override
            public void run() {
                SendMSearchMessage();
            }
        }, 0, M_SEARCH_BROADCASTING_INTERVAL);
    }

    public void StartReceivingSSDPDeviceInfo() {
        new Thread(new Runnable() {
            @Override
            public void run() {
                ReceiveSSDPDeviceInfo();
            }
        }).start();
    }

    public void StartReceivingSSDPMessages() {
        new Thread(new Runnable() {
            @Override
            public void run() { 
                ReceiveSSDPMessages();
            }
        }).start();
    }

    public void SendMSearchMessage() {
        SSDPSearchMsg searchRootDevice = new SSDPSearchMsg(
                SSDPConstants.ST_RootDevice);

        try {
            multicastSocket.send(searchRootDevice.toString());
        } catch (IOException e) {
            // TODO Auto-generated catch block
            Log.e("SSDPMainComponent", "SendMSearchMessage IOException", e);
        }
    }

    private void ReceiveSSDPMessages() {
        while (true) {
            DatagramPacket dp;
            try {
                dp = listenerSocket.receive();
                String packetData = new String(dp.getData());
                Log.i("ReceiveSSDPMessages", packetData.toString());

                SSDPDiscoveryPacketParser packetParser = new SSDPDiscoveryPacketParser(packetData);
                if (!packetParser.isValidDiscoveryPacket())
                    continue;

                if (packetParser.isNotifyMessage()) {
                    //TODO: Add received device information to DB
                }
                else {
                    //Respond with this device's information back to the requested device
                    SendMSearchResponseMsg(packetParser.getMX(),dp.getAddress(),dp.getPort());
                }
            } catch (IOException e) {
                e.printStackTrace();
            }
        }
    }

    private void SendMSearchResponseMsg(final int mx, final InetAddress destAddr, final int port) {
        new Thread(new Runnable() {
            @Override
            public void run() {
                try {
                    Thread.sleep(mx * 1000);
                    SSDPSearchResponseMsg responseMsg = new SSDPSearchResponseMsg(ROOT_DEVICE_URL);
                    Log.i("MSearchResponse","DEST ADDR: " + destAddr.getHostAddress() + ":" + Integer.toString(port));
                    listenerSocket.send(responseMsg.getRootResponseMsg(), destAddr, port);
                } catch (InterruptedException e) {
                    Log.e("SSDPMainComponent", "SendMSearchResponseMsg InterruptedException",e);
                } catch (IOException e) {
                    Log.e("SSDPMainComponent", "SendMSearchResponseMsg IOException",e);
                }
            }
        }).start();
    }

    private void ReceiveSSDPDeviceInfo() {
        if (multicastSocket == null || isMulticastSocketEnabled) {
            return;
        }
        isMulticastSocketEnabled = true;
        while (true) {
            try {
                DatagramPacket dp;
                dp = multicastSocket.receive();
                String packetData = new String(dp.getData());
                Log.v("SSDP PACKET DATA", packetData.toString());

                SSDPDevicePacketParser packetParser = new SSDPDevicePacketParser(packetData);

                if (!packetParser.requestXML())
                    continue;
                DeviceDbAdapter deviceDbAdapter = DeviceDbAdapter.getInstance();
                ServiceApiDbAdapter serviceApiDbAdapter = ServiceApiDbAdapter.getInstance();

                try {
                    deviceDbAdapter.open();
                    DeviceInfoStruct deviceStruct = packetParser.getDeviceInfo();
                    //If Invalid XML Info (with respect to LG protocol including Port/Device ID in location xml), go to next loop
                    if (deviceStruct == null || TextUtils.isEmpty(deviceStruct.getServerPort()) || TextUtils.isEmpty(deviceStruct.getDeviceID()) ||
                            TextUtils.isEmpty(deviceStruct.getDeviceID()) || TextUtils.isEmpty(deviceStruct.getManufacturer()))
                        continue;

                    serviceApiDbAdapter.open();
                    ServiceStruct serviceStruct = packetParser.getServiceAPI();

                    //Invalid XML Info for services, go to next loop
                    if (serviceStruct == null || TextUtils.isEmpty(serviceStruct.getDeviceID()) || serviceStruct.getAPICount() <= 0)
                        continue;

                    if (deviceDbAdapter.hasDevice(deviceStruct.getDeviceID())) {
                        deviceDbAdapter.updateDevice(deviceStruct);
                        Log.i("ReceiveSSDPDeviceInfo", "Updated Device: " + deviceStruct.getServerIP() + ":" + deviceStruct.getServerPort());
                    } 
                    else {
                        deviceDbAdapter.insertDevice(deviceStruct);
                        Log.i("ReceiveSSDPDeviceInfo", "Inserted Device: " + deviceStruct.getServerIP() + ":" + deviceStruct.getServerPort());
                    }


                    ArrayList<APIStruct> apiList = serviceStruct.getAPIStructList();
                    String deviceID = serviceStruct.getDeviceID();
                    for (int i = 0; i < apiList.size(); i++) {
                        String apiType = apiList.get(i).getApiType();
                        ArrayList<String> apiNameList = apiList.get(i).getApiNameList();
                        for (int j = 0; j < apiNameList.size(); j++) {
                            String apiName = apiNameList.get(j);
                            if (!serviceApiDbAdapter.hasServiceAPI(deviceID, apiType, apiName))
                                serviceApiDbAdapter.insertServiceAPI(deviceID,apiType,apiName);
                        }
                    }

                } catch (SAXException e) {
                    Log.e("SSDPMainComponent", "ReceiveSSDPDeviceInfo SAXException", e);
                } catch (ParserConfigurationException e) {
                    Log.e("SSDPMainComponent", "ReceiveSSDPDeviceInfo ParserConfigurationException", e);
                }
                finally {
                    if (deviceDbAdapter.isOpen())
                        deviceDbAdapter.close();

                    if (serviceApiDbAdapter.isOpen())
                        serviceApiDbAdapter.close();
                }
            } catch (IOException e) {
                Log.e("SSDPMainComponent", "ReceiveSSDPDeviceInfo IOException", e);
            }
        }
    }
}

Already at this point, I've made things too complicated. Although I'm not 100% sure, I believe here are some things I'm doing wrong:

  • SSDPMainComponent is doing too many things. It's supposed to be the class that starts up all the adjunct components, but it's also parsing the XML data received as part of the other devices' M-SEARCH response.
  • The way I'm starting the threads seem awkward. Since android needs you to do any socket related work on a separate thread, perhaps it can't be avoided to a degree.
  • Just by looking at the private methods being called to initialize certain components (SendNotifyMessage, SendMSearchMessage, ReceiveSSDPDeviceInfo, ReceiveSSDPMessages), I can't quite tell which socket it will use (Listener/Multicast).
  • The namings in general seem very awkward. At a cursory glance, you can't quite tell what "MulticastSocket" or "ListenerSocket" is supposed to do in this application's context.
  • I think I'm violating quite a bit of KISS in many places too.

I guess I dragged on the question for too long but all I really want is some sort of guideline to make this code more elegant and understandable.

\$\endgroup\$
3
\$\begingroup\$

The SSDPMainComponent is doing four different jobs (the code for StartPeriodicNotify() seems to be missing though). Yet on closer inspection those jobs have very little in common. Some share the same socket, but that's it.

We can simply refactor SSDMainComponent into delegating these jobs to 4 helper classes. I've also used FutureTasks instead of joining on one Socket creating thread. (this postpones waiting for the thread, until we actually need one of the sockets, with any luck it'll be ready by the time we need it)

public class SSDPMainComponent {
    private FutureTask<SSDPMulticastSocket> multicastSocket;
    private FutureTask<SSDPListenerSocket> listenerSocket;
    private static final String ROOT_DEVICE_XML_URL_PATH = "/localService/xml/rootdevice.xml";
    private static URL ROOT_DEVICE_URL;

    public SSDPMainComponent(Context context) throws IOException {
        ROOT_DEVICE_URL = new URL("http://" + NetworkUtils.getLocalIpAddress(context)
                + ":" + RestfulWebServerConstants.WEB_SERVER_PORT
                + ROOT_DEVICE_XML_URL_PATH);
        multicastSocket = new FutureTask<>(new Callable<SSDPMulticastSocket>() {
            @Override
            public SSDPMulticastSocket call() throws Exception {
                return new SSDPMulticastSocket();
            }
        });
        new Thread(multicastSocket).start();
        listenerSocket = new FutureTask<>(new Callable<SSDPListenerSocket>() {
            @Override
            public SSDPListenerSocket call() throws Exception {
                return new SSDPListenerSocket();
            }
        });
        new Thread(listenerSocket).start();
    }

    public void StartPeriodicMSearch() throws InterruptedException {
        new SSDPPeriodicSearcher(getMulticastSocket()).startPeriodicMSearch();
    }

    public void StartReceivingSSDPDeviceInfo() throws InterruptedException {
        new SSDPInfoReceiver(getMulticastSocket()).startReceivingSSDPDeviceInfo();
    }

    public void StartReceivingSSDPMessages() throws InterruptedException {
        new SSDPReceiver(getListenerSocket(), ROOT_DEVICE_URL).StartReceivingSSDPMessages();
    }

    private SSDPMulticastSocket getMulticastSocket() throws InterruptedException {
        try {
            return multicastSocket.get();
        } catch (ExecutionException e) {
            throw new RuntimeException(e.getCause());
        }
    }

    private SSDPListenerSocket getListenerSocket() throws InterruptedException {
        try {
            return listenerSocket.get();
        } catch (ExecutionException e) {
            throw new RuntimeException(e.getCause());
        }
    }

}

In fact, it is now little more than a component to start creating the sockets. If we expose the getters for the sockets, we could simply move the content of the start...() methods to initSSDPComponent().

Of the helper classes, only SSDPInfoReceiver is doing too much. As you yourself already point out, it is doing the parsing and processing of the received datagram. But delegating this to a Parser and a Processor component will dramatically simplify SSDPInfoReceiver.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.