TheThingsArchive / node-app-sdk

The Things Network Application SDK for Node.JS
https://www.thethingsnetwork.org/docs/node-js/
MIT License
41 stars 28 forks source link

Accept uplink message as first argument for downlink method #2

Closed FokkeZB closed 8 years ago

FokkeZB commented 8 years ago

This way the user could do:

client.on('uplink', function(msg) {
  client.downlink(msg, new Buffer('4869', 'hex'));
});

If downlink() sees the first argument is an object it would then use msg.dev_eui (and msg.port?) to do the downlink.

See https://github.com/TheThingsNetwork/node-app-lib/issues/2#issuecomment-247863572 for reasoning against:

An alternative implementation is for msg to have a downlink method that binds to the client with the dev_eui (and port?) already set:

client.on('uplink', function(msg) {
  msg.downlink(new Buffer('4869', 'hex'));
});
FokkeZB commented 8 years ago

Some additional thoughts:

  1. If we add support for this it should be under a new method name (reply(msg, buffer)) so that the send/downlink() method stays simple.
  2. We should first build SDKs for more platforms with the basic API before we add convenience methods like this.
FokkeZB commented 8 years ago

Closing. This would make it look too much like a response, which it is not.

johanstokking commented 8 years ago

We can do a response in fact. I guess there are many use cases in which you want to respond within the receive window.

FokkeZB commented 8 years ago

For it to be a response is merely a matter of timing right? Implementing this API wouldn't guarantee it to be delivered as response

johanstokking commented 8 years ago

True. But it would be nice to make it easily accessible in the SDKs.

Also, we're extending RX1 from 1 second to 5 seconds to be able to make the roundtrip possible.

FokkeZB commented 8 years ago

OK, but then I'd suggest to support client.send(msg, payload) to make it less prone to be confused for a guaranteed response then msg.respond(payload).

FokkeZB commented 8 years ago

Closing as dev_id is no longer in payload.

See: https://github.com/TheThingsNetwork/node-app-lib/pull/25#discussion_r80195257