add support for connect over raw tcp socket #425
Conversation
There was a problem hiding this comment.
Hi @cs8425. I like your pull request, I think it's a valuable addition. Unfortunately I cannot test it at the moment, as I don't have an unused esp8266 (some are in the mail).
I have made some suggestions for fixes / improvements. Can you please amend them?
js/serial.js
Outdated
| @@ -1,6 +1,9 @@ | |||
| 'use strict'; | |||
|
|
|||
| var serial = { | |||
| connectionType: 'serial', // 'serial' or 'tcp' | |||
There was a problem hiding this comment.
Just for maintainability, can you please add new stuff to the bottom of the list?
There was a problem hiding this comment.
after failed, before transmitting?
or after outputBuffer, before connect?
Sorry for my bad English.
There was a problem hiding this comment.
Where it is now is good. Having new stuff at the beginning is confusing. And don't worry about English, it isn't my first language either, and the same goes for many contributors here.
js/serial.js
Outdated
| @@ -12,10 +15,76 @@ var serial = { | |||
| transmitting: false, | |||
| outputBuffer: [], | |||
|
|
|||
| LOGHEAD: 'SERIAL: ', | |||
There was a problem hiding this comment.
This isn't a constant, so it should be logHead.
js/serial.js
Outdated
| connect: function (path, options, callback) { | ||
| var self = this; | ||
| self.openRequested = true; | ||
|
|
||
| var testUrl = path.match(/tcp:\/\/(.*):(.*)/) |
There was a problem hiding this comment.
Be more specific here: /^http:\/\/([A-Za-z0-9\.-]+)\:(\d+)$/. This will also ensure that the port number is numeric.
js/serial.js
Outdated
| var testUrl = path.match(/tcp:\/\/(.*):(.*)/) | ||
| if (testUrl) { | ||
| self.connectionIP = testUrl[1]; | ||
| self.connectionPort = testUrl[2] || self.connectionPort; |
There was a problem hiding this comment.
testUrl[2] will always be defined. If you want it to default to the predefined port, make the port number optional in the regex: /^http:\/\/([A-Za-z0-9\.-]+)(?:\:(\d+))?$.
js/serial.js
Outdated
| self.openRequested = false; | ||
| } | ||
|
|
||
|
|
js/serial.js
Outdated
| connect: function (path, options, callback) { | ||
| var self = this; | ||
| self.openRequested = true; | ||
|
|
||
| var testUrl = path.match(/tcp:\/\/(.*):(.*)/) | ||
| if (testUrl) { | ||
| self.connectionIP = testUrl[1]; |
There was a problem hiding this comment.
I'd stick the two clauses of this if statement into connectTcp and connectSerial respectively to increase readability.
js/serial.js
Outdated
| console.log('setNoDelay', noDelayResult) | ||
| if(noDelayResult != 0) { | ||
| self.openRequested = false; | ||
| console.log(self.LOGHEAD + 'Failed to setNoDelay'); |
There was a problem hiding this comment.
Is this a case where the app requires additional permissions? If so, we should log it to the application's log with GUI.log(chrome.i18n.getMessage('<messageName>')). (Messages are in messaages.json.)
There was a problem hiding this comment.
app only requires 1 more permissions,
already in manifest.json.
setNoDelay is to disable Nagle's Algorithm,
which cause a little delay between data input and real send out.
Maybe change it as an option?
There was a problem hiding this comment.
No that's all good then. There are cases where apps need to be given special permissions in chrome (i.e. outside of the app), but if this isn't one of them then just logging it to the console is fine.
|
fixes and add a little error handle. known issue: |
|
I am not worried about the CLI issue. There is a similar one when using a serial connection: If disconnecting the cable while in CLI with a battery connected, it's not possible to reconnect before power cycling. |
|
Thank you very much for your contribution, @cs8425. |
use esp8266 as wifi-to-serial transparent bridge.
test with esp-link v2.2.3
I had disabled debug log, not test with debug log on.
(web setting page >> Debug log >> off, see: jeelabs/esp-link#83 )
usage:
select
Manual Selectiontype
tcp://esp-ip:port(esp-link default:tcp://192.168.4.1:2323)and click
connect