refactoring for Delhi release from IOTech #74

cloudxxx8 · 2018-10-16T16:38:03Z

fix #67 #27 #25 #24 #22 #21 #20 #16 #12 #10 #9 #7

iain-anderson · 2018-10-17T14:28:11Z

This looks largely okay to me. I suggest we approve, and consider the following issues for subsequent work:

Mappings are applied the same way in get and set operations; one should be the reverse of the other. (Note, mappings are not in scope for Delhi)
In TransformReadParameter and TransformWriteParameter we repeatedly convert to float64 and back. Better to convert once, apply any applicable transforms and then covert back.
Do we need to concern ourselves with Schedules and ScheduleEvents after startup, ie once we have uploaded anything necessary to metadata and instantiated any local schedules. Can we therefore eliminate the caches for these items.
In the command handler we get the type name for a parameter by looking in the ValueDescriptor. If instead we get it from the DeviceObject we may be able to remove much of the ValueDescriptor handling code (will still need to be able to create one when creating / modifying a Device Profile).

Also, I didn't see an implementation of #23 here, and with respect to #20 is a literal string comparison what is required? I had assumed that the assertion string would be an expression of some sort.

cloudxxx8 · 2018-10-17T14:48:38Z

@iain-anderson , thanks for your review. Please see my feedback below:

We don't need to reverse the mapping in the code, because get and set are different "ResourceOperation", and they should be defined mappings reversely in profile if this resource allows both get and set.
Got it, I've thought about it. Let's improve it later.
The schedule execution needs the schedule and schedule event cache, which hasn't been checked in.
Yeah, let's see whether we have change to eliminate ValueDescriptor from cache. However, it's relatively small.
You are right. I've removed Implement binary encoding of Events & Readings #23 here. For Implement Reading assertions #20, I don't really understand this requirement, so I just followed the Java code and requirement document to implement for now.

Assertions
Assertions are another attribute in a device resource's value PropertyValue which specify a string value which the result is compared against. If the comparison fails, then the result is set to a string of the form “Assertion failed for device resource: , with value: ”, this also has a side-effect of setting the device operatingstate to DISABLED. In the case of the single device /command endpoint, a 500 status code is also returned. If the all form of the /command endpoint is used an assertion failure will return a 200 status, and it's the responsibility of the client to check each reading for assertion failures.

iain-anderson · 2018-10-17T15:27:56Z

1. We don't need to reverse the mapping in the code, because get and set are different "ResourceOperation", and they should be defined mappings reversely in profile if this resource allows both get and set.

Yes, good point.

jduranf

Just reviewed half of the work, I will try to review the rest in this week.
Very nice work in this huge PR!

jduranf · 2018-10-17T20:47:05Z

internal/cache/profiles.go

+}
+
+type profileCache struct {
+	dpMap    map[string]models.DeviceProfile //in dpMap, key is Device name, and value is DeviceProfile instance


key is Device Profile name, not Device name

Got it and modified.

jduranf · 2018-10-17T20:51:05Z

internal/cache/profiles.go

+	setResult := make(map[string][]models.ResourceOperation, len(profileResources))
+	for _, pr := range profileResources {
+		getResult[pr.Name] = pr.Get
+		setResult[pr.Name] = pr.Set


Shouldn't be checked if pr.Set or pr.Get is not defined?

Got it and added checking

jduranf · 2018-10-17T20:55:29Z

internal/cache/profiles.go

+}
+
+func (p *profileCache) Update(profile models.DeviceProfile) error {
+	name, ok := p.nameMap[profile.Id.Hex()]


Why not just call RemoveByName() and then Add()? I think this will simplify the code maintenance.
This can be applied to all other caches.

Sounds better. I am modifying it, but it should use RemoveById instead of RemoveByName because the name might be changed.

jduranf · 2018-10-17T20:58:21Z

internal/cache/profiles.go

+		if rosMap, ok = p.getOpMap[prfName]; !ok {
+			return nil, fmt.Errorf("profiles: ResourceOperations: specified profile: %s not found", prfName)
+		}
+	} else {


Shouldn't be safer to do } else if strings.ToLower(method) == "set" {?

Yes, it's safer. Modified it.

jduranf · 2018-10-17T21:17:12Z

internal/clientinit/clientinitializer.go

+	if common.UseRegistry {
+		if !checkServiceAvailableByConsul(common.CurrentConfig.Clients[serviceId].Name) {
+			time.Sleep(10 * time.Second)
+			checkServiceAvailable(serviceId)


Instead of calling itself, why not use a for loop like this one?

for { if common.UseRegistry { if checkServiceAvailableByConsul(common.CurrentConfig.Clients[serviceId].Name) == true { return } } else { if checkServiceAvailableByPing(serviceId) == nil { return } } time.Sleep(10 * time.Second) }

agree, and modified

jduranf · 2018-10-17T21:24:48Z

internal/common/consts.go

+	APIScheduleEventRoute   = APIPrefix + "/scheduleevent"
+	APIEventRoute           = APIPrefix + "/event"
+	APILoggingRoute         = APIPrefix + "/logs"
+	APIPingRoute            = APIPrefix + "/ping"


Instead of using those const, should be better to use the ones defined in github.com/edgexfoundry/edgex-go/pkg/clients/constants.go

Got it and modified

cloudxxx8 · 2018-10-18T14:11:29Z

@jduranf , thanks for the review
I am traveling tomorrow, and will take care of your suggestion on Saturday.

internal/handler/command.go

cloudxxx8 · 2018-10-20T11:37:59Z

@jduranf
I've modified and squashed the code as all you suggested.
Thank you so much.

tonyespy · 2018-10-20T23:25:47Z

I'm working my way through your commit. I'm traveling tomorrow, so should have plenty of time on the plane to finish my review.

cloudxxx8 · 2018-10-21T05:54:40Z

just pushed a scheduling mechanism in a separated commit to fix #79

async.go

tonyespy · 2018-10-23T07:26:41Z

async.go

+			if common.CurrentConfig.Device.DataTransform {
+				err := transformer.TransformReadResult(cv, do.Properties.Value)
+				if err != nil {
+					common.LogCli.Error(fmt.Sprintf("CommandValue (%s) transformed failed: %v", cv.String(), err))


the first two log messages use the function name as a prefix, but this one doesn't.

if the transform fails, why push?

thanks, modified the log message
by the way, I opened an issue about unifying the log message format, and will fix all of them

you are right.
I skip the transform failed reading now

tonyespy · 2018-10-23T07:28:35Z

async.go

-			svc.lc.Error(msg)
+			err := transformer.CheckAssertion(cv, do.Properties.Value.Assertion, &device)
+			if err != nil {
+				common.LogCli.Error(fmt.Sprintf("Assertion failed for Device Resource: %s, with value: %s", cv.String(), err))


the first two log messages use the function name as a prefix, but this one doesn't. also, if the assertion fails, why push?

you are right, modified as the previous conversation

async.go

example/cmd/simple-device/main.go

internal/cache/profiles.go

tonyespy

Overall, the rewrite looks really good (aside from the size of the PR)! Please review my comments and and make changes where appropriate.

I would also like to see a more informative commit message. Please review the "Top four things to do (or not do) on the following page:

https://wiki.edgexfoundry.org/display/FA/Committing+Code+Guidelines

If you need help with suggested wording of the commit message, please let me know. It also might be a good idea to include the "fixes issues #x" comments in the body of the commit message vs. just in the PR description.

example/cmd/simple-device/res/configuration.toml

internal/scheduler/executor.go

internal/transformer/transformparam.go

internal/transformer/transformresult.go

model/asyncvalues.go

model/protocoldriver.go

cloudxxx8 · 2018-10-31T06:26:14Z

@tonyespy

this way causes a bug

func (s *scheduleEventCache) All() []models.ScheduleEvent {
	ses := make([]models.ScheduleEvent, len(s.seMap))
	for _, schEvt := range s.seMap {
		ses = append(ses, schEvt)
	}
	return ses
}

the initial elements of the slice are empty and new element will be appended to the end of the slice.
modified the code back

tonyespy

There are a couple of additional problems noted which I think should be fine to track via issues. The only things I'd still really like to see fixed are:

the commit message (see below for a suggested wording)
requiring use of the startup package
service name in the config file

Let's see if we can work these out during tomorrow's WG meeting.

As for the commit message, here's a suggested wording:

Major re-factor of the SDK for initial release

This commit is a major re-factor of the SDK code prior to it's first
release. Major changes include:

re-organization of packaging structure, private code moved under /internal

new public functions for add/remove/update devices & profiles

scheduling

assertion, transform, and mapping support

You can leave the "fixed" line as is at below the above text (or just remove it), although in the future, you need to prefix each issue # with a keyword for it to be auto-closed (eg. Fixes #X, fixes #Y, and fixes #Z). Otherwise, you'll need to manually close the issues using the github UI.

tonyespy · 2018-11-04T00:23:55Z

async.go

+			err := transformer.CheckAssertion(cv, do.Properties.Value.Assertion, &device)
+			if err != nil {
+				common.LoggingClient.Error(fmt.Sprintf("processAsyncResults - Assertion failed for Device Resource: %s, with value: %s, %v", cv.RO.Object, cv.String(), err))
+				continue


Per the device requirements, when an assertion fails, the result in the Reading should be set to the string "Assertion failed for device resource: , with value: ". We might want to also add the assertion string as used in the message logged by CheckAssertion itself.

I'm fine if we track this via a new issue...

In this case, we still add the reading when assertion failed.
modified as your suggestion. The string in the reading is:
"Assertion failed for device resource, with value: %s and assertion: %s"

tonyespy · 2018-11-04T00:27:01Z

async.go

+				newCV, ok := transformer.MapCommandValue(cv)
+				if ok {
+					cv = newCV
+				} else {


If there's no mapping for a reading, is it really supposed to fail, or does the reading just pass thru "as is"? This was never really defined anywhere AFAIK.

The whole code is as follows:

if len(cv.RO.Mappings) > 0 { newCV, ok := transformer.MapCommandValue(cv) if ok { cv = newCV } else { common.LoggingClient.Error(fmt.Sprintf("processAsyncResults - Mapping failed for Device Resource Operation: %v, with value: %s, %v", cv.RO, cv.String(), err)) continue } }

if there is no mapping defined, the map transform won't happen

Can you add an issue to track this? I'm still not sure what the behavior should be if there's no mapping found for a reading. I would think that the reading should just pass thru then "as is", but here we're logging a message and then continuing on to the next result.

It looks like I misunderstood your comment. I've removed "continue" for now, so it will be "as is" and modified the Log to Warning level.

#89 is opened

tonyespy · 2018-11-04T00:28:28Z

async.go

+
+		// push to Core Data
+		event := &models.Event{Device: acv.DeviceName, Readings: readings}
+		go common.SendEvent(event)


If SendEvent fails, you can't tell from the resulting log message that it was triggered from an async reading (vs. a command endpoint reading).

I'm fine if we track this with an issue...

SendEvent function logs the error message when it failed

func SendEvent(event *models.Event) { _, err := EventClient.Add(event) if err != nil { LoggingClient.Error(fmt.Sprintf("Failed to push event for device %s: %s", event.Device, err)) } }

Sure, but as I mentioned, you can't tell from the log message whether the send failed due to an async result or from a call to the command endpoint.

I modified the code to send the event in the async.go to replace go common.SendEvent()

_, err := common.EventClient.Add(event) if err != nil { common.LoggingClient.Error(fmt.Sprintf("processAsyncResults - Failed to push event %v: %s", event, err)) }

example/cmd/simple-device/res/configuration.toml

internal/cache/profiles.go

tonyespy · 2018-11-04T20:53:19Z

internal/transformer/transformparam.go

+		valueFloat64 = v
+	}
+
+	valueFloat64 = math.Log(valueFloat64) / math.Log(b)


It looks like this is doing the inverse of a base transform for a read value. As this wasn't documented anywhere (i.e. the device requirements document), it would be nice to at least add comments inline explaining why this is so. Same applies for offset & scale.

If you want to track this by issue, that's fine w/me. We should also add this into the device requirements document and release a new version.

added comment inline
we could discuss whether we should remove this in the WG meeting
math.Log only accepts float64

updated to use internal constant in profile cache

tonyespy · 2018-11-04T21:01:49Z

internal/transformer/transformresult.go

+			return value, err
+		}
+		ns := uint8(s)
+		value = v * ns


What happens if the result is too large to fit into value? The device services requirements document states that similar to an assertion, the result string should be set to "Overflow failed for device resource: ". This applies to offsets too.

opened #88 for this

tonyespy · 2018-11-04T21:21:47Z

internal/cache/devices.go

+	return dc
+}
+
+func Devices() DeviceCache {


Accessing the caches this way means there's always a function call (unless the compiler can optimize this) whenever a cache is accessed vs. the old way of just using globals that were created via singletons. If you create an issue for my previous comment about sync.Once vars, you should include this in the same issue.

it's a trade off when I made it as a separated package. I don't think it is an issue, but please feel free to open it.

tonyespy · 2018-11-04T21:28:26Z

pkg/startup/bootstrap.go

+
+	//flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) // clean up existing flag defined by other code
+	flag.BoolVar(&common.UseRegistry, "registry", false, "Indicates the service should use the registry.")
+	flag.BoolVar(&common.UseRegistry, "r", false, "Indicates the service should use registry.")


Directly setting an internal var like this means that it's not even possible to use device.NewService without using the boostrap package! Usage of registry should be a parameter to NewService like it was previously.

Sorry, I missed it. Added it back now.

This commit is a major re-factor of the SDK code prior to it's first release. Major changes include: * re-organization of packaging structure, private code moved under /internal * new public functions for add/remove/update devices & profiles scheduling * assertion, transform, and mapping support fixed issue #67 #27 #25 #24 #22 #21 #20 #16 #12 #10 #9 #7 #79 Signed-off-by: Cloud Tsai <cloudxxx8@gmail.com>

cloudxxx8 · 2018-11-06T02:22:32Z

@tonyespy I've moved service name in to source code. Please make a final review of this PR when you have time. After merging, I will start working on #44 and #81 and try to open new PR by this week

tonyespy

Approved

tonyespy · 2018-11-06T16:53:20Z

async.go

+				err := transformer.TransformReadResult(cv, do.Properties.Value)
+				if err != nil {
+					common.LoggingClient.Error(fmt.Sprintf("processAsyncResults - CommandValue (%s) transformed failed: %v", cv.String(), err))
+					continue


We might want to do the same as assertions here (i.e. set the reading result to "Transformation failed..."). Probably worth a new issue...

#94 opened for this topic

tonyespy · 2018-11-06T16:56:12Z

internal/cache/init.go

+		}
+		newDeviceCache(ds)
+
+		dps := make([]models.DeviceProfile, len(ds))


I still think this is wrong... please an open an issue to track.

tonyespy · 2018-11-06T17:05:53Z

internal/handler/command.go

+
+		err = transformer.CheckAssertion(cv, do.Properties.Value.Assertion, device)
+		if err != nil {
+			common.LoggingClient.Error(fmt.Sprintf("Handler - execReadCmd: Assertion failed for device resource: %s, with value: %s", cv.String(), err))


Note we need to treat assertion failures in the command endpoint in the same way we handle them in async.go. Please open an issue for this...

#92 opened for this issue

#93 opened for the init cache topic

tonyespy · 2018-11-06T17:07:56Z

pkg/models/commandrequest.go

@@ -4,7 +4,7 @@
 //
 // SPDX-License-Identifier: Apache-2.0

-package device
+package models

 import "github.com/edgexfoundry/edgex-go/pkg/models"


I know it was my suggestion that maybe we put the models under pkg/models vs. just models, however now I'm beginning to think this was incorrect, as all the other public code isn't included underneath pkg. This is simple enough to change back, but maybe worth discussion via email vs. an issue?

#91 opened for this topic

jduranf reviewed Oct 18, 2018

View reviewed changes

jduranf reviewed Oct 19, 2018

View reviewed changes

internal/handler/command.go Show resolved Hide resolved

tonyespy reviewed Oct 23, 2018

View reviewed changes

async.go Outdated Show resolved Hide resolved

tonyespy reviewed Oct 23, 2018

View reviewed changes

async.go Show resolved Hide resolved

tonyespy reviewed Oct 23, 2018

View reviewed changes

example/cmd/simple-device/main.go Outdated Show resolved Hide resolved

weichou1229 reviewed Oct 23, 2018

View reviewed changes

internal/cache/profiles.go Outdated Show resolved Hide resolved

tonyespy requested changes Oct 26, 2018

View reviewed changes

tonyespy requested changes Nov 4, 2018

View reviewed changes

tonyespy approved these changes Nov 6, 2018

View reviewed changes

tonyespy merged commit bd91015 into edgexfoundry:master Nov 6, 2018

This was referenced Nov 7, 2018

Implement floating point base64 encoding #24

Closed

Implement All form of GET command endpoint #25

Closed

Implement actuation command support #27

Closed

Implement internal scheduler mechanism #79

Closed

cloudxxx8 deleted the iotech_refactoring branch November 9, 2018 01:56

refactoring for Delhi release from IOTech #74

refactoring for Delhi release from IOTech #74

Conversation

cloudxxx8 commented Oct 16, 2018 • edited Loading

iain-anderson commented Oct 17, 2018

cloudxxx8 commented Oct 17, 2018

iain-anderson commented Oct 17, 2018

jduranf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudxxx8 commented Oct 18, 2018

cloudxxx8 commented Oct 20, 2018

tonyespy commented Oct 20, 2018

cloudxxx8 commented Oct 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonyespy left a comment

Choose a reason for hiding this comment

cloudxxx8 commented Oct 31, 2018 • edited Loading

tonyespy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudxxx8 commented Nov 6, 2018

tonyespy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudxxx8 Nov 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudxxx8 commented Oct 16, 2018 •

edited

Loading

cloudxxx8 commented Oct 31, 2018 •

edited

Loading

cloudxxx8 Nov 7, 2018 •

edited

Loading