Skip to content
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

UDP port configuration confusion #264

Closed
lucaberta opened this issue May 4, 2017 · 25 comments
Closed

UDP port configuration confusion #264

lucaberta opened this issue May 4, 2017 · 25 comments
Labels
Category: Core related Related to the (external) core libraries Type: Bug Considered a bug
Milestone

Comments

@lucaberta
Copy link

As posted in: https://www.letscontrolit.com/forum/viewtopic.php?f=6&t=3024

I have compiled and ran the latest Mega code and have noticed that a few graphical changes have been done on the GUI, most notably in the Advanced tab.

One thing which has always been very confusing for me was the notion of "UDP port" configuration which in previous versions was put after the syslog IP and syslog level fields.
Now the label has been changed to "Syslog UDP port" and has been moved in between the syslog IP and syslog level field.

Except that the UDP port which is included in that configuration line is NOT used for syslog! It's instead used for the sendSysInfoUDP function which is extremely useful as it allows for automatic discovery of all the ESPEasy units on the local LAN, and they are displayed in a table at the bottom of the main tab, if one decides to put the same UDP port in that configuration item.

Granted, if you put 514 thinking that the port is for syslog (whose default port is 514) things DO work too, but it's quite misleading.

In fact, checking the actual syslog code, port 514 is hardwired in the syslog client at the very top of Networking.ino.

Some cleanup of the configuration and better description of this feature would be in order, in my view. It is extremely useful, and I have discovered it just by coincidence!

@psy0rz
Copy link
Member

psy0rz commented May 4, 2017

@jkDesignDE you probably create this change/bug? :)

@lucaberta
Copy link
Author

@psy0rz my guess is that the roots of the bug were already there, since the generic "UDP port" label in the advanced configuration page existed already way back when in ESPEasy v1.

When I traced the different lines of code last night it seemed obvious that the new label, probably introduced by @jkDesignDE as you say, goes deeper in the misunderstanding that the UDP port configuration field does NOT belong to the external syslog server configuration, since the 514 port is hardwired in the syslog function in Networking.ino.

Hope this help!

@psy0rz
Copy link
Member

psy0rz commented May 4, 2017

ah you're right indeed.

yeah i want to cleanup that networking code a bit, some things are quite fuzzy there and the sysinfo packets use ugly offsets instead of structs (like the other packets do).

also we need to get rid of those magic numbers (use defines), and it needs more comments.

plus i think there are some possible buffer overflows if it receives incorrect packets.

@psy0rz psy0rz added Type: Bug Considered a bug Category: Core related Related to the (external) core libraries labels May 4, 2017
@psy0rz psy0rz added this to the 2.0.0 milestone May 4, 2017
@JK-de
Copy link
Contributor

JK-de commented May 4, 2017

I ordered and grouped the page without knowing the exact meaning of this item. I thought it belongs to syslog because of its position - sorry.
Will be fixed soon.
Any suggestions for descriptive naming and grouping???

@lucaberta
Copy link
Author

You make a very good point @jkDesignDE which is, is there a real name for the "Node List" feature in ESPEasy?

I literally discovered it by coincidence when I decided to test external syslog and found that the UDP port there made no difference, but rather enabled the "Node List" feature in the Main tab.

I am still not 100% sure how it works as I have not checked the code with great attention, but suspect it's a broadcast based UDP protocol, where every node announces its presence to the others listening on the local subnet.

I also suspect that @psy0rz did a few additions between v1 and v2 as we now have additional fields in the Node List table, namely the "type" column which did not exist before, though I suspect that the data is generated locally by "decoding" the "build" field, so maybe the UDP protocol has not been changed at all.

In my view this should be a much highlighted feature as it allows to have a bird's eye view on all the ESPEasy devices in one's network. And maybe we should suggest a "standard" port to use. I use 8266 for it! ;-)

Any feedback?

@beicnet
Copy link
Contributor

beicnet commented May 4, 2017

Maybe it can bee called "Node List UDP port:" ;)

@tedenda
Copy link
Contributor

tedenda commented May 4, 2017 via email

@psy0rz
Copy link
Member

psy0rz commented May 4, 2017

yeah, global sync and the "node list" is done by the same UDP protocol. its a proprietary ESPEasy UDP protocol thats handled in networking.ino.

since this is still experimental, disable it if you have stability issues.

@beicnet
Copy link
Contributor

beicnet commented May 4, 2017

You can do it like this to avoid further description confusion! ;)

udp_example

@tedenda
Copy link
Contributor

tedenda commented May 4, 2017 via email

@JK-de
Copy link
Contributor

JK-de commented May 4, 2017

My suggestion for naming:

## Inter-ESPEasy Communication (experimental) #######################
UDP port:        [0815]
Global sync:     [X]

@psy0rz
Copy link
Member

psy0rz commented May 4, 2017

@jkDesignDE yeah like that is nice.

@lucaberta
Copy link
Author

How about "ESPEasy Discovery Protocol" instead?

I know I am nitpicking, but I think it's clearer, and Global sync rides on top of this protocol which performs the discovery, so I don't see any conflict nor confusion by the alternative naming.

A name such as "discovery protocol" has been used by many vendors in the past, and is quite clearly understood.

Hey, I have worked at Cisco Systems for 11 years, so I have my biases... :-D

@psy0rz
Copy link
Member

psy0rz commented May 4, 2017

Sounds good ! :)

@beicnet
Copy link
Contributor

beicnet commented May 5, 2017

@lucaberta Not a bad proposition! ;)

@svmac
Copy link
Contributor

svmac commented May 5, 2017

That UDP port is not only for discovery, it is used by SendTo command too. Like jkDesignDE, I propose group name "Inter-ESPEasy Network (experimental)".

@lucaberta
Copy link
Author

As I said above:

"I know I am nitpicking, but I think it's clearer, and Global sync rides on top of this protocol which performs the discovery, so I don't see any conflict nor confusion by the alternative naming."

The beauty of OSS is that I would be able to change the name in my own fork, if I don't like what has been chosen! 😆

JK-de pushed a commit to JK-de/ESPEasy that referenced this issue May 5, 2017
Group name can be changed during merge in... ;)
@beicnet
Copy link
Contributor

beicnet commented May 5, 2017

And why is it labeled as a bug?!? 🥇

@JK-de
Copy link
Contributor

JK-de commented May 5, 2017

...because the naming "Syslog UDP port" is wrong and it is placed in the wrong group

@beicnet
Copy link
Contributor

beicnet commented May 5, 2017

Ah, okay!

@uzi18
Copy link
Contributor

uzi18 commented May 5, 2017 via email

@beicnet
Copy link
Contributor

beicnet commented May 5, 2017

Because it's not only using for "Global sync"

@uzi18
Copy link
Contributor

uzi18 commented May 5, 2017 via email

@beicnet
Copy link
Contributor

beicnet commented May 5, 2017

It's already in Poll request query da3b11c !

@ghost
Copy link

ghost commented May 6, 2017

The UDP protocol usage inherits from the old Nodo project where it was used for peer2peer and broadcast type communication of events. It has also been used within ESP Easy to be able to communicate with (old) Nodo devices that use the Wiznet ethernet controller. That also explains the Unit number usage that was important for all Nodo communication. The Node list is not only for display, but it keeps track of all active nodes in case of unicast traffic to all listening nodes. The size of this list is limited to 32 units to save valuable RAM. (this could also be done with UDP broadcasting but there have been issues with ESP units being less receptive on broadcast traffic for some reason...)

The UDP port is there to configure the basic UDP server process that could effectively handle multiple types of traffic and usage. Just like the generic webserver process on port 80.

psy0rz pushed a commit that referenced this issue May 8, 2017
Group name can be changed during merge in... ;)
@psy0rz psy0rz closed this as completed May 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Core related Related to the (external) core libraries Type: Bug Considered a bug
Projects
None yet
Development

No branches or pull requests

7 participants