Skip to content
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.

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

Spark.function() args > 63 chars knocks Core offline briefly #30

Open
technobly opened this issue Apr 11, 2015 · 10 comments
Open

Spark.function() args > 63 chars knocks Core offline briefly #30

technobly opened this issue Apr 11, 2015 · 10 comments

Comments

@technobly
Copy link

Tried with 0.3.4 and the latest and greatest HAL firmware and the Core is doing the same thing, dropping off the cloud for a good 1 second count if I throw more than 63 chars at the API for Spark.function()

@dmiddlecamp has confirmed no hard limit for this one in the API, so it sounds like the limit must be causing this response at the firmware level.

It seems like a hard coded limit in the API would be good:

Pros:

  • saves BW
  • can return a lovely error message "stahp sending me more than 63 chars!"

Cons:

  • if different devices have different limits, more CPU usage on server
  • ?

I also think the firmware should have a hard coded limit that keeps it operating sanely.

@m-mcgowan
Copy link

The cause of the drop-off the cloud is this check here:

https://github.com/spark/core-communication-lib/blob/master/src/spark_protocol.cpp#L887-L890

If the function argument passed is longer than the limit - currently 63 characters then return false.

The function returns false on error (typically read/write error), which causes the caller to drop the device off the cloud. (Sets SPARK_CLOUD_SOCKETED=0, so the next iteration of the background loop, the cloud is reconnected.)

So the question is, rather than dropping the cloud connection, is there a better response?

  • return true, and not send a response. This will cause the cloud to eventually timeout.
  • return true, and send a result of -1.
  • truncate the parameter?
  • keep as is - it's a caller problem, they can sanitize their data!

@zsup
Copy link

zsup commented Apr 14, 2015

I think any response is better than dropping the cloud connection. Although between your first three options I don't have a strong preference

@dmiddlecamp
Copy link
Contributor

I think ideally the device should return 'FunctionReturnError' to the cloud code: Message.Code.BAD_REQUEST, type: Message.Type.NON, and not drop the connection?

@andyw-lala
Copy link

Since the only UI on the core is a blinky LED, and that might be in a box
hundreds of miles away, how about we generate a generic exception reporting
mechanism, that the core can use to say "I was asked to do X, which is not
good" - this would take some time to actually design well, but combined
with the dashboard, things could get very useful very quickly.

Bonus points for immediately including a cloud-reported ASSERT() and/or
debug_printf() mechanism.

On Tue, Apr 14, 2015 at 10:05 AM, David Middlecamp <[email protected]

wrote:

I think ideally the device should return 'FunctionReturnError' to the
cloud code: Message.Code.BAD_REQUEST, type: Message.Type.NON, and not
drop the connection?


Reply to this email directly or view it on GitHub
#30 (comment)
.

Andy

@andyw-lala
Copy link

Oh - and return some safe, non-pathological return value, and do not kick
the core offline.

On Tue, Apr 14, 2015 at 10:10 AM, Andy Warner [email protected] wrote:

Since the only UI on the core is a blinky LED, and that might be in a box
hundreds of miles away, how about we generate a generic exception reporting
mechanism, that the core can use to say "I was asked to do X, which is not
good" - this would take some time to actually design well, but combined
with the dashboard, things could get very useful very quickly.

Bonus points for immediately including a cloud-reported ASSERT() and/or
debug_printf() mechanism.

On Tue, Apr 14, 2015 at 10:05 AM, David Middlecamp <
[email protected]> wrote:

I think ideally the device should return 'FunctionReturnError' to the
cloud code: Message.Code.BAD_REQUEST, type: Message.Type.NON, and not
drop the connection?


Reply to this email directly or view it on GitHub
#30 (comment)
.

Andy

Andy

@m-mcgowan
Copy link

The Spark.log() proposal addresses communicating errors to the cloud, so that would be put to good use here.

@andyw-lala
Copy link

+1

On Tue, Apr 14, 2015 at 10:37 AM, Matthew McGowan [email protected]
wrote:

The Spark.log() proposal addresses communicating errors to the cloud, so
that would be put to good use here.


Reply to this email directly or view it on GitHub
#30 (comment)
.

Andy

@andyw-lala
Copy link

Although we might want to partition categories of logs (think syslog, but
-lite), so that API violations, user asserts, system asserts, etc were not
all dumped together with the output from user log calls, and could be
flagged as distinct in the dashboard & audit/log trails associated with the
core/user.

Note that this also flips into fleet management - anything that suggests
the app might be misbehaving should be able to be redirected to the fleet
owner/admin, not the user.

On Tue, Apr 14, 2015 at 10:47 AM, Andy Warner [email protected] wrote:

+1

On Tue, Apr 14, 2015 at 10:37 AM, Matthew McGowan <
[email protected]> wrote:

The Spark.log() proposal addresses communicating errors to the cloud, so
that would be put to good use here.


Reply to this email directly or view it on GitHub
#30 (comment)
.

Andy

Andy

@m-mcgowan
Copy link

Yep to all of that. afaik that's the direction we're heading. api errors will be distinct from other logs.

@andyw-lala
Copy link

+10^6

On Tue, Apr 14, 2015 at 10:55 AM, Matthew McGowan [email protected]
wrote:

Yep to all of that. afaik that's the direction we're heading. api errors
will be distinct from other logs.


Reply to this email directly or view it on GitHub
#30 (comment)
.

Andy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants