[FR] Marlin 2.0 ESP32 support for alternative WiFi lib like ESP3D

[luc-github]

Hello,
I have ported ESP3D https://github.com/luc-github/ESP3D and the corresponding WebUI https://github.com/luc-github/ESP3D-WEBUI for Marlin ESP32 here : https://github.com/luc-github/Marlin/tree/bugfix-2.0.x

The behavior is same as the one I did for WiFi backpack and Azteeg WiFi : https://youtu.be/br7BryjWPJU?t=223, it allows SD upload, Printer control, Marlin configuration, and much more ^_^

I do not think making a PR make sense as there is already a basic WiFi support already merged (#11209).

Instead I would like to propose a PR that give entry points in ESP32 HAL for an alternative external library like #define USE_EXTERNAL_WIFI_HANDLE in Configuration_adv.h so by default if this define is not enabled the existing WiFi handler is used.

The PR would also add entry point in GCODE as I use M585/586/587/588/589 to configure wifi and settings. So need to be able to add extra GCODE commands.

If these entries points would exist, this would easily simplify the code for any new Handler without touching the Marlin code.
It would allow also to update external lib without affecting Marlin code and not maintain a fork for Marlin with ESP3D of course.
Additionnaly this would avoid any wifi issue related to external lib to be handled on Marlin github and so not bring extra work on Marlin.

Before starting moving Marlin for ESP3D as library I would like to know if such PR would be accepted. I would be happy to do it of course.

Please let me know.

thank you

[mylife4aiurr]

outstanding work

[vivian-ng]

I have been using @luc-github fork which allows use of ESP3D. I must say it sets the stage for using the ESP32 to its full capabilities by providing a functional webUI. Without something like the ESP3D webUI, the ESP32 ends up just being yet another 32-bit microcontroller with less pins; its WiFi capability is not being fully exploited.

With ESP3D webUI, the printer can be controlled using a web browser on a smart phone. Files can be sent to it via my PC (where I run my slicing software), then printed using any web browser either on my phone or PC. I have a webcam near my printer too, and the webUI also allows you to put the IP address of the webcam. This allows me to actually see the printer printing too.

I really hope those working on the ESP32 HAL, and Marlin code in general, can see how we can integrate this feature in the main Marlin repo.

[luc-github]

@felixstormI link and continue discussion started here because it better not to poluate other topic ^_^
simon-jouet/ESP32Controller#10 (comment)

some small separate PRs .... replacing EEPROM SPIFFS with NVS,

this one is easy and not actully related to ESP3D so it should not be an issue

another one with your synchronous WebServer implementation (as an alternative using #ifdefs) and then wait for them to be merged

Well that one is not small as web servers API are totally different as well as constraints in code, the handlers are differents as well as type, I do a support such thing in ESP3D 2.0 and 3.0 and this complexify code, that is why I suggested to separate code because bring everything to HAL will make things different, also there are others web servers available that could be used like use the httpd one used for ESP32 Cam.
here the headers of some part of the coexistence.
https://github.com/luc-github/ESP3D/blob/2.1/esp3d/asyncwebserver.h
https://github.com/luc-github/ESP3D/blob/2.1/esp3d/syncwebserver.h

and to again clarify the webUI is not part of that code - we are talking about web url and handler, as well as functions that are handled in code like DHT notification, email/push notifications, telnet, and any other network features.

I am open to any implementation - but merging ESP3D webserver code may bring lot mess (if it is even merged after work is done make both webserver to be defined at compilation level)

[felixstorm]

@luc-github about WiFi, OTA etc. - I do not think it's necessary to support both WiFi implementations. The current one IMHO is very, very basic (AFAIR it will even prevent Marlin from booting if it cannot connect), so IMHO enhancements would be very welcome here anyway. I would again suggest a separate PR that can take mode, SSID and password from defines in Configuration_adv.h for now as long as there are no g-codes yet to change the settings.

And about the web server implementation: I agree it makes stuff a bit more complicated, but probably not as much as it seems. The current code is enclosed in #if ENABLED(WEBSUPPORT), so if you rename that to WEBSUPPORT_ASYNC you're probably done with this. And your code is enclosed in #ifdef ENABLE_HTTP so you could take a similar path here and it would probably make stuff clearer to move the different implementations to separate sub folders of src/HAL/HAL_ESP32). Then you may have to add your libraries to platformio.ini (compiler and linker should add only the ones actually used in code to the output) and probably do some minor cleanup - but this might be it.

Again - this is only my humble personal opinion, but if I am not mistaken @simon-jouet, @grownseed and @Idorobots expressed very similar opinions here.

[luc-github]

@felixstorm sorry I may not have same reading as you about the thread,
the last opinion of @grownseed I got was #11209 (comment)

t sounds like you're a lot further into this than I am, so I'm wondering if there's any point taking my branch any further.

About maintaining Async and Sync together, yes I know the impact of maintaining such situation this is why I stop to do it in new version of ESP3D. :)
About wifi OTA implementation, take separaly it is fine but they are part of whole wifi features IMHO.

Your suggestion to have both web servers solutions (which cover most of the network code actually) in HAL, it will add more code to maintain in Marlin repo not related to Marlin itself, more issues to monitor, more PR to review, etc ... and again for something not really related to Marlin but WiFi or even soon ethernet.

Anyway, let's say it is Ok to handle such new code, (I am not talking about replacement like EEPROM support but Async / Sync servers) what the benefit to make both solutions to coexists in Marlin?

Current one with webserver with serial like output monitoring and fixed wifi, and the features present in my fork (https://github.com/luc-github/Marlin/issues/10).
And do not misunderstand me, I fully respect what @grownseed did, but he wrote himself : what the point ?

Again to clarify, my proposal is not only for ESP3D code but any wifi solution, like WiFiManager or any other libs handling wifi features. and any new board with also wifi feature but different API than ESP32

But I would prefer to get an official direction/ decision instead of trying one small PR first to see what happen...

Also to be honest this ESP3D porting to Marlin is not my main project, I did it as I promised to @simon-jouet when we discussed at the begining and maintained per request for @vivian-ng,
So:
If decision/plan is to not allow alternative lib I am fine.
If decision/plan is to merge all step by step, I am fine
If decision/plan is to replace existing solution, I am fine.
if decision/plan is not merging anything from my repo, I am fine too ^_^.

But will be more than happy to invest time, do MR/PR, etc, if needed, but only if direction is defined.

I think it make sense to have a plan first instead of blind coding, and do not waste time of Marlin Developers reviewing/rejecting MR/PR ^_^

[vivian-ng]

I wonder if the Marlin team and the active contributors to the ESP32 HAL can provide some direction on the way ahead for @luc-github so that he can move ahead.

1. Coexistence: Basically, leave the current ESP32 HAL as it is, while using a ton of #ifdef statements to encapsulate the changes made by @luc-github. This gives users options to choose what they want, but we will probably end up with a lot of redundant code, like two copies of implementation for EEPROM, two copies of implementation for OTA, etc.
2. Replacement: Replace existing features like EEPROM support, OTA, and webserver/UI with @luc-github's implementation. This is a decision which the writers of those bits of code should have a say in, so
   @thinkyhead @simon-jouet @grownseed @Idorobots @felixstorm should have some say in this. It doesn't have to be a complete replacement of everything in the HAL, just the parts which the developers think should be replaced.
3. External lib: @luc-github's implementation is made an "external lib" which can be included by the user if so desired. Personally, I don't know how he plans to do this, but to me, it would be a nightmare trying to maintain what is basically two separate HALs for the ESP32.

Hopefully, after getting all your inputs, @luc-github will have enough direction to know how best to merge his work with the main Marlin repo.

[simon-jouet]

Slow reply time, i've been very busy and little time I had went into the new board.

Regarding Async/Sync for the webserver code, if the plan is to bring @luc-github code then I don't think there is much point keeping the current async code. It will just add maintenance over time and if nobody uses it then there isn't much point. The current code already needs maintenance because it doesn't fully work, so not much enabling/disabling broken code as an option.

The current OTA code is working fine, so not sure there is much benefits changing that?

Bringing the features of ESP3D into Marlin would be very good the web interface it provides is clearly much much more advanced than the current code. If it can be cleanly added to the current code it would clearly be a great addition.

[vivian-ng]

I was facing an issue with the ESP32 rebooting each time an endstop was triggered during homing on a coreXY setup. It was narrowed down to an issue with the endstop_triggered function in stepper.cpp due to the ESP32 not being able to use floating point operations during interrupts. @luc-github came up with a solution for this by using a divide by integer 2 (/ 2) instead of multiply by float 0.5 (0.5f *). While this is specific to ESP32, still, with no clear directions on how his fork can be merged with the main repo, we are not able to do a PR to get this fix back into the main repo.

Since most of those contributing to the ESP32 HAL like @simon-jouet have already given their inputs, maybe @thinkyhead can provide a bit of clarity on how to proceed with this issue?

[felixstorm]

@vivian-ng IMHO you should probably just file a regular PR replacing the multiplication by 0.5f with a division by 2. In case of any objections you could still change the PR to only divide by 2 if you're compiling for an ESP32 and leave the old code for the other architectures.

[vivian-ng]

Hi, now that 2.0.0 is officially released, is it time to revisit this issue and see how we can merge back the changes by @luc-github for the ESP32 controller into the main repo?

[vivian-ng]

@luc-github I think we have the "official" view on this from @thinkyhead.

Scott Lahteine My official view is that developers should get to know GitHub and how to submit Pull Requests to the projects they work on. ://marlinfw.org/.../getting_started_pull_requests.html

Maybe you can just submit your fork as a new branch through a pull request?

[luc-github]

@vivian-ng @thinkyhead the question is not about knowing github (I think I know) neither how to do PR (I think I know too) - but about Marlin guidelines development to know if it is better to have entry points to integrate external lib when there is already light code for same function in Marlin or to replace existing code by new one.

This answer on facebook is weird (at best), and should happen here IMHO, but if it the rule, sorry I missed it.

I would suggest to update http://marlinfw.org/docs/development/contributing.html to mention questions about Marlin development rules ( if any could be ask ?), should be done on facebook and not on github ^_^

If this answer is for question I have submitted above (in github), I will do PR without asking any question, because it seems not appropriate, again sorry for doing that with this topic.

[vivian-ng]

@luc-github My reading of reply from @thinkyhead on FB is that, as long as the guidelines given in the Marlin docs about contributing are followed, people should feel free to submit their PRs. Because that is what open source development is about, everyone can contribute to make things better. Once the PR has been submitted, the main developers can review the code and work to get it in (or reject it), and if necessary, update the documentation about external libs. I mean, the extensible UI is an example of such an evolution.

So maybe just create the branch and send in the PR, and everyone can then follow up on the discussion through the PR to see how best to integrate the changes looking at the code.

[luc-github]

Well I am not mind reader sorry, so my reading of the situation is different - may be wrong, may be not 😸.
I was just trying to save everybody time to limit back and forth changes by going to the right direction.
Now I know it is not necessary.

[thinkyhead]

Good work, @luc-github, and an exemplary PR. It looks straightforward and I don't anticipate any issues, but now that it's merged I'm sure we'll find out!

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.