Marlin icon indicating copy to clipboard operation
Marlin copied to clipboard

[BUG] WIFISUPPORT abuse

Open ellensp opened this issue 2 years ago • 6 comments

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

Yes, and the problem still exists.

Bug Description

WIFISUPPORT was added to Marlin in https://github.com/MarlinFirmware/Marlin/pull/11020 Exclusively for ESP32 based controllers.

Recently the define has been usurped to also include esp modules that have additional IO pins https://github.com/MarlinFirmware/Marlin/pull/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 https://github.com/MarlinFirmware/Marlin/issues/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. https://github.com/MarlinFirmware/Marlin/pull/26097

This directly leads to users creating issues such as https://github.com/MarlinFirmware/Marlin/issues/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 avatar Jan 04 '24 01:01 ellensp

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

ellensp avatar Jan 04 '24 02:01 ellensp

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

sargonphin avatar Jan 04 '24 02:01 sargonphin

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.

thisiskeithb avatar Jan 04 '24 04:01 thisiskeithb

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 avatar Apr 22 '24 08:04 dc740

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.

dc740 avatar Apr 24 '24 08:04 dc740