Green Millhouse - The Broadlink Binding part 3

Apologies to people who were interested in my Broadlink binding for OpenHAB 2.0 ... I migrated my code to the 2.4.0 family and things apparently stopped working. I was short on spare time so assumed something major had changed in the OpenHAB binding API and didn't have time to investigate. Turns out it was actually my own stupid fault, refactoring to clean up the code, I'd managed to cut out the "happy path" that actually updated OpenHAB with values from the device <facepalm />.

Anyway, back into it now with a couple of extra things rectified as well:

2.4.0-BETA-2.jar

This version also keeps track of the ScheduledFuture that is used to periodically poll the Broadlink device, and correctly calls cancel() on it if the Broadlink binding is getting disposed. This should put an end to those
...the handler was already disposed
errors flooding the logs.

2.4.0-BETA-3.jar

This version finally addresses the "reconnect" issues; i.e. when a device unexpectedly falls off the network, re-authentication with it would, more often than not, fail. The fix was to reset most of the state associated with the device once we lose contact with it. In particular, the packet counter, deviceId and deviceKey variables that we obtained from the previous authentication response.

As a result of this, my A1 Environmental Sensor seems to be working in all scenarios. Next up is long-term robustness testing while sorting out device discovery, making sure my RM3 Mini works, and checking the multiple heterogeneous devices will all play nicely together.

2.4.0-BETA-4.jar

This version reflects my testing with my RM3 Mini IR blaster. This device seems more sensitive to certain parameters in the authentication procedure - and it turned out that resetting the packet counter (as introduced in BETA-3) is not needed and can actually cause re-authentication to fail for this device. So there's actually less code in this release, but more functionality - which is always nice.
Device discovery is also working now too. From the OpenHAB Paper UI, go to
Configuration -> Things -> '+' -> Broadlink Binding and wait 10 seconds for the network broadcast to complete. If you have several Broadlink devices active on your network it may take a couple of scans to pick them all up.

2.4.0-BETA-5.jar

Small refactors for code cleanliness, e.g. grouping all networking functions together in NetworkUtils.java. Also added specific polling code for the SP3 smart switch device; prior to now this was using the same polling code as an SP2 device, but based on comments on this post from Jorg, it has its own function now, with extra diagnostic logging to hopefully pinpoint what is going on. I suspect that it doesn't reply in the same way as an SP2, so this is an "investigative" release; expect BETA-6 to follow with (hopefully) fixed support for the SP3. I may have to resort to buying one myself if we can't sort it out over the internet ;-)

2.4.0-BETA-6.jar

Fixed misidentification of an SP3 switch as an SP2 (appears to have been a typo in the original JAR). After investigating Jorg's incorrect polling issue, I took a look over at the python-broadlink module code for inspiration, and lo and behold, there was more going on there. This module seems to have had a lot of love (42 contributors!) and seems to have addressed all the quirks of these devices, so I had no hesitations in implementing the changes; namely that testing payload[4] == 1 is not sufficient. The Python code also checks against 3 and 0xFD. Looks like the LS bit of that byte is the one that matters, but if this works, that's fine!

2.4.0-BETA-7.jar

Added a ton more weird-and-wonderful variants of the RM2, courtesy of the aforementioned Python library. Broadlink seem to be constantly updating their device names (RM2, RM2 Pro Plus, RM2 Pro Plus R1, RM2 Pro Plus 2, etc etc), giving a new identification code to each one. I can't fathom a method to their coding scheme, so for the moment, we just have to play catch-up. In the case where we can't identify what seems to be a Broadlink device, I'm now logging (at ERROR level) the identification code we found, so that people out there can feature-request the support for their device.

2.4.0-BETA-8.jar

With thanks to excellent Github contributor FreddyFox, querying SP2/SP3 switch state should now be working. The old code attempted to decode the state of the switch from the encrypted payload, when of course it needs to be decrypted first. Thanks again FreddyFox!

2.4.0-BETA-9.jar

The main feature of this version is support for Dynamic IP Addresses.
There is a new switch available under the advanced Thing properties (open the SHOW MORE area in PaperUI) - it’s marked “Static IP” and it defaults to ON. If you are on a network where your Broadlink device might periodically be issued a different IP address, move it to the OFF position.
Now, if we lose communications with this Thing, rather than mark it OFFLINE, we’ll go into a mini-Discovery mode, and attempt to find the device on the network again, using its MAC address (which never changes). If we find the device, we update its current IP address in its Thing config, and everything continues without a hiccup. If we fail to find it, it goes OFFLINE as normal.
I should give credit to the LIFX binding which uses a very similar scheme - by chance I happened to come across it and realised it would be great for Broadlink devices too. As an extra bonus, it prompted me to redesign the entire discovery system and make it asynchronous; as a result it is now MUCH more reliable at finding devices (particularly if you have multiple Broadlink devices on your network) without having to scan repeatedly.

2.4.0-BETA-10.jar

This version fixes a few small issues:

• RM3 devices can now have a polling frequency specified (this was an omission from the thing-types.xml configuration file)
• Thing logging extracted to its own class and improved: Device status ONLINE/OFFLINE/undetermined shown in logs as ^/v/?
• Each ThingHandler's network socket is now explicitly closed when we lose contact with the device. This seems to help subsequent reconnections.

2.4.0-BETA-11.jar

This version attempts to improve general reliability. The Broadlink network protocol always acknowledges every command sent to a device, so logically, the sendPacket() and receivePacket() functions have been coalesced to sendAndReceivePacket(). This in turn allowed for a simple retry mechanism to be implemented. If we time out waiting for a response from the device, we immediately retry, sending the packet again. Together with some improved logging, this should hopefully be enough to fix (or at least understand) devices prematurely being marked "offline" on unreliable networks.

Here's the latest JAR anyway, put it into your addons directory and let me know in the comments how it goes for you: https://dl.bintray.com/themillhousegroup/generic/org.openhab.binding.broadlink-2.4.0-BETA-11.jar

Green Millhouse - Fixing the OpenHAB BroadLink Binding (part 2)

Development of the Broadlink OpenHAB binding has been going very well, but with just one scenario (and arguably the most-common one) not working reliably. With the OpenHAB server up-and-running and the Broadlink binding installed and configured for a particular disconnected device, turn the device on.

The copious logging I've added to the binding told me that this was resulting in an error with code -7 when the device was first detected, and I was a little stumped as to how to proceed. As many before me have done, I started entering random semi-related strings into Google to see what might turn up. And I hit gold. The author of the Python Broadlink library did some great work documenting the Broadlink protocol, and while perusing it a particular sentence jumped out at me:

You must obtain an authorisation key from the device before you can communicate.

Of course! When the binding first boots it calls authenticate for each device it knows about, and it does the same when a device setting is changed. But it does not when, after finding a newly-booted device, tries to get its status!

I added an authenticated boolean in the base Handler class, only setting it when authenticate() succeeds and clearing it whenever we lose contact with the device. Any attempt to communicate with the device first checks the boolean and does the authenticate() dance if necessary. And it works like a charm. We're very nearly there now - I just want to DRY up some of the payload-handling methods which seem to have been copy-pasted, add a 'presence' channel that mirrors the internal state of the binding, and remove the heavy dependence on IP addresses, and I think it's good to go.

Green Millhouse - Fixing the OpenHAB BroadLink Binding (part 1)

You can follow along at Github, but my rebuilding of the Broadlink OpenHAB binding is nearing completion.

I've been building and testing locally with my A1 Air Quality Sensor, and since fixing some shared-state issues in the network layer, haven't yet experienced any of the reliability problems that plagued the original binding.

For reasons that aren't clear (because I'm working from a decompiled JAR file), the original binding was set up like this in the base Thing handler (which all Broadlink Things inherit from):

public class BroadlinkBaseThingHandler extends BaseThingHandler {
   private static DatagramSocket socket = null;
   static boolean commandRunning = false;
   ...
}

As soon as I saw those static members, alarms started ringing in my head - especially when combined with an inheritance model, you've got a definite "fragile base class" problem at compile-time, and untold misery at runtime when multiple subclasses start accessing the socket like it's their exclusive property!

An attempt to mitigate the race-conditions which must have abounded, the commandRunning boolean only complicated matters:

    public boolean sendDatagram(byte message[])
    {
        try
        {
            if(socket == null || socket.isClosed())
            {
                socket = new DatagramSocket();
                socket.setBroadcast(true);
            }
            InetAddress host = InetAddress.getByName(thingConfig.getIpAddress());
            int port = thingConfig.getPort();
            DatagramPacket sendPacket = new DatagramPacket(message, message.length, new InetSocketAddress(host, port));
            commandRunning = true;
            socket.send(sendPacket);
        }
        catch(IOException e)
        {
            logger.error("IO error for device '{}' during UDP command sending: {}", getThing().getUID(), e.getMessage());
            commandRunning = false;
            return false;
        }
        return true;
    }

    public byte[] receiveDatagram()
    {
        try {
            socket.setReuseAddress(true);
            socket.setSoTimeout(5000);
        } catch (SocketException se) {
            commandRunning = false;
            socket.close();
            return null;
        }

        if(!commandRunning) {
            logger.error("No command running - device '{}' should not be receiving at this time!", getThing().getUID());
            return null;
        }

        try
        {
            if(socket != null)
            {
                byte response[] = new byte[1024];
                DatagramPacket receivePacket = new DatagramPacket(response, response.length);
                socket.receive(receivePacket);
                response = receivePacket.getData();
                commandRunning = false;
                socket.close();
                return response;
            }
        }
        catch (SocketTimeoutException ste) {
            if(logger.isDebugEnabled()) {
                logger.debug("No further response received for device '{}'", getThing().getUID());
            }
        }

        catch(Exception e)
        {
            logger.error("IO Exception: '{}", e.getMessage());
        }

        commandRunning = false;
        return null;
    }

So we got a pseudo-semaphore that is trying to detect getting into a bad state (because of shared-state), but itself is shared-state, thereby experiencing the same unreliability.

Here's what the new code looks like:

public class BroadlinkBaseThingHandler extends BaseThingHandler {
    private DatagramSocket socket = null;
    ...

    public boolean sendDatagram(byte message[], String purpose) {
        try {
            logTrace("Sending " + purpose);
            if (socket == null || socket.isClosed()) {
                socket = new DatagramSocket();
                socket.setBroadcast(true);
                socket.setReuseAddress(true);
                socket.setSoTimeout(5000);
            }
            InetAddress host = InetAddress.getByName(thingConfig.getIpAddress());
            int port = thingConfig.getPort();
            DatagramPacket sendPacket = new DatagramPacket(message, message.length, new InetSocketAddress(host, port));
            socket.send(sendPacket);
        } catch (IOException e) {
            logger.error("IO error for device '{}' during UDP command sending: {}", getThing().getUID(), e.getMessage());
            return false;
        }
        logTrace("Sending " + purpose + " complete");
        return true;
    }

    public byte[] receiveDatagram(String purpose) {
        logTrace("Receiving " + purpose);

        try {
            if (socket == null) {
                logError("receiveDatagram " + purpose + " for socket was unexpectedly null");
            } else {
                byte response[] = new byte[1024];
                DatagramPacket receivePacket = new DatagramPacket(response, response.length);
                socket.receive(receivePacket);
                response = receivePacket.getData();
//                socket.close();
                logTrace("Receiving " + purpose + " complete (OK)");
                return response;
            }
        } catch (SocketTimeoutException ste) {
            logDebug("No further " + purpose + " response received for device");
        } catch (Exception e) {
            logger.error("While {} - IO Exception: '{}'", purpose, e.getMessage());
        }

        return null;
    }    

A lot less controversial I'd say. The key changes:

• Each subclass instance (i.e. Thing) gets its own socket
• No need to track commandRunning - an instance owns its socket outright
• The socket gets configured just once, instead of being reconfigured between Tx- and Rx-time
• Improved diagnostic logging that always outputs the ThingID, and the purpose of the call

The next phase is now stress-testing the binding with a second heterogeneous device (sadly I don't have another A1, that would be great for further tests), my RM3 Mini IR-blaster. I'll be trying adding and removing the devices at various times, seeing if I can trip the binding up. The final step will be making sure the Thing discovery process (which is the main reason to upgrade to OpenHAB 2, and is brilliant) is as good as it can be. After that, I'll be tidying up the code to meet the OpenHAB guidelines and hopefully getting this thing into the official release!

Deep-diving into the Google pb= embedded map format

Building off the excellent work of Andrew Whitby, I wanted to go further in understanding this unusual format. Specifically because when trying to parse the lat-long of the marker out of it, the least couple of significant digits were always "off", and frustratingly, by a seemingly-random amount.

Let's take a look at a Google "Embed map" URL for a random lat-long. You can obtain one of these by clicking a random point on a Google Map, then clicking the lat-long hyperlink on the popup that appears at the bottom of the page. From there the map sidebar swings out; choose Share -> Embed map - that's your URL.

"https://www.google.com/maps/embed?pb=
!1m18!1m12!1m3!1d3152.8774048836685
!2d145.01352231578036
!3d-37.792912740624445!2m3!1f0!2f0!3f0!3m2!1i1024!2i768
!4f13.1!3m3!1m2!1s0x0%3A0x0!2zMzfCsDQ3JzM0LjUiUyAxNDXCsDAwJzU2LjYiRQ
!5e0!3m2!1sen!2sau!4v1471218824160"

Well, it's not pretty, but with the help of Andrew Whitby's cheat sheet and the comments from others, it turns out we can actually render it as a nested structure knowing that the format [id]m[n] means a structure (multi-field perhaps?) with n children in total - my IDE helped a lot here with indentation:

"https://www.google.com/maps/embed?pb=" +
    "!1m18" +
        "!1m12" +
            "!1m3" +
                "!1d3152.8774048836685" +
                "!2d145.01352231578036" +
                "!3d-37.792912740624445" +
            "!2m3" +
                "!1f0" +
                "!2f0" +
                "!3f0" +
            "!3m2" +
                "!1i1024" +
                "!2i768" +
            "!4f13.1" +
        "!3m3" +
            "!1m2" +
                "!1s0x0%3A0x0" +
                "!2zMzfCsDQ3JzM0LjUiUyAxNDXCsDAwJzU2LjYiRQ" +
        "!5e0" +
    "!3m2" +
        "!1sen" +
        "!2sau" +
    "!4v1471218824160"

It all (kinda) makes sense! You can see how a decoder could quite easily be able to count ! characters to decide that a bang-group (or could we call it an m-group?) has finished. I'm going to take a stab and say the e represents an enumerated type too - given that !5e0 is "roadmap" (default) mode and !5e1 forces "satellite" mode.

So this is all very well but it doesn't explain why URLs that I generate using the standard method don't actually put the lat-long I selected into the URL - yet they render perfectly! What do I mean? Well, the lat-long that I clicked on (i.e. the marker) for this example is actually:

               -37.792916,  145.015722

And yet in the URL it appears (kinda) as:

               -37.792912,  145.013522

Which is enough to be slightly, visibly, annoyingly, wrong if you're trying to use it as-is by parsing the URL. What I thought I needed to understand now was this section of the URL:

                "!1d3152.8774048836685" +
                "!2d145.01352231578036" +
                "!3d-37.792912740624445" +

Being the "scale" and centre points of the map. Then I realised - it's quite subtle, but for (one presumes) aesthetic appeal, Google doesn't put the map marker in the dead-centre of the map. So these co-ordinates are just the map centre. The marker itself is defined elsewhere. And there's only one place left. The mysterious z field:

   !2zMzfCsDQ3JzM0LjUiUyAxNDXCsDAwJzU2LjYiRQ

Sure enough, substituting the z-field from Mr. Whitby's example completely relocates the map to put the marker in the middle of Iowa. So now; how to decode this? Well, on a hunch I tried base64decode-ing it, and bingo:

% echo MzfCsDQ3JzM0LjUiUyAxNDXCsDAwJzU2LjYiRQ | base64 --decode
37°47'34.5"S 145°00'56.6"E

So there we have it. I can finally parse out the lat-long of the marker when given an embed URL. Hope it helps someone else out there...