Skip to content
Open
Changes from 9 commits
Commits
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
128 changes: 75 additions & 53 deletions Bullwinkle.class.nut
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
Copy Markdown
Contributor

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
Copy Markdown
Author

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
Copy Markdown
Contributor

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
Copy Markdown
Author

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
Copy Markdown
Contributor

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,12 @@ 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"] : 15000,
"onError" : ("onError" in settings) ? settings["onError"] : null
};

// Initialize out message handlers
Expand Down Expand Up @@ -95,10 +98,11 @@ class Bullwinkle {
// Parameters:
// name The message name
// data Optional data
// ts Optional timestamp for the data

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is unclear what the developer is expected to do with this new field nor what effect setting it will have

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I suppose having proper docs would help with that :)

Its just a timestamp associated with the data. If not passed in, Bullwinkle will use the time that .send() is called (which is the v2.3.1 behavior) but this change allows for any timestamp that a developer wants to use.

Imagine a battery powered device that records data 1/minute and writes it to nv/spiflash. Then 1/hour it connects to WiFi and send all of the data. Probably in this case you would just send one big data structure of value/ts but you could send 60 bullwinkle messages with the correct timestamp as part of the message.

The actual reason I went through the trouble of making these changes is for impPager. With that class, we write data to spiflash when bullwinkle.onFail happens. I then read 1 datapoint at a time and send it through bullwinkle (to avoid crashing out of memory if I send all the data at once). Here, I would like the ability to send the data with the original timestamp the data was created, not the timestamp that I call bullwinkle.send() for the nth time.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not put the timestamp in your data instead of in Bullwinkle's?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fully agree with Aron. This is what I'd suggest too.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That is fine (although not what I have implemented currently). If this is the path forward, I still don't think .ts needs to be sent across the wire or stored twice in RAM... Although that may be a v3 feature

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You might not but others might 👯‍♂️

//
// 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 +151,6 @@ class Bullwinkle {
"name": command,
"data": data,
"ts": ts,
"tries": 0,
};
}

Expand All @@ -162,11 +165,18 @@ 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
Copy Markdown

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
Copy Markdown
Author

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
Copy Markdown

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
Copy Markdown
Contributor

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.

@deldrid1 deldrid1 Oct 6, 2016

Copy link
Copy Markdown
Author

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
Copy Markdown
Contributor

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
Copy Markdown

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
Copy Markdown
Author

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
Copy Markdown

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
Copy Markdown
Author

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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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.

imp.wakeup(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))
}
// Send the message
_partner.send(BULLWINKLE, message);
}

// Sends a response (ACK, NACK, REPLY) to a message
Expand Down Expand Up @@ -365,17 +375,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 +395,7 @@ class Bullwinkle {

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

@ppetrosh ppetrosh Oct 5, 2016

Copy link
Copy Markdown

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
Copy Markdown
Author

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
Copy Markdown

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
Copy Markdown
Author

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
Copy Markdown

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
Copy Markdown
Contributor

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
Copy Markdown
Author

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
Copy Markdown
Contributor

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
Copy Markdown
Author

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
Copy Markdown
Contributor

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,6 +407,37 @@ class Bullwinkle {
}.bindenv(this);
};

// 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, calles onError callback if needed
//
// Parameters:
Expand All @@ -414,7 +455,7 @@ class Bullwinkle {
// message timeouts.
function _watchdog() {
// Get the current time
local t = time();
local t = Bullwinkle._isAgent() ? time() : hardware.micros()/1000000

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

// if it's a message awaiting a reply
local ts = "retry" in message ? message.retry.ts : message.ts;
local ts = "retry" in message ? message.retry.ts : split(package._ts, ".")[0].tointeger(); //Use either the retry ts or the package ts, 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)) {
// 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];
}
_packageFailed(package, BULLWINKLE_ERR_NO_RESPONSE)
}
}

Expand All @@ -483,6 +502,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 +513,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 @@ -565,5 +588,4 @@ class Bullwinkle.Package {
return format("%d.%06d", d/1000000, d%1000000);
}
}

}