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

Rework of arduino compatibility in WiFiSTAClass::config #4145

Merged
merged 5 commits into from
Jan 17, 2018
Merged

Rework of arduino compatibility in WiFiSTAClass::config #4145

merged 5 commits into from
Jan 17, 2018

Conversation

devyte
Copy link
Collaborator

@devyte devyte commented Jan 12, 2018

No description provided.

@devyte
Copy link
Collaborator Author

devyte commented Jan 12, 2018

No idea why the diff took the entire function.
@d-a-v I added your suggestion to check that gateway and ip are in the same subnet.

@devyte
Copy link
Collaborator Author

devyte commented Jan 12, 2018

Fixes #4114

@devyte
Copy link
Collaborator Author

devyte commented Jan 12, 2018

Going to do another iteration of this, after discussion with @d-a-v .

@devyte
Copy link
Collaborator Author

devyte commented Jan 14, 2018

@d-a-v I may have this wrong, but the check we discussed seems to give false positives.
Please check me on this:

The first part of the check calculates that both IP and gateway are in the same subnet for both possible argument orders.
ESP argument order is: ip, gateway, subnet, dns1
Arduino arg order is: ip, dns, gateway, subnet.

  bool espOrder = (local_ip & arg2) == (arg1 & arg2); 
  bool arduinoOrder = (local_ip & arg3) == (arg2 & arg3);

The second part of the check was to figure out what to do:

  • if one or the other is true, you know the argument order
  • if both are false, there is a mistake in the values, so error out (or rather, return false)

However, consider this example:

  ip:  192.168.0.128
  gw:  192.168.0.1
  sn:  255.255.255.0
  dns: 192.168.0.1

If the arguments are given in ESP order:

           a0 a1 a2 a3
 esporder: ip gw sn dns

bool espOrder = (192.168.0.128 & 255.255.255.0) == (192.168.0.1 & 255.255.255.0); //true
bool arduinoOrder = (192.168.0.128 & 192.168.0.1) == (255.255.255.0 & 192.168.0.1); //true

In which case the check can't tell which order was used. If the above is correct, then I think it'd be better to stick with this PR as it is.
I could have missed something, though :P

@d-a-v
Copy link
Collaborator

d-a-v commented Jan 14, 2018

You are damn right. The test I proposed is not sufficient.
I don't feel right testing netmask[0][&255]==255 because it's a particular case (why not nm[0]&0b11111000==0b11111000)(248) or with 224, 240, 252...
So I should split hairs no more, testing the first netmask's byte would be sufficient and common enough.

@devyte devyte self-assigned this Jan 17, 2018
@devyte devyte added this to the 2.5.0 milestone Jan 17, 2018
@devyte devyte merged commit b08ff10 into esp8266:master Jan 17, 2018
@devyte devyte deleted the issue4114 branch January 17, 2018 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants