core-communication-lib icon indicating copy to clipboard operation
core-communication-lib copied to clipboard

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

Open technobly opened this issue 10 years ago • 10 comments

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.

technobly avatar Apr 11 '15 05:04 technobly

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!

m-mcgowan avatar Apr 14 '15 14:04 m-mcgowan

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

zsup avatar Apr 14 '15 14:04 zsup

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?

dmiddlecamp avatar Apr 14 '15 15:04 dmiddlecamp

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 https://github.com/spark/core-communication-lib/issues/30#issuecomment-92894212 .

Andy

andyw-lala avatar Apr 14 '15 15:04 andyw-lala

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 https://github.com/spark/core-communication-lib/issues/30#issuecomment-92894212 .

Andy

Andy

andyw-lala avatar Apr 14 '15 15:04 andyw-lala

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

m-mcgowan avatar Apr 14 '15 15:04 m-mcgowan

+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 https://github.com/spark/core-communication-lib/issues/30#issuecomment-92914960 .

Andy

andyw-lala avatar Apr 14 '15 15:04 andyw-lala

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 https://github.com/spark/core-communication-lib/issues/30#issuecomment-92914960 .

Andy

Andy

andyw-lala avatar Apr 14 '15 15:04 andyw-lala

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

m-mcgowan avatar Apr 14 '15 15:04 m-mcgowan

+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 https://github.com/spark/core-communication-lib/issues/30#issuecomment-92927134 .

Andy

andyw-lala avatar Apr 14 '15 15:04 andyw-lala