-
Notifications
You must be signed in to change notification settings - Fork 727
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Variable TCP Telnet ports via web interface #205
base: master
Are you sure you want to change the base?
Conversation
Updates for changing/selecting telnet ports
Updates for changing/selecting telnet ports
Fixed formatting/extra space
Updates for changing/selecting telnet ports. Fixed formatting.
Moved new variable at bottom as requested in this header comments.
Updates for changing/selecting telnet ports
Updates for changing/selecting telnet ports
Todo: Missing function for init in serbridge?
Updates for changing/selecting telnet ports -Add function to reinit serbridge after change Telnet ports.
Updates for changing/selecting telnet ports
Make sure we use our custom telnet ports at bootup
Fixed bad variable reference
Fix implicit function call.
Remove unecssary function
Fixed a stupid typo in a function name.
Changed function for unsigned integers.
Build failed... but I cannot determine why. esp-link/config.c:39:3: error: large integer implicitly truncated to unsigned type [-Werror=overflow] unsigned integer, 8 bits should hold a max of 65535 - perfect for setting a TCP port, or so it would seem. Do I need to change this to a uint16_t? Thanks for the input/help. |
Changed flashconfig.telnet_port to unsigned 16bit integer.
Changed functions for unsigned 16bit integer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking promising! :-)
} | ||
|
||
int8_t ok = 0; | ||
uint8_t port1, port2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ports are 16 bits: 0..65535
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course! I got the thought of a short int = 8bit stuck in my head yesterday.
Once I realized that... unsigned 16bit should get me there.
uint8_t port1, port2; | ||
ok |= getUInt16Arg(connData, "port1", &port1); | ||
ok |= getUInt16Arg(connData, "port2", &port2); | ||
if (ok < 0) return HTTPD_CGI_DONE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't just return here, you have to send some HTTP response back. A 304 not modified or a 200 OK would be reasonable choices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ill modify it to include the responses.
} | ||
return HTTPD_CGI_DONE; | ||
|
||
collision: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put this inline since there is only one "goto" to here.
|
||
// apply the changes | ||
//serbridgeReinit(); | ||
serbridgeInit(flashConfig.telnet_port1, flashConfig.telnet_port2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please apply change after saving otherwise you might apply the changes, fail to save and return an error, but then things are inconsistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point
#include "httpd.h" | ||
|
||
int cgiTelnet(HttpdConnData *connData); | ||
// int8_t telnet_port1, telnet_port2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can delete this comment...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
@@ -96,6 +97,8 @@ HttpdBuiltInUrl builtInUrls[] = { | |||
{ "/services/info", cgiServicesInfo, NULL }, | |||
{ "/services/update", cgiServicesSet, NULL }, | |||
{ "/pins", cgiPins, NULL }, | |||
{ "/telnet/info", cgiTelnet, NULL}, | |||
{ "/telnet/update", cgiTelnet, NULL}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You only need one since you distinguish using GET and POST. Not sure why there are two for services, something for me to check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I picked up on this. However I wanted to attempt to avoid someone making an easy mistake of sending a PUT request via '/telnet' & accidently modifying it.
However, ill be happy to merge it to just one.
Also, ill have to modify the Ajax method for making inputs since it makes a call to /update
Services uses two because this & pins cgi use a single method to call back set or get. They determine which to use based on the header received(or GET or PUT)
@@ -469,6 +469,8 @@ serbridgeInitPins() | |||
} | |||
|
|||
// Start transparent serial bridge TCP server on specified port (typ. 23) | |||
// Here is where we need to change the ports. But how do we do this from the Ajax call? | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete left-over comment? You have that here https://github.com/jeelabs/esp-link/pull/205/files#diff-b6cc56038011d1226a2ee85cb280b897R47
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, old comment. Left it for me to find while working on this.
if (ok < 0) return HTTPD_CGI_DONE; | ||
|
||
char *coll; | ||
if (ok > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I heaven't dealt with the ajax stuff in a while and forgotten everything (oops), but I think you're gonna get one port at a time when you fill out one input field and hit enter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right. Ill modify to have it check the flashconfig variables.
Corrected several mistakes (16 vs 8bit int) Check to see which variable we are modifying & fill the other one from flag so we can make sure they do NOT conflict. Added return error http header using existing collision goto.
Changes made so we can use a single namespace "/telnet" vs two. Somewhat dirty fix inserted into existing ajax function.
Place variable declartion in correct place.
Haven't done extensive tests yet but after the "change call to 0/1 instead of 1/2" commit, it appears to work. |
Should make code more fluid & inuitive. All code changes made to reflect javascrupt, html, and C.
This should be ready to test again. Try the build from here: |
@@ -52,12 +52,15 @@ ESP_HOSTNAME ?= esp-link | |||
# Base directory for the compiler. Needs a / at the end. | |||
# Typically you'll install https://github.com/pfalcon/esp-open-sdk | |||
# IMPORTANT: use esp-open-sdk `make STANDALONE=n`: the SDK bundled with esp-open-sdk will *not* work! | |||
XTENSA_TOOLS_ROOT ?= $(abspath ../esp-open-sdk/xtensa-lx106-elf/bin)/ | |||
XTENSA_TOOLS_ROOT ?= $(abspath ../espressif/xtensa-lx106-elf/bin)/ | |||
$(warning Using XTENSA TOOLS from $(XTENSA_TOOLS_ROOT)) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please look at this commit for a more thorough explanation of changes made in Makefile
fuzzball03@286e886
I may write a new javscript method all together for this... My goal might be to make one that is more adaptable to save on time & reduce overall size/complexity of code.
FYI I am continuing work, based on your code. My changes aren't stable enough to commit yet. |
Same here. I've done some pretty extensive changes to the HTML and On Tue, Nov 8, 2016, 12:05 AM dannybackx [email protected] wrote:
|
@dannybackx The code works, though it still needs some serious cleanup. |
Thanks, I didn't find this yet. |
The new UI for changing the ports looks great in this version, but the menu bar at the left won't work. Do you see the same ? Also many of the areas keep showing the spinning circle. |
What is the status of this PR? It doesn't look like it's ready to be merged and some of the changes have been implemented in other PRs? There are lots of changes to travis and Makefile here that make me rather uncomfortable. Should this be closed and a more focused PR opened if there is interest? |
Hello, I'm also interested if the Ip and port can be changed from the browser interface. thank you! |
Any body can send me the binary with the tcp port option? It is a great feature. |
why you need different port? |
Some aplications has other port For example OBD2 APP |
you can redirect it on localhost |
Per issue #167 ... Here ya go, first attempt, untested.
Spent an hour or so at work on this today...
I've only done really simple C coding, this will be my first that consists of multiple headers.
Please be easy on me - CONSTRUCTIVE CRITICISM WELCOME.
Note - this is untested.