fix: use provision watcher logic for discovered devices (#42)

#39

Signed-off-by: Anthony Casagrande <anthony.j.casagrande@intel.com>

internal/driver/discover.go

@@ -24,9 +24,10 @@ import (
 )
 
 const (
-	DefaultDevicePrefix = "LLRP"
-	UnknownVendorID     = 0
-	UnknownModelID      = 0
+	defaultDevicePrefix    = "LLRP"
+	unknownVendorID        = 0
+	unknownModelID         = 0
+	unknownFirmwareVersion = ""
 )
 
 // discoveryInfo holds information about a discovered device
@@ -36,6 +37,7 @@ type discoveryInfo struct {
 	port       string
 	vendor     uint32
 	model      uint32
+	fwVersion  string
 }
 
 // newDiscoveryInfo creates a new discoveryInfo with just a host and port pre-filled
@@ -236,7 +238,6 @@ func (info *discoveryInfo) updateExistingDevice(device contract.Device) error {
 			"oldInfo", fmt.Sprintf("%+v", tcpInfo),
 			"discoveredInfo", fmt.Sprintf("%+v", info))
 
-		// todo: double check to make sure EdgeX calls driver.UpdateDevice()
 		device.Protocols["tcp"] = map[string]string{
 			"host": info.host,
 			"port": info.port,
@@ -397,19 +398,21 @@ func probe(host, port string, timeout time.Duration) (*discoveryInfo, error) {
 	info := newDiscoveryInfo(host, port)
 	if readerCaps.GeneralDeviceCapabilities == nil {
 		driver.lc.Warn("ReaderCapabilities.GeneralDeviceCapabilities was nil, unable to determine vendor and model info")
-		info.vendor = UnknownVendorID
-		info.model = UnknownModelID
+		info.vendor = unknownVendorID
+		info.model = unknownModelID
+		info.fwVersion = unknownFirmwareVersion
 	} else {
 		info.vendor = readerCaps.GeneralDeviceCapabilities.DeviceManufacturer
 		info.model = readerCaps.GeneralDeviceCapabilities.Model
+		info.fwVersion = readerCaps.GeneralDeviceCapabilities.FirmwareVersion
 	}
 
 	if readerConfig.Identification == nil {
 		driver.lc.Warn("ReaderConfig.Identification was nil, unable to register discovered device.")
 		return nil, fmt.Errorf("unable to retrieve device identification")
 	}
 
-	prefix := DefaultDevicePrefix
+	prefix := defaultDevicePrefix
 	if VendorIDType(info.vendor) == Impinj {
 		prefix = ImpinjModelType(info.model).HostnamePrefix()
 	}
@@ -480,31 +483,44 @@ func newDiscoveredDevice(info *discoveryInfo) dsModels.DiscoveredDevice {
 		"RFID",
 		"LLRP",
 	}
+
+	// Add vendor string only if it is a known vendor
+	vendorStr := VendorIDType(info.vendor).String()
+	if !strings.HasPrefix(vendorStr, "VendorIDType(") {
+		labels = append(labels, vendorStr)
+	} else {
+		vendorStr = ""
+	}
+
+	// Add model string if it is available. Currently only supported for a select few Impinj devices.
+	var modelStr string
 	if VendorIDType(info.vendor) == Impinj {
-		labels = append(labels, Impinj.String())
-		modelStr := ImpinjModelType(info.model).String()
-		// only add the label if we know the model
+		ipjModelStr := ImpinjModelType(info.model).String()
 		if !strings.HasPrefix(modelStr, "ImpinjModelType(") {
 			labels = append(labels, modelStr)
+			modelStr = ipjModelStr
 		}
 	}
 
 	// Note that we are adding vendorPEN to the protocol properties in order to
 	// allow the provision watchers to be able to match against that info. Currently
 	// that is the only thing the provision watchers use for matching against.
-	//
-	// We would prefer to put the vendorPEN, and possibly model and fw version
-	// in a separate "protocol", possibly named "metadata", however there is a bug in the
-	// current logic that does not allow this.
-	//
-	// see: https://github.com/edgexfoundry/device-sdk-go/issues/598
 	return dsModels.DiscoveredDevice{
 		Name: info.deviceName,
 		Protocols: map[string]contract.ProtocolProperties{
 			"tcp": {
-				"host":      info.host,
-				"port":      info.port,
+				"host": info.host,
+				"port": info.port,
+			},
+			// we are adding a "metadata" protocol as a place to store general properties for
+			// matching against provision watchers. It is not an actual protocol, but
+			// the recommended way of adding generic key/value pairs to a device.
+			"metadata": {
 				"vendorPEN": strconv.FormatUint(uint64(info.vendor), 10),
+				"vendorStr": vendorStr,
+				"model":     strconv.FormatUint(uint64(info.model), 10),
+				"modelStr":  modelStr,
+				"fwVersion": info.fwVersion,
 			},
 		},
 		Description: "LLRP RFID Reader",

internal/driver/discover_test.go

@@ -194,13 +194,22 @@ func TestAutoDiscoverNaming(t *testing.T) {
 			if discovered[0].Name != test.name {
 				t.Errorf("expected discovered device's name to be %s, but was: %s", test.name, discovered[0].Name)
 			}
-			pen, ok := discovered[0].Protocols["tcp"]["vendorPEN"]
+
+			pen, ok := discovered[0].Protocols["metadata"]["vendorPEN"]
 			if !ok {
-				t.Fatalf("Missing vendorPEN field in tcp protocol. ProtocolMap: %+v", discovered[0].Protocols)
+				t.Fatalf("Missing vendorPEN field in metadata protocol. ProtocolMap: %+v", discovered[0].Protocols)
 			}
 			if pen != strconv.FormatUint(uint64(test.caps.DeviceManufacturer), 10) {
 				t.Errorf("expected vendorPEN to be %v, but was: %s", test.caps.DeviceManufacturer, pen)
 			}
+
+			fw, ok := discovered[0].Protocols["metadata"]["fwVersion"]
+			if !ok {
+				t.Fatalf("Missing fwVersion field in metadata protocol. ProtocolMap: %+v", discovered[0].Protocols)
+			}
+			if fw != test.caps.FirmwareVersion {
+				t.Errorf("expected fwVersion to be %v, but was: %s", test.caps.FirmwareVersion, fw)
+			}
 		})
 	}
 }

internal/driver/driver.go

@@ -54,13 +54,10 @@ const (
 	AttribVendor   = "vendor"
 	AttribSubtype  = "subtype"
 
-	// Note: For now disable the registration of provision watchers since we are not using them
-	registerProvisionWatchers = false
+	// enable this by default, otherwise discovery will not work.
+	registerProvisionWatchers = true
 	provisionWatcherFolder    = "res/provision_watchers"
 
-	GenericDeviceProfile = "Device.LLRP.Profile"
-	ImpinjDeviceProfile  = "Impinj.LLRP.Profile"
-
 	// discoverDebounceDuration is the amount of time to wait for additional changes to discover
 	// configuration before auto-triggering a discovery
 	discoverDebounceDuration = 10 * time.Second
@@ -892,50 +889,7 @@ func (d *Driver) discover(ctx context.Context) {
 		d.lc.Warn("Discover process has been cancelled!", "ctxErr", ctx.Err())
 	}
 
-	// Note: We have to send data over this channel to let the SDK know we are done discovering.
-	// see: https://github.com/edgexfoundry/device-sdk-go/issues/609
-	d.deviceCh <- nil
 	d.lc.Info(fmt.Sprintf("Discovered %d new devices in %v.", len(result), time.Now().Sub(t1)))
-
-	// Note: For now we have to resort to adding our discovered devices ourselves due to multiple bugs in the
-	// provision watcher code, as well as no clear way to tell if a device was matched by a PW or not.
-	// see: https://github.com/edgexfoundry/device-sdk-go/issues/598
-	// see also: https://github.com/edgexfoundry/device-sdk-go/issues/606
-	for _, discovered := range result {
-		if _, err := d.registerDevice(discovered); err != nil {
-			d.lc.Error("Error adding device.", "name", discovered.Name, "error", err)
-		}
-	}
-}
-
-func (d *Driver) registerDevice(discovered dsModels.DiscoveredDevice) (id string, err error) {
-	profile := GenericDeviceProfile
-	// Note: This is a bit of a workaround based on provision watcher logic. It was only left
-	// this way (as opposed to a total refactor) in order to allow a smooth transition
-	// back to provision watchers once the assortment of bugs have been fixed.
-	if discovered.Protocols["tcp"]["vendorPEN"] == strconv.FormatUint(uint64(Impinj), 10) {
-		profile = ImpinjDeviceProfile
-	}
-	// remove the field as it is no longer needed/useful
-	delete(discovered.Protocols["tcp"], "vendorPEN")
-
-	return d.svc.AddDevice(contract.Device{
-		DescribedObject: contract.DescribedObject{
-			Timestamps: contract.Timestamps{
-				Origin: time.Now().UnixNano(),
-			},
-			Description: discovered.Description,
-		},
-		Name: discovered.Name,
-		Profile: contract.DeviceProfile{
-			Name: profile,
-		},
-		Protocols: discovered.Protocols,
-		Labels:    discovered.Labels,
-		Service: contract.DeviceService{
-			Name: ServiceName,
-		},
-		AdminState:     contract.Unlocked,
-		OperatingState: contract.Enabled,
-	})
+	// pass the discovered devices to the EdgeX SDK to be passed through to the provision watchers
+	d.deviceCh <- result
 }

internal/driver/types.go

@@ -39,6 +39,6 @@ func (imt ImpinjModelType) HostnamePrefix() string {
 	case XArray, XArrayEAP, XArrayWM:
 		return "xArray"
 	default:
-		return DefaultDevicePrefix
+		return defaultDevicePrefix
 	}
 }