Closed jasoncodes closed 7 years ago
Merging #59 into master will increase coverage by
0.14%
. The diff coverage is100%
.
@@ Coverage Diff @@
## master #59 +/- ##
==========================================
+ Coverage 60.29% 60.43% +0.14%
==========================================
Files 45 45
Lines 1413 1418 +5
Branches 224 226 +2
==========================================
+ Hits 852 857 +5
Misses 561 561
Impacted Files | Coverage Δ | |
---|---|---|
lib/lifx/client.js | 88.84% <100%> (+0.2%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 29a1263...ab5b486. Read the comment docs.
I originally implemented this as a new bindPort
option but then decided a new sendPort
option (which would rarely be changed) would be nicer and more backwards compatible. Force pushed.
Thanks for the PR!
I have played around with dgram socket parameter reuseAddr
to tackle the same issue. I've been able to run multiple intances after modifying the client.js
line from:
this.socket = dgram.createSocket('udp4');
to:
this.socket = dgram.createSocket({type: 'udp4', reuseAddr: true});
However I felt it being more of a hack. Your approach seems like the correct way to do this. I've now tested the PR also and it worked great!
I've previously tried setting a different port using the { port: <port> }
option for the client but that never worked out. If I understand your change correctly, the problem why it didn't work previously was that after changing the port
option to lets say 12345, the client would also try sending the outbound packets to port 12345 which isn't the one listened by the lights (56700/udp)?
@MariusRumpf could you have time to check this? I vote this change should definately be merged.
I too tried setting reuseAddr
but found that UDP responses were not being routed back to the correct process when I was testing on my macOS development machine. I didn’t investigate any further (as I have had similar issues in the past when using reuseAddr
on unrelated things).
If I understand your change correctly, the problem why it didn't work previously was that after changing the port option to lets say 12345, the client would also try sending the outbound packets to port 12345 which isn't the one listened by the lights (56700/udp)?
That’s correct. Without decoupling the bind port from the send port, outbound packets were going to the wrong place.
An aside: From memory I believe binding to 56700 may have been required/recommended with original firmware versions anything with cloud support does not have this restriction.
@MariusRumpf ok to merge then?
@ristomatti yes, feel free to do so.
This PR splits the bind port configuration option from target port address option and changes the default bind port to be random. This permits multiple instances of
node-lifx
to be used without having to bind to seperate interfaces.