Remote device list bug when a device has new network information

[joolz]

Suppose a device of a given instance is discovered (say device 100, which is on 192.168.0.30), it gets added to the remoteDevices list in the LocalDevice object.

Now if that IP address for device 100 gets updated (to say 192.168.0.31), when it is discovered by BACnet4j again, it checks the instance, address and network to see if it exists in the removeDevices list. Because it is different, the check fails, and it gets added to the remoteDevices list again, with new network information.

The problem is that there are now two devices in the remoteDevices list which have the same device instance. If BACnet4j were to send out a request to device 100, it will used the old device network information, not the new one. It is worse if a device is accessed via an MSTP network, and is moved from one device to another.

I've come up a fix to resolve the issue. I am having issues with accessing CVS for some reason, and because of that it isn't letting me create a patch, so here is a before and after:
com.serotonin.bacnet4j.LocalDevice.java:570

    private RemoteDevice getRemoteDeviceImpl(int instanceId, Address address, Network network) {
        for (RemoteDevice d : remoteDevices) {
            if (d.getInstanceNumber() == instanceId && d.getAddress().equals(address)
                    && ObjectUtils.equals(d.getNetwork(), network))
                return d;
        }
        return null;
    }

Changed to:

    private RemoteDevice getRemoteDeviceImpl(int instanceId, Address address, Network network) {
        for (RemoteDevice d : remoteDevices) {
            if (d.getInstanceNumber() == instanceId) {
            	if (d.getAddress().equals(address) && ObjectUtils.equals(d.getNetwork(), network))
            		return d;
            	else
            		remoteDevices.remove(d);
            }
        }
        return null;
    }

This will ensure that there cannot be two devices of the same instance in the remoteDevices list (which must be unique on the network as per the standard). Alternatively, the check could be done in getRemoteDeviceCreate() instead if the getRemoteDevice() function is used and removing it from the list is undesirable, however I found no reference to that function in BACnet4j and figured doing it this way would be more efficient.

[mlohbihler]

Thanks joolz. The code originally only checked the device id, in accordance with the spec. However we found that in practice folks regularly and happily have devices with the same device ids on their networks. I would recommend implementing a "strict" setting that could branch code so that users could choose their level of spec compliance.

[joolz]

I am a little taken aback hearing that there are people using the same device instance for multiple devices on a network. They should be shot!

From what I can see however, with the code as is, even if they were happy to violate (as personally I don't think there is any level of compliance in this department) the standard and have same device instance for multiple devices, it will only ever work with the first device in the list if BACnet4j has to make any sort of request to that device (I could be wrong, but that is my observation). So I'm not really sure how useful the current implementation is to anyone, as it doesn't really handle multiple devices, and it doesn't handle devices that change network details.
I agree a flag in the localDevice object would be the simplest way to accomodate all parties, so that particular behaviour could be specified.