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

[BUG] WIFISUPPORT abuse #26624

Open
ellensp opened this issue Jan 4, 2024 · 6 comments
Open

[BUG] WIFISUPPORT abuse #26624

ellensp opened this issue Jan 4, 2024 · 6 comments

Comments

@ellensp
Copy link
Contributor

ellensp commented Jan 4, 2024

Did you test the latest bugfix-2.1.x code?

Yes, and the problem still exists.

Bug Description

WIFISUPPORT was added to Marlin in #11020
Exclusively for ESP32 based controllers.

Recently the define has been usurped to also include esp modules that have additional IO pins
#25586

But this makes no sense as WIFISUPPORT makes marlin include the following esp32 only libraries

(ESP3D_)?WIFISUPPORT                   = AsyncTCP, ESP Async WebServer
                                         ESP3DLib=https://github.com/luc-github/ESP3DLib/archive/dc0f3d96c6.zip
                                         arduinoWebSockets=links2004/[email protected]
                                         luc-github/[email protected]

This PR also broke allowing a standard SERIAL_PORT_2 on a ESP32 based controller
#26611
Also websockets on esp32 is currently broken..

Further the PR "Clarify WIFISUPPORT", did not clarify anything, most users have no idea if their module is "simple WiFi serial port" or not.
#26097

This directly leads to users creating issues such as #26622

Bug Timeline

Since Mar 29, 2023

Expected behavior

enabling WIFISUPPORT to behave as inteneded, one way or the other

Actual behavior

enabling WIFISUPPORT includes esp32 only librarys for all boards and does not work for modules

Steps to Reproduce

enable WIFISUPPORT on a random non esp32 based controller.

Version of Marlin Firmware

Bugfix-2.1.x

@ellensp
Copy link
Contributor Author

ellensp commented Jan 4, 2024

I purpose that these changed are reverted

Instead add WIFIMODULE
This set up the additional IO pins where they are required and warns of no addition IO pins when they are not defined and also defines a WIFIMODULE_SERIAL_PORT so it has a use with standard UART only wifi modules.

Then rename WIFISUPPORT to something less generic, perhaps MARLIN_RUNNING_ON_ESP32_WIFI

@sargonphin
Copy link
Contributor

Sorry if I upset you guys with this Wifi stuff, that wasn't my intention! 😰

@thisiskeithb
Copy link
Member

Sorry if I upset you guys with this Wifi stuff, that wasn't my intention! 😰

Not to worry, no one is upset. This has been fixed & then unfixed, so we're wanting to fix it again...for good, even if it means reinstating the original sanity check preventing the feature from being enabled on unsupported hardware.

@dc740
Copy link
Contributor

dc740 commented Apr 22, 2024

I stumbled on this by pure chance (found a wifi module I bought years ago and I thought it would be a quick plug and play)...

@ellensp suggestion seems the safest path to get wifi modules working again. while keeping ESP32 boards working too. We would get the best of both:

  • ESP8266 and other modules could be used with Marlin (ie: ESP3D based add-ons, like the MKS or the BTT esp8266 modules)
  • ESP32 based boards would still work.

For my use case (esp8266 as add-on on an octopus pro), right now the compilation tried to fetch all those unused libraries for no reason at all.

WIFISUPPORT is too generic and it means two different things depending on the use case. I think there is room to have both cases supported.

@dc740
Copy link
Contributor

dc740 commented Apr 24, 2024

as a side note, If I want to add support for RTL8710, which flag would that fall into? Same for the RDA5981? (I just googled these two as an example. don't take it literally)

ESP is not the only wifi enabled chip, so WIFISUPPORT seems too generic. Ideally I'd opt for a flag that enables initializing different chipsets without pulling in all the ESP dependencies.

As an example, the linked PR uses a very old ESP8266 I had from many years ago, and it requires GPIO15 to be pulled down to boot (same as reprap does). but I can only do it by enabling WIFISUPPORT.

I'm willing to create a PR with a proposal over the weekend, but I'd like that we all agree on a solution before putting time on it.

@SMH17
Copy link

SMH17 commented Oct 6, 2024

Around WIFISUPPORT an unbelievable number of messes has been done, and at the root of this there is a bad naming (WIFISUPPORT is too generic to indicate the WIFI feature of a specific family of MCUs) followed by a too loose PR verification in allowing the merging of code with unintended changes.
This has caused the merging of code of people that considered WIFISUPPORT as a general option for Wi-Fi availability and code of other people that considered WIFISUPPORT only for the Wi-Fi features of the ESP-based motherboards.

Now, many configurations require WIFISUPPORT enabled in order to make Wi-Fi module work, despite they aren't ESP-based motherboards, but at same time code isn't correctly compiled on these if WIFISUPPORT is enabled.

e.g.
Consider you want to use a MKS board with STM32 MCU and an ESP8266 module installed on it. A similar configuration should work without enabling WIFISUPPORT according official statement.

Unfortunately, this isn't true, and leaving WIFISUPPORT disabled, WIFI module doesn't work at all, and even if you try to access Wi-Fi feature in LVGL panel nothing happens despite ESP Wi-Fi module firmware correctly flashed, because the codebase includes conditional blocks that exclude Wi-Fi related features if WIFISUPPORT if false.
In older Marlin releases was enough to enable WIFISUPPORT regardless the configuration comment indication, and it worked, but now enabling it, the build fails, because it looks for dependencies specifically intended for ESP32 MCUs that aren't available for the environment of non-ESP motherboards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants