Clean up handling of lat/long geo-coordinates.

Change-Id: I64fca56c7deb9a8baa6c68558365ec2a8c38168c

@@ -30,12 +30,13 @@ public abstract class BasicElementConfig<S> extends AllowedEntityConfig<S> {
     protected static final String RACK_ADDRESS = "rackAddress";
     protected static final String OWNER = "owner";
 
-    protected static final double DEFAULT_COORD = -1.0;
+    protected static final double ZERO_THRESHOLD = Double.MIN_VALUE * 2.0;
+    private static final double DEFAULT_COORD = 0.0;
 
     /**
      * Returns friendly label for the element.
      *
-     * @return friendly label or element id itself if not set
+     * @return friendly label or element identifier itself if not set
      */
     public String name() {
         return get(NAME, subject.toString());
@@ -51,10 +52,25 @@ public abstract class BasicElementConfig<S> extends AllowedEntityConfig<S> {
         return (BasicElementConfig) setOrClear(NAME, name);
     }
 
+    private static boolean doubleIsZero(double value) {
+        return value >= -ZERO_THRESHOLD && value <= ZERO_THRESHOLD;
+    }
+
+    /**
+     * Returns true if the geographical coordinates (latitude and longitude)
+     * are set on this element.
+     *
+     * @return true if geo-coordinates are set
+     */
+    public boolean geoCoordsSet() {
+        return !doubleIsZero(latitude()) || !doubleIsZero(longitude());
+    }
+
     /**
      * Returns element latitude.
      *
-     * @return element latitude; -1 if not set
+     * @return element latitude; 0.0 if (possibly) not set
+     * @see #geoCoordsSet()
      */
     public double latitude() {
         return get(LATITUDE, DEFAULT_COORD);
@@ -71,9 +87,10 @@ public abstract class BasicElementConfig<S> extends AllowedEntityConfig<S> {
     }
 
     /**
-     * Returns element latitude.
+     * Returns element longitude.
      *
-     * @return element latitude; -1 if not set
+     * @return element longitude; 0 if (possibly) not set
+     * @see #geoCoordsSet()
      */
     public double longitude() {
         return get(LONGITUDE, DEFAULT_COORD);

+/*
+ * Copyright 2016-present Open Networking Laboratory
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.onosproject.net.config.basics;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.onosproject.net.config.basics.BasicElementConfig.ZERO_THRESHOLD;
+
+/**
+ * Unit tests for {@link BasicElementConfig}.
+ */
+public class BasicElementConfigTest {
+
+    private static final ObjectMapper MAPPER = new ObjectMapper();
+
+    private static final String E1 = "e1";
+
+    // concrete subclass of abstract class we are testing
+    private static class ElmCfg extends BasicElementConfig<String> {
+        ElmCfg() {
+            object = MAPPER.createObjectNode();
+        }
+
+        @Override
+        public String toString() {
+            return object.toString();
+        }
+    }
+
+    private static void print(String fmt, Object... args) {
+        System.out.println(String.format(fmt, args));
+    }
+
+    private static void print(Object o) {
+        print("%s", o);
+    }
+
+    private BasicElementConfig<?> cfg;
+
+    @Before
+    public void setUp() {
+        cfg = new ElmCfg().name(E1);
+    }
+
+    @Test
+    public void basicNoGeo() {
+        print(cfg);
+        assertFalse("geo set?", cfg.geoCoordsSet());
+        assertEquals("lat", 0.0, cfg.latitude(), ZERO_THRESHOLD);
+        assertEquals("lon", 0.0, cfg.longitude(), ZERO_THRESHOLD);
+    }
+
+    @Test
+    public void geoLatitudeOnly() {
+        cfg.latitude(0.1);
+        print(cfg);
+        assertTrue("geo NOT set", cfg.geoCoordsSet());
+        assertEquals("lat", 0.1, cfg.latitude(), ZERO_THRESHOLD);
+        assertEquals("lon", 0.0, cfg.longitude(), ZERO_THRESHOLD);
+    }
+
+    @Test
+    public void geoLongitudeOnly() {
+        cfg.longitude(-0.1);
+        print(cfg);
+        assertTrue("geo NOT set", cfg.geoCoordsSet());
+        assertEquals("lat", 0.0, cfg.latitude(), ZERO_THRESHOLD);
+        assertEquals("lon", -0.1, cfg.longitude(), ZERO_THRESHOLD);
+    }
+
+    @Test
+    public void geoLatLong() {
+        cfg.latitude(3.1415).longitude(2.71828);
+        print(cfg);
+        assertTrue("geo NOT set", cfg.geoCoordsSet());
+        assertEquals("lat", 3.1415, cfg.latitude(), ZERO_THRESHOLD);
+        assertEquals("lon", 2.71828, cfg.longitude(), ZERO_THRESHOLD);
+    }
+}

@@ -34,8 +34,6 @@ import java.util.Set;
  */
 public final class BasicHostOperator implements ConfigOperator {
 
-    protected static final double DEFAULT_COORD = -1.0;
-
     private BasicHostOperator() {
     }
 
@@ -81,10 +79,8 @@ public final class BasicHostOperator implements ConfigOperator {
         if (cfg.name() != null) {
             newBuilder.set(AnnotationKeys.NAME, cfg.name());
         }
-        if (cfg.latitude() != DEFAULT_COORD) {
+        if (cfg.geoCoordsSet()) {
             newBuilder.set(AnnotationKeys.LATITUDE, Double.toString(cfg.latitude()));
-        }
-        if (cfg.longitude() != DEFAULT_COORD) {
             newBuilder.set(AnnotationKeys.LONGITUDE, Double.toString(cfg.longitude()));
         }
         if (cfg.rackAddress() != null) {

@@ -54,18 +54,20 @@ public class BasicHostOperatorTest {
     private static final BasicHostConfig BHC = new BasicHostConfig();
     private static final String NAME = "testhost";
     private static final double LAT = 40.96;
+    private static final double LON = 0.0;
 
     @Before
     public void setUp() {
         BHC.init(ID, "test", JsonNodeFactory.instance.objectNode(), mapper, delegate);
         BHC.name(NAME).latitude(40.96);
+        // if you set lat or long, the other becomes valid as 0.0 (not null)
     }
 
     @Test
     public void testDescOps() {
         HostDescription desc = BasicHostOperator.combine(BHC, HOST);
         assertEquals(NAME, desc.annotations().value(AnnotationKeys.NAME));
-        assertEquals(null, desc.annotations().value(AnnotationKeys.LONGITUDE));
+        assertEquals(String.valueOf(LON), desc.annotations().value(AnnotationKeys.LONGITUDE));
         assertEquals(String.valueOf(LAT), desc.annotations().value(AnnotationKeys.LATITUDE));
     }
 }

@@ -7,12 +7,15 @@ onos ${host} null-simulation stop custom
 onos ${host} wipe-out please
 onos ${host} null-simulation start custom
 
+# null-create-device <type> <name> <#ports> <latitude> <longitude>
+# null-create-link <type> <src> <dst>
+
 onos ${host} <<-EOF
 
-null-create-device switch s1 10 0 0
-null-create-device switch s2 10 0 0
-null-create-device switch s3 10 0 0
-null-create-device switch s4 10 0 0
+null-create-device switch s1-Bristol 10 51.4500 -2.5833
+null-create-device switch s2-London 10 51.5072 -0.1275
+null-create-device switch s3-Dover 10 51.1295 1.3089
+null-create-device switch s4-Brighton 10 50.8429 -0.1313
 null-create-device switch s5 10 0 0
 null-create-device switch s6 10 0 0
 null-create-device switch s7 10 0 0
@@ -20,11 +23,11 @@ null-create-device switch s8 10 0 0
 null-create-device switch s9 10 0 0
 # null-create-device switch s10 10 0 0
 
-null-create-link direct s1 s2
-null-create-link direct s2 s3
-null-create-link direct s2 s4
-null-create-link direct s3 s4
-null-create-link direct s3 s5
+null-create-link direct s1-Bristol s2-London
+null-create-link direct s2-London s3-Dover
+null-create-link direct s2-London s4-Brighton
+null-create-link direct s3-Dover s4-Brighton
+null-create-link direct s3-Dover s5
 null-create-link direct s6 s5
 null-create-link direct s6 s8
 null-create-link direct s7 s9

@@ -31,7 +31,6 @@ import org.onosproject.incubator.net.tunnel.OpticalTunnelEndPoint;
 import org.onosproject.incubator.net.tunnel.Tunnel;
 import org.onosproject.incubator.net.tunnel.TunnelService;
 import org.onosproject.mastership.MastershipService;
-import org.onosproject.net.ElementId;
 import org.onosproject.net.Annotated;
 import org.onosproject.net.AnnotationKeys;
 import org.onosproject.net.Annotations;
@@ -40,6 +39,7 @@ import org.onosproject.net.DefaultEdgeLink;
 import org.onosproject.net.Device;
 import org.onosproject.net.DeviceId;
 import org.onosproject.net.EdgeLink;
+import org.onosproject.net.ElementId;
 import org.onosproject.net.Host;
 import org.onosproject.net.HostId;
 import org.onosproject.net.HostLocation;
@@ -77,8 +77,8 @@ import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 import java.util.Optional;
+import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 
 import static com.google.common.base.Preconditions.checkNotNull;
@@ -94,6 +94,8 @@ import static org.onosproject.ui.topo.TopoUtils.compactLinkString;
  */
 public abstract class TopologyViewMessageHandlerBase extends UiMessageHandler {
 
+    private static final String NO_GEO_VALUE = "0.0";
+
     // default to an "add" event...
     private static final DefaultHashMap<ClusterEvent.Type, String> CLUSTER_EVENT =
             new DefaultHashMap<>("addInstance");
@@ -361,10 +363,10 @@ public abstract class TopologyViewMessageHandlerBase extends UiMessageHandler {
 
         String slng = annotations.value(AnnotationKeys.LONGITUDE);
         String slat = annotations.value(AnnotationKeys.LATITUDE);
-        boolean haveLng = slng != null && !slng.isEmpty();
-        boolean haveLat = slat != null && !slat.isEmpty();
-        try {
-            if (haveLng && haveLat) {
+        boolean validLng = slng != null && !slng.equals(NO_GEO_VALUE);
+        boolean validLat = slat != null && !slat.equals(NO_GEO_VALUE);
+        if (validLat && validLng) {
+            try {
                 double lng = Double.parseDouble(slng);
                 double lat = Double.parseDouble(slat);
                 ObjectNode loc = objectNode()
@@ -372,11 +374,9 @@ public abstract class TopologyViewMessageHandlerBase extends UiMessageHandler {
                         .put("lng", lng)
                         .put("lat", lat);
                 payload.set("location", loc);
-            } else {
-                log.trace("missing Lng/Lat: lng={}, lat={}", slng, slat);
+            } catch (NumberFormatException e) {
+                log.warn("Invalid geo data: longitude={}, latitude={}", slng, slat);
             }
-        } catch (NumberFormatException e) {
-            log.warn("Invalid geo data: longitude={}, latitude={}", slng, slat);
         }
     }