-
-
Notifications
You must be signed in to change notification settings - Fork 96
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
Improve setup LAN DHCP connection #636
Conversation
BSB_LAN/BSB_LAN.ino
Outdated
if (network_type == LAN) { | ||
SerialOutput->println(Ethernet.localIP()); | ||
SerialOutput->println(Ethernet.subnetMask()); | ||
SerialOutput->println(Ethernet.gatewayIP()); | ||
} else { |
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.
This was already logged by ARDUINO_EVENT_ETH_GOT_IP
and so redundant
I appreciate you working on our code, but I would very much appreciate it if you let me know first what you are planning to do and discuss this with me first. That saves at least me and most likely both of us a lot of time if I don't have to go through several pull requests and changed files and have to do a lot of guesswork... |
This was in draft and I wasn't expecting you to react so quickly. I still have to provide you the complete description so that you can easily understand the difference (before / after). I have TCP socket issue that I try to understand. As I was looking at the setup debug message, I improved a bit readability. Besides this, I don't want to change anything here |
12779b4
to
3d6fdee
Compare
The event messages are still new and will most likely only be enabled when developer debug mode is active. This may lead to improper formatting etc., but since this will be only used in rare circumstances, I don't think it is an issue. In any case, it should not lead to a situation where output for "normal" mode is removed (as it seems to be the lines 7723++ you mentioned above. |
Also, you removed this line: |
I just moved down this part to not repeat all the conditions in static or dhcp |
3d6fdee
to
dcc9fcf
Compare
I wasn't in debug mode, but to be safe, I put it back but with labels.
|
dcc9fcf
to
1e31c5b
Compare
Ok, I didn't see that. One of the reasons why I prefer opening a bug report or feature request before submitting PRs where the intention can be discussed first as this makes code review or PRs a lot easier. |
1e31c5b
to
b597268
Compare
You're right. I was initially using Ethernet.connected() but this is not in stable. I put it back and added in Eth class. Result without lan cable:
With cable:
|
b597268
to
6353e43
Compare
} | ||
} | ||
|
||
#if defined(ESP32) | ||
if (network_type == LAN && !Ethernet.connected()) createTemporaryAP(); |
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.
By the way, a parameter inside createTemporaryAP
to disable such backdoor on network issue would be recommended. But that's another topic...
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.
What do you mean with "such backdoor on network issue"? Please be more specific if you raise issues.
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.
It looks like if you have a LAN issue (for example a power outage that will cause esp to restart faster than the lan), you are creating an open AP that anyone can discover and connect to.
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.
That may be the case, yes. But that person would need to know the password for the temporary WiFi network and also all the other security related aspects (such as passcode, HTTP credentials) in order to do anything.
Under what condition is |
|
I'll run more test soon and let you know |
I checked the sources and this specific bit is set/cleared upon the event ARDUINO_EVENT_ETH_CONNECTED or ARDUINO_EVENT_ETH_DISCONNECTED respectively. So this is what the event handler already covers. However, I think that I observed that if there is no link in the first place, there is no "disconnected" event, and the same goes IIRC for cases where there is no Ethernet adapter in the first place (i.e. on a NodeMCU). This means that if a user has set the network type to LAN on a NodeMCU, this condition won't start the AP. |
Not sure I understand properly what you mean by mis-configuration.
LAN DHCP or static, there is no change, it's resetting on begin as the brownout detector triggers
|
Huh? Where is that brownout coming from? That's usually a hardware issue. |
This is what I get on an ESP32-NodeMCU when the default network_type value is not changed:
Up to this point, no network event fires, but at least ETH_CONNECTED_BIT is initially zero, so setting up the temporary AP works in this case. What I can't test here is if the same also works if |
Indeed. I tried with a different esp32 NodeMCU board and this is what I get: With Static IP
With DHCPWe see it's not waiting to acquire an IP
Before
|
Have your tests been done with a plugged-in network cable or not? My questions before were based on the assumption that a network cable is conencted and the conditions a) and b) are met. |
6353e43
to
5749202
Compare
I have implemented most of this manually after this PR was no longer compatible with framework 3.0.1. Thanks! |
You can do whatever you want with this PR. Besides improving logged message, nothing is changed
Before
Issue
After