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

Adding softAP SSID & PSK query API #4138

Merged
merged 18 commits into from
Mar 28, 2018
Merged

Adding softAP SSID & PSK query API #4138

merged 18 commits into from
Mar 28, 2018

Conversation

IMAN4K
Copy link
Contributor

@IMAN4K IMAN4K commented Jan 11, 2018

Signed-off-by: Iman Ahmadvand [email protected] (github: @IMAN4K)

Signatures:
String ESP8266WiFiAP::softAPSSID() const;
String ESP8266WiFiAP::softAPPSK() const;
String ESP8266WiFiAPClass::softAPPSK() const {
struct softap_config config;
wifi_softap_get_config(&config);
return String(reinterpret_cast<char*>(config.password));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, this could cause an issue. When a 64-byte psk is used, it is not 0-terminated, and String assumes a null-terminator.
Instead, a char psk[65] should be declared, config.password should be memcpy()d into it, and then psk[64] = 0;. Then, return String(psk).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devyte
Yes That's right.
This is the String constructor:

String::String(const char *cstr) {
    init();
    if(cstr)
        copy(cstr, strlen(cstr));
}

}

/**
* Get the configurd(Not-In-Flash) softAP PSK or PASSWORD.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor typo -> configured.

@devyte devyte mentioned this pull request Jan 24, 2018
String ESP8266WiFiAPClass::softAPSSID() const {
struct softap_config config;
wifi_softap_get_config(&config);
char* name = reinterpret_cast<char*>(config.ssid);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just

config.ssid[sizeof(config.ssid) - 1] = 0; // guard
return String((const char*)(void*)config.ssid) ?

String(char*) will anyway search for the last \0

Copy link
Contributor Author

@IMAN4K IMAN4K Jan 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@d-a-v
Based on @devyte comment, user must be able to configure 64 byte length PSK at max, not 63

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK sorry I did not see the hidden-outdated comment.

@@ -47,6 +47,9 @@ class ESP8266WiFiAPClass {
uint8_t* softAPmacAddress(uint8_t* mac);
String softAPmacAddress(void);

String softAPSSID() const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation on this line is different from nearby lines

* @return String SSID.
*/
String ESP8266WiFiAPClass::softAPSSID() const {
struct softap_config config;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation in this function is different from nearby functions

@@ -112,4 +112,51 @@ String IPAddress::toString() const
return String(szRet);
}

/* ref: https://code.woboq.org/userspace/glibc/resolv/inet_pton.c.html#inet_pton4 */
static bool inet_pton4(const char *start, const char *end) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this function different from IPAddress::fromString(), found here? If they serve equivalent purpose, I'd rather not duplicate.

Copy link
Contributor Author

@IMAN4K IMAN4K Mar 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devyte
So sorry for late reply.
You say we should modify bool IPAddress::fromString(const char *address) function for adding validate support ? without duplicate
Because current IPAddress public API doesn't support validating IPV4 as string

Copy link
Contributor Author

@IMAN4K IMAN4K Mar 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devyte
My suggestion:
1.Adding a private validating method:
static bool IPAddress::isValid(const char* arg, size_t len);
2.Adding correspond public method:
static bool IPAddress::isValid(const String& ip);
3.Reuse method of 1 inside 2 and bool IPAddress::fromString(const char *address) function

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IMAN4K said:

Because current IPAddress public API doesn't support validating IPV4 as string

Are you saying that the following does not validate a string?
bool isvalid = IPAddress().fromString("17604123874106239");

Copy link
Contributor Author

@IMAN4K IMAN4K Mar 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devyte
Yes that's correct, my mistake!
But we need to make an object for every validation.
It's better with a public static member(with good api name isValid rather than fromString), then every where in code:
if (IPAddress::isValid(...)) ...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The object constructed is 4 bytes in size, essentially one int, which is a temporary instantiated on the stack and immediately destroyed, so that's not a problem (btw, destruction is trivial).
What I suggest is a static convenience method like you propose, but something like this:

bool IPAddress::isValid(const char *str)
{
  return IPAddress().fromString(str);
}
bool IPAddress::isValid(String &str)
{
  return IPAddress().fromString(str);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.
I'll make changes very soon.

@devyte
Copy link
Collaborator

devyte commented Mar 23, 2018

@IMAN4K will you be replying to this?

@@ -112,4 +112,51 @@ String IPAddress::toString() const
return String(szRet);
}

/* ref: https://code.woboq.org/userspace/glibc/resolv/inet_pton.c.html#inet_pton4 */
static bool inet_pton4(const char *start, const char *end) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The object constructed is 4 bytes in size, essentially one int, which is a temporary instantiated on the stack and immediately destroyed, so that's not a problem (btw, destruction is trivial).
What I suggest is a static convenience method like you propose, but something like this:

bool IPAddress::isValid(const char *str)
{
  return IPAddress().fromString(str);
}
bool IPAddress::isValid(String &str)
{
  return IPAddress().fromString(str);
}

@devyte devyte merged commit 241531a into esp8266:master Mar 28, 2018
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.

4 participants