Adding node to read Bean scratch characteristics.

[garnold]

[mplewis]

@raykamp is the one in charge of this repo, but he's out of the office until next week. We'll be back to you soon! Thanks for the pull request.

[garnold]

@mplewis No problem, thanks for the update!

[raykamp]

@garnold, thank you for submitting this! I have a couple bits of feedback:

1. Node-RED is much easier to use when nodes are passing around messages that are "primitive values" (or buffers) rather than an array or object. I'd suggest a settings menu something like this:
   Each visual node would represent a single scratch bank. The output "msg" of the node would be a string, number, buffer, etc.
2. We have the ability to write scratch characteristics from node-red (ble-bean has this feature). Your PR is a good implementation of the read (1 input for triggering a read, 1 output for receiving the reading).
   We can make a second "write" node that has just a single input and can be configured to write a string, number, or buffer to a single scratch characteristic.
   These two nodes can assume names like beanScratchRead and beanScratchWrite.
3. Consider using the "Async" library to clean up nested asynchronous callbacks. Line 60 -> 94 of "Bean/beanScratch.js"
   https://github.com/caolan/async
4. We shouldn't be forcing a connection in the feature specific bean nodes. The "bean.js" config node manages the single peripheral/device resource (connection/disconnection/timeout), contains methods to access it's features, and handles queuing of functions when the bean isn't connected. Check out how "requestTemp" is implemented as an example.

Please share your thoughts on these.

-Ray

[garnold]

@raykamp Thanks for the feedback!

1. Let me noodle on this one for a bit. At it's simplest, the node as written can be configured to output a single scratch characteristic to msg.payload, but allows for multiple properties to be set on the same message corresponding to the desired/selected scratch characteristics. For me this use case is important, and provides for an atomic snapshot of the current scratch values. The alternative would be to require values from multiple messages to be collected before continuing the flow, ie. in the context of a function node.
2. "Write" node coming shortly, but here's a start: ae2a7c5
3. I'm new to Node.js and didn't know about "async", thanks! 2b1d6e0
4. Yeah, I tried to "cheat" and stay away from adding wrapper methods to bean.js for each scratch characteristic. I'll make that change.

[garnold]

@raykamp Re: 4, 9295ddd.

That said, I'm not sure the value of queuing requests when we can't find the desired Bean. Imagine the case where a Bean goes offline for a considerable amount of time (ie. a few hours or a day): what's the value of replaying all of those requests?

Instead I would suggest that the various Bean nodes drop the message (ie. interrupt the flow) when the desired Bean can't be found.

[garnold]

@raykamp Re: 2, 606f4c0.

Eager to hear you thoughts on 1 and 4.

[raykamp]

@garnold, regarding the queuing: I think it's very useful for a case other that you've described. It's useful when you use the device in infrequent spurts and you want a request to initiate a connection. I'm aware that the feature needs to be more fleshed out. I'll be adding 2 parameters to the Bean configuration. First is to set a time limit for how long a queued request will remain, and the second is a maximum number of queued requests. For your described case, both or one of these could just be set to 0. This would drop all messages that are send to the node when the Bean isn't connected.

[raykamp]

@garnold, thanks for the explanation on #1. I agree completely, the "atomic snapshot" is useful. The implementation looks great. At first glance, It didn't register with me that you could assign a single scratch bank to msg.payload if your application required.

[raykamp]

OK, tomorrow I'm going to test the code and get this pulled in. Great work @garnold

[garnold]

Thanks @raykamp ! And I like you where you're going with the queuing mechanism.

Right now the read node defaults to the all-in-one option, but I'm wondering if instead it should default to the simple case you described, ie. read scratch characteristic 1 and set as msg.payload. Frankly, the reason I have it reading all scratch characteristics at once and setting them as msg.scratch[1-5] is to avoid making the UI inconsistent/complex.

Also, I rebased out the npm version commits and deleted the corresponding tags. In retrospect it feels like that should be the responsibility of the repo owner / NPM publisher.

[raykamp]

Nice work on this @garnold. It's working very well!

One question on the implementation. I was a little confused when seeing a payload property on the output message from the "Read Scratch" node. It looks like you are modifying the incoming message to that node and sending it out. Can you think of any reasons why we shouldn't create an entirely new message for the output? It could clear up some confusion, but if the existing scheme is more useful, I'm all happy to keep it.

I'm not sure if you knew about this, but you can assign default node properties! So we can have the scratch type default to string in the properties menu. And perhaps for the write node, we can have "to" field default to bank 1.

[garnold]

Thanks @raykamp !

Now that is a good question. I always thought of flows as "building up" a message, however I can definitely see it the other way. I've posed the question to the Node-RED list.

I wasn't happy with how much you have to duplicate between the client-side and server-side in Node-RED, so I chose to do the defaulting just on the server-side and documented the behavior here and here:

https://github.com/PunchThrough/node-red-contrib-bean/pull/1/files#diff-c18245b2c086f5c44495f0ec9f137aaeR35 https://github.com/PunchThrough/node-red-contrib-bean/pull/1/files#diff-c18245b2c086f5c44495f0ec9f137aaeR132

... but perhaps it's worth rethinking that decision to make it more obvious on the client-side. Let me know @raykamp

[raykamp]

Hi Garnold, thanks for posing that to the Node-RED mailing list and opening https://github.com/PunchThrough/node-red-contrib-bean/pull/2.

One thing I just noticed related to this PR: Is there a reason for the scratch write node to have an output?

[garnold]

@raykamp Great catch! Fixed in https://github.com/PunchThrough/node-red-contrib-bean/pull/3