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

WiFiSTA - new status constant WL_STOPPED #8849

Merged
merged 2 commits into from
Nov 10, 2023

Conversation

JAndrassy
Copy link
Contributor

WL_NO_SHIELD as initial status() value stops any generic Arduino WiFi example

Generic Arduino WiFi examples start with

  if (WiFi.status() == WL_NO_SHIELD) {
    Serial.println("Communication with WiFi module failed!");
    // don't continue
    while (true) {
      delay(1);
    }
  }

@CLAassistant
Copy link

CLAassistant commented Nov 8, 2023

CLA assistant check
All committers have signed the CLA.

@SuGlider SuGlider self-requested a review November 9, 2023 13:28
@SuGlider
Copy link
Collaborator

SuGlider commented Nov 9, 2023

@JAndrassy - I think that this change will cause issues. WL_IDLE is used for other cases.

I can see that WL_NO_SHILED is in the Arduino WiFi Shiled Library examples, but it is not the case for Arduino ESP32.
https://github.com/arduino-libraries/WiFi/blob/master/examples/WiFiWebClient/WiFiWebClient.ino

I also see that the Arduino WiFi Library has a Note: this library was retired and is no longer maintained.

Looking into the new UNO R4 WiFi, it uses other way to check the module: WL_NO_MODULE = WL_NO_SHIELD and it comes from ESP32 module.
https://github.com/arduino/ArduinoCore-renesas/blob/main/libraries/WiFi/examples/WiFiWebClient/WiFiWebClient.ino

In ESP32 Arduino when the WiFi Interface is suspended, it goes to WL_NO_SHIELD state, which makes sense because it would be like "removing the shield". When the code starts the WiFi is suspended, which is equivalent to WL_NO_SHIELD...

This status is changed in the WiFi Event Call back
https://github.com/espressif/arduino-esp32/blob/master/libraries/WiFi/src/WiFiGeneric.cpp#L1036

} else if(event->event_id == ARDUINO_EVENT_WIFI_STA_STOP) {
WiFiSTAClass::_setStatus(WL_NO_SHIELD);
clearStatusBits(STA_STARTED_BIT | STA_CONNECTED_BIT | STA_HAS_IP_BIT | STA_HAS_IP6_BIT);

Copy link
Collaborator

@SuGlider SuGlider left a comment

Choose a reason for hiding this comment

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

I don't see a clear motivation to this change in the ESP32 Arduino scope.
@me-no-dev - what do you think?

@me-no-dev
Copy link
Member

@SuGlider agreed.

@JAndrassy
Copy link
Contributor Author

maybe be there should be a new constant in wl_status_t for stopped STA

@SuGlider
Copy link
Collaborator

SuGlider commented Nov 9, 2023

maybe be there should be a new constant in wl_status_t for stopped STA

Let's say that we create a WL_STOPPED and this is the initial state. What would WL_NO_SHIELD be used for?
I only can find WL_NO_SHIELD in the ESP32 Arduino WiFi Code in 3 places:

1- in the declaration of the wl_status_t enumeration in WiFiType.h
2- for the initial value of static wl_status_t _sta_status = WL_NO_SHIELD; in WiFiSTA.cpp
3- when the the event ARDUINO_EVENT_WIFI_STA_STOP happens + WiFiSTAClass::_setStatus(WL_NO_SHIELD); in WiFiGeneric.cpp

A new constant for stopped STA would be used in (2) and (3). It should be added with a higher value (254) because the inital state must be higher than WL_DISCONNECT to get it started in the code of uint8_t WiFiSTAClass::waitForConnectResult(unsigned long timeoutLength).

This would make WL_NO_SHIELD useless and it would be there just for backward compatibility.

But ... this could be a way to make it compatible with Arduino Upstream. @me-no-dev PTAL.

@JAndrassy
Copy link
Contributor Author

This would make WL_NO_SHIELD useless and it would be there just for backward compatibility.

as already the comment next to is states :-)

@SuGlider
Copy link
Collaborator

SuGlider commented Nov 9, 2023

This would make WL_NO_SHIELD useless and it would be there just for backward compatibility.

as already the comment next to is states :-)

OK, please proceed with the change for evaluation.

@SuGlider SuGlider self-assigned this Nov 9, 2023
@SuGlider SuGlider self-requested a review November 9, 2023 17:36
Copy link
Collaborator

@SuGlider SuGlider left a comment

Choose a reason for hiding this comment

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

Let's create a WL_STOPPED = 254 and use it as inital value.

to change initial WiFi.status() because WL_NO_SHIELD
is checked in any generic Arduino WiFi example
@JAndrassy JAndrassy changed the title WiFiSTA - change initial WiFi.status() to WL_IDLE_STATUS WiFiSTA - new status constant WL_STOPPED Nov 9, 2023
@me-no-dev
Copy link
Member

All those statuses are really there only for some sort of compatibility. Reality is that original WiFi library is STA only, external chip with a very different capabilities. I am generally all for compatibility, as long as it does not prevent us to progress and provide new and better things. As @SuGlider said, this is almost never used, so I am also fine with whatever you guys decide, as long as it does not break anything.

Copy link
Collaborator

@SuGlider SuGlider left a comment

Choose a reason for hiding this comment

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

Thanks @JAndrassy - LGTM.

@me-no-dev me-no-dev merged commit 3fa2807 into espressif:master Nov 10, 2023
37 checks passed
@JAndrassy JAndrassy deleted the wifi_init_status_idle branch November 11, 2023 04:00
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.

None yet

4 participants