Skip to content

Better partition Bullwinkle.Package and bullwinkle messages sent "over the wire" #20

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
deldrid1 wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Better partition Bullwinkle.Package and bullwinkle messages sent "over the wire" #20

Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
fb1c863
Bump Version
deldrid1 Sep 27, 2016
d07ea66
Add prependID to settings
deldrid1 Sep 27, 2016
e58d826
Allow custom timestamps to be used with .send(name, data, ts)
deldrid1 Sep 27, 2016
348fd2c
Move message.tries into package._tries
deldrid1 Sep 27, 2016
dabe747
Move _packageTimeout into its own workflow and call immediately if th…
deldrid1 Sep 27, 2016
4784503
Revert "Add prependID to settings"
deldrid1 Sep 27, 2016
e2149af
Bug fixes identified during testing
deldrid1 Oct 3, 2016
e5d00f7
Bug fix for usage on Agent
deldrid1 Oct 4, 2016
adb215a
Add lowMemoryThreshold setting to address #18.
deldrid1 Oct 4, 2016
82ce3b4
Bug fix for messages not calling their onFail handlers after a timeout
deldrid1 Oct 5, 2016
b9b0155
Add _checkTimer for all imp.wakeup's
deldrid1 Oct 5, 2016
5e91b88
Add firstMessageID setting to Bullwinkle.
deldrid1 Oct 5, 2016
b1514ce
bugfix for ts
deldrid1 Nov 26, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
158 changes: 95 additions & 63 deletions Bullwinkle.class.nut
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,21 @@ enum BULLWINKLE_MESSAGE_TYPE {
REPLY,
ACK,
NACK,
TIMEOUT,
FAILED,
DONE
}

// Error messages
const BULLWINKLE_ERR_NO_HANDLER = "No handler for Bullwinkle message";
const BULLWINKLE_ERR_NO_RESPONSE = "No Response from partner";
const BULLWINKLE_ERR_LOW_MEMORY = "imp running below low memory threshold";
const BULLWINKLE_ERR_NO_CONNECTION = "server.isconnected() == false"
const BULLWINKLE_ERR_TOO_MANY_TIMERS = "Too many timers";
const BULLWINKLE_WATCHDOG_TIMER = 0.5;


class Bullwinkle {
static version = [2,3,1];
static version = [2,3,2];
Copy link
Contributor

@blindman2k blindman2k Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be:
static version = [2,4,0];

Copy link
Author

@deldrid1 deldrid1 Oct 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on where the pull request ends up, it might ought to be [3,0,0]...

Copy link
Contributor

@blindman2k blindman2k Oct 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we may reserve v3.0.0 for a complete rewrite :)

Copy link
Author

@deldrid1 deldrid1 Oct 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just (more than gently) trying to nudge you in that direction ;)

Copy link
Contributor

@blindman2k blindman2k Oct 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not the one you need to convince 😸


// The bullwinkle message
static BULLWINKLE = "bullwinkle";
Expand All @@ -46,11 +48,13 @@ class Bullwinkle {
constructor(settings = {}) {
// Initialize settings
_settings = {
"messageTimeout": ("messageTimeout" in settings) ? settings["messageTimeout"].tostring().tointeger() : 10,
"retryTimeout": ("retryTimeout" in settings) ? settings["retryTimeout"].tostring().tointeger() : 60,
"maxRetries": ("maxRetries" in settings) ? settings["maxRetries"].tostring().tointeger() : 0,
"autoRetry" : ("autoRetry" in settings) ? settings["autoRetry"] : false,
"onError" : ("onError" in settings) ? settings["onError"] : null
"messageTimeout": ("messageTimeout" in settings) ? settings["messageTimeout"].tostring().tointeger() : 10,
"retryTimeout": ("retryTimeout" in settings) ? settings["retryTimeout"].tostring().tointeger() : 60,
"maxRetries": ("maxRetries" in settings) ? settings["maxRetries"].tostring().tointeger() : 0,
"autoRetry" : ("autoRetry" in settings) ? settings["autoRetry"] : false,
"lowMemoryThreshold": ("lowMemoryThreshold" in settings) ? settings["lowMemoryThreshold"].tointeger() : 15000,
"firstMessageID": ("firstMessageID" in settings) ? settings["firstMessageID"].tointeger() : 0
Copy link

@ppetrosh ppetrosh Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please elaborate, what is this for? Thanks!

Copy link
Author

@deldrid1 deldrid1 Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

firstMessageID is designed to allow semi globally unique message IDs and limit the chance of collisions (in case you are keeping up with the IDs on the Agent, etc. to know if you have missed a message).

Bullwinkle has a pool of 2,147,483,647 IDs that it can use. Based on the RAM/SPIFlash limitations of Agent/Device, you are never going to have that many valid IDs available to compare against (although over a course of weeks/months you may cycle through them). This just allows you to have bullwinkle's ID generation start wherever you want it, instead of at 0. (For impPager testing, seeing message ID 0 after multiple reboots in a row wasn't terribly helpful for debugging)

See line 67 - this ought to be refactored to just be used directly there (there is no reason to keep it in this settings object as it is only used in the constructor), however, it does make the code a bit clearer for someone who is reviewing the source code instead of the readme.

Copy link

@ppetrosh ppetrosh Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, got it. Thanks for the explanation. Let's please describe all these new configuration settings in the doc, when we all agree on the changes.

"onError" : ("onError" in settings) ? settings["onError"] : null
};

// Initialize out message handlers
Expand All @@ -59,8 +63,8 @@ class Bullwinkle {
// Initialize list of packages
_packages = {};

// Initialize the ID counter
_nextId = 0;
// Initialize the ID counter (can be set to math.rand() or the last message ID you have in nv to prevent ID collisions with something like impPager)
_nextId = settings.firstMessageID;

// Setup the agent/device.on handler
_partner = _isAgent() ? device : agent;
Expand Down Expand Up @@ -95,10 +99,11 @@ class Bullwinkle {
// Parameters:
// name The message name
// data Optional data
// ts Optional timestamp for the data
//
// Returns: Rocky.Package object
function send(name, data = null) {
local message = _messageFactory(BULLWINKLE_MESSAGE_TYPE.SEND, name, data);
// Returns: Bullwinkle.Package object
function send(name, data = null, ts = null) {
local message = _messageFactory(BULLWINKLE_MESSAGE_TYPE.SEND, name, data, ts);
local package = Bullwinkle.Package(message);
_packages[message.id] <- package;
_sendMessage(message);
Expand Down Expand Up @@ -147,7 +152,6 @@ class Bullwinkle {
"name": command,
"data": data,
"ts": ts,
"tries": 0,
};
}

Expand All @@ -162,11 +166,20 @@ class Bullwinkle {
if (_watchdogTimer == null) _watchdog();

// Increment the tries
if (message.type == BULLWINKLE_MESSAGE_TYPE.SEND) {
message.tries++;
if (message.type == BULLWINKLE_MESSAGE_TYPE.SEND && message.id in _packages) {
_packages[message.id]._tries++;
Copy link

@ppetrosh ppetrosh Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor cosmetic: let's please try to keep the size of indents the same across the code, like 4 spaces.

Copy link
Author

@deldrid1 deldrid1 Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry - copy/paste from the IDE to Atom does some weird things... Getting ElectricImp-Sublime working is a must! :)

Copy link

@ppetrosh ppetrosh Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Thanks! I'm working on it...

Copy link
Contributor

@blindman2k blindman2k Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change from tries to _tries breaks my existing code.

Copy link
Author

@deldrid1 deldrid1 Oct 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth making the breaking change to reduce overhead on the wire? (I would think from imp's perspective for "free" developer devices and your paying customer's perspective for large device deployments, that would be a win...)

@blindman2k - could the onFail callback function be updated to something like

.onFail(function(err, message, retry, numTries) {
        // Try sending the message again in 60 seconds, assuming we've not made more than 3 tries
  if(numTries <3){
        if (!retry(60)) {
            server.error("No more retry attempts are allowed");
        }
  }
})

I'm also curious as to why you look at message.tries directly instead of just passing the maxRetries setting and let the class take care of it for you? I've not used the retry functionality too much myself (TCP typically takes care of it for me, or I just assume the message will fail and I need to deal with it some other way) so I may be missing something...

Copy link
Contributor

@blindman2k blindman2k Oct 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a/ The maxRetries parameter came after the tries field
b/ If I remember correctly the maxRetries doesn't inform the caller when it stops retrying. Not 100% on this.

Breaking developers code is not something I would recommend at this stage.

}

if(imp.getmemoryfree() > _settings.lowMemoryThreshold && (_isAgent() || server.isconnected())){
Copy link

@ppetrosh ppetrosh Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we discussed this awhile ago and agreed to keep track of available memory on the app side. But I might be wrong. Not sure if this is generic enough to be a part of the library but don't want to push back hard here.

Copy link
Author

@deldrid1 deldrid1 Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was left rather open ended in Issue #18. This is essentially a 2 line implementation that adds very little library bloat, so I went ahead and snuck it in.

Copy link

@ppetrosh ppetrosh Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we end up adding this feature into the library, my preference would be to at least keep the default behavior unchanged, i.e. have this flag equals to 0.

Copy link
Author

@deldrid1 deldrid1 Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgive my bias towards wanting the feature :)

That seems reasonable to me to preserve existing functionality.

Copy link
Contributor

@blindman2k blindman2k Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sneaky, very sneaky.
I suggest you set the default value at 0 instead of 15000 to ensure that functionality is the same.

_partner.send(BULLWINKLE, message); // Send the message
} else if(message.id in _packages){ //run the failure flow (if the package exists)
local reason = imp.getmemoryfree() <= _settings.lowMemoryThreshold ? BULLWINKLE_ERR_LOW_MEMORY : BULLWINKLE_ERR_NO_CONNECTION
Copy link
Contributor

@blindman2k blindman2k Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This technique is a bit lazy. It's not a big issue now but it may become one in the future.


local timer = imp.wakeup(0.0, function(){ // run on the "next tick" so that the onFail handler can have a chance to register itself
_packageFailed(_packages[message.id], reason)
}.bindenv(this));
_checkTimer(timer)
}
// Send the message
_partner.send(BULLWINKLE, message);
}

// Sends a response (ACK, NACK, REPLY) to a message
Expand Down Expand Up @@ -330,7 +343,7 @@ class Bullwinkle {
handler(BULLWINKLE_ERR_NO_HANDLER, message, retry);

// Delete the message if the dev didn't retry
if (message.type == BULLWINKLE_MESSAGE_TYPE.NACK) {
if (message.type == BULLWINKLE_MESSAGE_TYPE.NACK && message.id in __bull.packages) {
delete __bull._packages[message.id];
}
});
Expand Down Expand Up @@ -365,17 +378,17 @@ class Bullwinkle {
// Check the message is still valid
if (!(message.id in _packages)) {
// server.error(format("Bullwinkle message id=%d has expired", message.id))
message.type = BULLWINKLE_MESSAGE_TYPE.DONE;
return false;
}

// Check there are more retries available
if (_settings.maxRetries > 0 && message.tries >= _settings.maxRetries) {
// server.error(format("Bullwinkle message id=%d has no more retries", message.id))
message.type = BULLWINKLE_MESSAGE_TYPE.DONE;
delete _packages[message.id];
return false;
}
message.type = BULLWINKLE_MESSAGE_TYPE.DONE;
return false;
}

// Check there are more retries available
if (_settings.maxRetries > 0 && _packages[message.id]._tries >= _settings.maxRetries) {
// server.error(format("Bullwinkle message id=%d has no more retries", message.id))
message.type = BULLWINKLE_MESSAGE_TYPE.DONE;
delete _packages[message.id];
return false;
}

// Set timeout if required
if (timeout == null) { timeout = _settings.retryTimeout; }
Expand All @@ -385,7 +398,7 @@ class Bullwinkle {

// Add the retry information
message.retry <- {
"ts": time() + timeout,
"ts": ( Bullwinkle._isAgent() ? time() : hardware.micros()/1000000 ) + timeout,
Copy link

@ppetrosh ppetrosh Oct 5, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general this looks a little bit confusing to me. We have Package.ts and Message.ts. It's almost impossible to remember which of them is used for what. If one needs to have a custom timestamp for the data, from my POV it should go to the data payload and be handled on the app level as well as all the custom information.

@betzrhodes, @blindman2k what do you think?

Copy link
Author

@deldrid1 deldrid1 Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I disagree? We have a Package._ts (used for Bullwinkle internal logic) and we have a message.ts that corresponds to the timestamp that is associated with the message.data.

I don't disagree that the ts could go in the data payload, however, message.ts is a key that much of my application code relies on (and I would assume others rely on it as well) so I don't think that removing its use in the lib should be taken lightly.

Copy link

@ppetrosh ppetrosh Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is that we should probably try to keep all the "custom" and application specific stuff in the message payload.

And regarding this particular change I'd recommend to keep the _ts for internal/generic purposes and build all the application specific logics on the custom message payload fields.

Copy link
Author

@deldrid1 deldrid1 Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not opposed to moving the retry data structure over to the Package - just work that I haven't done yet and wasn't necessary to accomplish what I am doing with impPager.

message.retry and message.tries are items that I don't expect anyone would be using, but message.ts is something that I would think lots of folks are using.

Either way it is probably a major bump to the lib since there could be user breaking features (if people are using those message key/values) so if you want to remove the ts from the message and make it be a part of the payload that is fine with me. I'll just wrap .send() and ensure that a ts is always added to the data :)

Copy link

@ppetrosh ppetrosh Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No-no, I don't want to remove .ts from the message. Neither I want to change it's semantics (what essentially the pull request does). My proposal is that if someone needs a custom timestamp, then he/she should be using the message payload to add it. Does it make sense?

Copy link
Contributor

@blindman2k blindman2k Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused by this change ... time() is seconds from epoch (or from boot if the device hasn't been online since power-up) and hardware.micros() microseconds from boot but wraps around every 36 minutes. How is this change not going to make a massive mess?

Copy link
Author

@deldrid1 deldrid1 Oct 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like to me message.ts should be the eventTime in epoch seconds. This provides the timestamp for the data and should be what is transmitted to the _partner for it to do whatever useful thing might be done with a timestamp that corresponds to the data.

For this pull request, package._ts is used only for timeouts and calculating latencies. time() by itself doesn't give very useful info as it is only precise down to the second, hence the need to go to hardware.millis() or hardware.micros(). time() also doesn't tick when there is no RTC present. Since 36 minutes seems like significant overkill for a timeout or for a network latency, I went with hardware.micros().

Copy link
Contributor

@blindman2k blindman2k Oct 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that time() is a pain, especially for code that executes before going online. You are probably right that 36 minutes is enough time for most but not for all. At the very least we would need to warn (or prevent) the developer not to set a messageTimeout greater than 2160.

Also, watchdog is no longer valid as it uses time() instead of micros().

Copy link
Author

@deldrid1 deldrid1 Oct 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use hardware.millis() to would provide a much larger window (25 days), however, I chose hardware.micros() to provide some amount of consistency between the agent and device...

In my very ugly code, watchdog is still valid. If time() is invalid and not ticking, the message fails before it ever gets to the watchdog. If time() is ticking, the watchdog still works correctly by doing all of the nasty split() calls.

Copy link
Contributor

@blindman2k blindman2k Oct 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to use millis() and you could always pad it with 0's. But it's not something you can compare to the agent so it hardly makes a difference.

Argh, this is a mess.

"sent": false
};

Expand All @@ -397,7 +410,38 @@ class Bullwinkle {
}.bindenv(this);
};

// checks that TIMER was set, calles onError callback if needed
// Call the onFail handler when a timeout occurs
//
// Parameters:
// package The Bullwinkle.Package that has timed out
//
function _packageFailed(package, reason) {
// Grab the onFail handler
local handler = package.getHandler("onFail");
local message = package._message

// If the handler doesn't exists
if (handler == null) {
// Delete the package, and move to next package
delete _packages[message.id];
}

// Grab a reference to this
local __bull = this;

// Build the retry method for onFail
local retry = _retryFactory(message);

// Invoke the handlers
message.type = BULLWINKLE_MESSAGE_TYPE.FAILED
handler(reason, message, retry);
// Delete the message if there wasn't a retry attempt
if (message.type == BULLWINKLE_MESSAGE_TYPE.FAILED) {
delete __bull._packages[message.id];
}
}

// checks that TIMER was set, calls onError callback if needed
//
// Parameters:
// timer The value returned by calling imp.wakeup
Expand All @@ -414,7 +458,7 @@ class Bullwinkle {
// message timeouts.
function _watchdog() {
// Get the current time
local t = time();
local t = time()

// Loop through all the cached packages
foreach(idx, package in _packages) {
Expand All @@ -434,31 +478,12 @@ class Bullwinkle {
}

// if it's a message awaiting a reply
local ts = "retry" in message ? message.retry.ts : message.ts;
if (t >= (ts + _settings.messageTimeout)) {
// Grab the onFail handler
local handler = package.getHandler("onFail");

// If the handler doesn't exists
if (handler == null) {
// Delete the package, and move to next package
delete _packages[message.id];
continue;
}

// Grab a reference to this
local __bull = this;

// Build the retry method for onFail
local retry = _retryFactory(message);

// Invoke the handlers
message.type = BULLWINKLE_MESSAGE_TYPE.TIMEOUT
handler(BULLWINKLE_ERR_NO_RESPONSE, message, retry);
// Delete the message if there wasn't a retry attempt
if (message.type == BULLWINKLE_MESSAGE_TYPE.TIMEOUT) {
delete __bull._packages[message.id];
}
local ts = "retry" in message ? message.retry.ts : split(package._ts, ".")[0].tointeger(); //Use either the retry ts or the package ts time(), but NOT the message ts so that it can be set for whenever the data was generated, instead of when Bullwinkle attempted to send it
if (t >= (ts + _settings.messageTimeout) || t == 946684800) { //RTC is invalid, which implies we have no connection and should retry immediately.
Copy link

@ppetrosh ppetrosh Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

946684800 should probably be a constant.

Copy link
Author

@deldrid1 deldrid1 Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const RTC_INVALID_TIME = 946684800; //Saturday 1st January 2000 12:00:00 AM UTC - this is what time() returns if the RTC signal from the imp cloud has not been received this boot.

seem reasonable?

Copy link

@ppetrosh ppetrosh Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks!

Copy link
Contributor

@blindman2k blindman2k Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Namespace ...
const BULLWINKLE_ RTC_INVALID_TIME = 946684800; //Saturday 1st January 2000 12:00:00 AM UTC - this is what time() returns if the RTC signal from the imp cloud has not been received this boot.

local timer = imp.wakeup(0.0, function(){
_packageFailed(package, BULLWINKLE_ERR_NO_RESPONSE)
}.bindenv(this));
_checkTimer(timer)
}
}

Expand All @@ -483,6 +508,9 @@ class Bullwinkle.Package {
// The timestamp of the original message
_ts = null;

// the number of attempts we have made to send the message
_tries = null;

// Class constructor
//
// Parameters:
Expand All @@ -491,6 +519,7 @@ class Bullwinkle.Package {
_message = message;
_handlers = {};
_ts = _timestamp();
_tries = 0;
}

// Sets an onSuccess callback that will be invoked if the message is successfully
Expand Down Expand Up @@ -545,10 +574,15 @@ class Bullwinkle.Package {
//
// Returns: The time difference in seconds (float) between the packages timestamp and now()
function getLatency() {
local t0 = split(_ts, ".");
local t1 = split(_timestamp(), ".");
local diff = (t1[0].tointeger() - t0[0].tointeger()) + (t1[1].tointeger() - t0[1].tointeger()) / 1000000.0;
local t0 = split(_ts, ".");
local t1 = split(_timestamp(), ".");

if (Bullwinkle._isAgent()) {
local diff = (t1[0].tointeger() - t0[0].tointeger()) + ( (t1[1].tointeger() - t0[1].tointeger()) / 1000000.0);
return math.fabs(diff);
} else {
return (t1[1].tointeger() - t0[1].tointeger()) / 1000000.0;
}
}

// Returns the time in a string format that can be used for calculating latency
Expand All @@ -561,9 +595,7 @@ class Bullwinkle.Package {
local d = date();
return format("%d.%06d", d.time, d.usec);
} else {
local d = math.abs(hardware.micros());
return format("%d.%06d", d/1000000, d%1000000);
return format("%d.%06d", time(), hardware.micros()); //this can be a bit of an ugly _ts but it allows us to calculate latencies up to 36 minutes long...
Copy link
Contributor

@blindman2k blindman2k Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, it is seriously ugly. You have littered the code with split() calls by doing this. I suggest having _ts as an array so you don't have to keep splitting it or make a function to split it like you have a function to create it.

The problem is it is NOT a valid use of hardware.micros(). On the agent it is valid, on the device it is not. hardware.micros() is not a sub-value of time().

Copy link
Author

@deldrid1 deldrid1 Oct 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True - it is not a subvalue of time and array makes more since. However, I was trying to keep consistency between the agent and device code implementations as much as possible (package._ts is always a string as opposed to a string on the agent and an array on the device).

Timeouts work off of the integer time() in seconds, and getLatency knows what is going on, but the implementation is confusing and ugly.

Since ._ts is a "private" _property, people shouldn't be depending on it and we can likely move to an array?

Copy link
Contributor

@blindman2k blindman2k Oct 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, getLatency only uses the seconds on the device. I missed that. Now, it's more accurate but even more ugly!

}
}

}