search

luc-github / ESP3DLib

ESP3D library for Marlin and ESP32 boards
GNU General Public License v3.0
97 stars 32 forks source link

Bugfix for new ESP IDF version (Marlin FW toolchain update) #51

Closed quiret closed 1 year ago

quiret commented 1 year ago

There is a bug in the codebase due to a change in Espressif toolchains. Several projects had to change their code due to this breaking change in object types. The culprit here is the global variable ESP_IF_WIFI_STA.

Example of another affected project: https://github.com/bdring/Grbl_Esp32/pull/782

This is required to get Marlin FW updated to the latest ESP32 toolchains. See here: https://github.com/MarlinFirmware/Marlin/pull/25327

Thank you for your time! Great project.

Hit me up when this is merged so I can replace the Marlin dependency back to your main repo.

luc-github commented 1 year ago

did you check https://github.com/luc-github/ESP3DLib/tree/3.0 ? this is what it is

luc-github commented 1 year ago

and change I did here : https://github.com/luc-github/Marlin/tree/ESP3D-V3-2.1.x

luc-github commented 1 year ago

using idf 5.0 just when out is not good idea - you better wait for idf 5.1 that will fix all issues meet in 5.0 and already do

what benefit of 5.0 vs stable 4.4.1 ? the one I use in testing fork where I tested several boards

https://github.com/luc-github/ESP3DLib/issues/39#issuecomment-1100863789

FYI https://github.com/bdring/Grbl_Esp32 is not more supported by dev

luc-github commented 1 year ago

Did you heavily tested your changes ?

quiret commented 1 year ago

Did you heavily tested your changes ?

Let me talk about just this PR. Usually I heavily test all of my work. I admit that this tiny change was merely a compatibility update! Compilation has worked just fine. I hope that you can follow the discussions at the link that I provided where other people did the exact same change and be convinced that this is a good change.

quiret commented 1 year ago

using idf 5.0 just when out is not good idea - you better wait for idf 5.1 that will fix all issues meet in 5.0 and already do

what benefit of 5.0 vs stable 4.4.1 ? the one I use in testing fork where I tested several boards

#39 (comment)

Not sure how you gather that I am using ESP-IDF 5.0. Marlin FW is using this guy over here:

https://registry.platformio.org/tools/platformio/framework-arduinoespressif32

Does your argument also apply to the Arduino Framework from Espressif? It is at version 2.0.6 now so it seems to be matured, counter to that 5.0 argument.

FYI https://github.com/bdring/Grbl_Esp32 is not more supported by dev

Oh I see! That's unfortunate. But you seem to be a really efficient dev so I don't mind :)

luc-github commented 1 year ago

I am not talking about the change you did in esp3dlib I am talking about huge impact of changing idf

it bring many breaking change and it is not because compilation pass, it actually behave properly.

An rely on fact platformio release it is stable is wrong especially on breaking changes

quiret commented 1 year ago

and change I did here : https://github.com/luc-github/Marlin/tree/ESP3D-V3-2.1.x

Your changes seem to overlap with changes that I want! How come your stuff has not made it's way into official Marlin yet?

quiret commented 1 year ago

I am not talking about the change you did in esp3dlib I am talking about huge impact of changing idf

it bring many breaking change and it is not because compolation change it actually behave properly.

An rely on fact platformio release it is stable is wrong especially on breaking changes

I have tested the proper functionality of Marlin on the configuration Espressif 6.0.0 with latest GCC 11.2.0 toolchain. Peripherals like SPI touchscreen with SD card are working. Not sure why you are questioning my testing?

There are things I wish to test further, like the custom MKS modules. Still need to work on other great things on my pipeline so please bear with me. Just know that I do test heavy changes like that VERY PROPERLY.

luc-github commented 1 year ago

man esp32 platformio 6.0.0 use idf 5.0...

my fork is under test that is why it is not pushed, because if it not working, support will give me overload, so I test it before pushing

quiret commented 1 year ago

did you check https://github.com/luc-github/ESP3DLib/tree/3.0 ? this is what it is

Is this a recommendation from you to try the 3.0.0 version of ESP3DLib? If that is true then I can put that on my tasklist. Of course, we are talking about the importance of testing. Rest assured, I promise to test it properly. May have to get additional hardware so might not happen soon.

luc-github commented 1 year ago

I am not questionning your testing, I am asking because I see lot of people pushing change without testing and in your PR you told you only tested 1 esp32 board

luc-github commented 1 year ago

your change not only affect tool chain but full esp32 core and related libraries

my concern is not the toolchain , but core itself

quiret commented 1 year ago

I am not questionning your testing, I am asking because I see lot of people pushing change without testing and in your PR you told you only tested 1 esp32 board

That is why I have only enabled the update for one board. You see I did NOT update the toolchain for every ESP32 board out there. I know that Espressif have fragile toolchains so other people should first test and then enable other boards with latest toolchains.

quiret commented 1 year ago

your change not only affect tool chain but full esp32 core and related libraries

my concern is not the toolchain , but core itself

The changes I do are great and according to official documentation by Espressif. Thus the great update will benefit every ESP32 board. I am well aware about different boards and their requirements so you can call me a very careful person.

luc-github commented 1 year ago

again my issue is not the tool chain but idf 5.0 core change notified as breaking changes. and not doing long wifi test is a risk per my experience e.g I updated ssdp library due to random issues not seen after 5 min tests but several hours. I just share my experience with esp dev and core issues, you do what you want with what I said.

But I do understand that you update only one board so if I merge your PR, what about the others boards using old core ? compilation will failed ?

quiret commented 1 year ago

again my issue is not the tool chain but idf 5.0 core change notified as breaking changes. and not doing long wifi test is a risk per my experience e.g I updated ssdp library due to random issues not seen after 5 min tests but several hours. I just share my experience with esp dev and core issues, you do what you want with what I said.

But I do understand that you update only one board so if I merge your PR, what about the others boards using old core ? compilation will failed ?

I am not sure right now, but there seems to be a WIFI_IF_STA back to ESP-IDF 4.1.4

https://docs.espressif.com/projects/esp-idf/en/v4.1.4/api-reference/network/esp_wifi.html?highlight=wifi_if_sta#c.WIFI_IF_STA

Maybe people just picked up using the wrong thing from the very beginning? Like an error that spread through the community? I will test it I guess.

About the WIFI testing scenario I guess you make an interesting point. And I acknowledge your point if you can point me to a real breaking change... Like any evidence that ESP IDF 5.0.0 is broken? I don't want to hinder progress just because of emotions.

luc-github commented 1 year ago

current Marlin use platformio 2.1.0 that use idf 4.0.x

the breaking change are notified in release notes compare to 4.4, but have more compare 4.0.x I am sure , this is not emotions but releases notes, you should read them.

quiret commented 1 year ago

current Marlin use platformio 2.1.0 that use idf 4.0.x

the breaking change are notified in release notes compare to 4.4, but have more compare 4.0.x I am sure , this is not emotions but releases notes, you should read them.

https://github.com/espressif/esp-idf/releases

You mean this list, right? Yea, this looks interesting. I suppose I should give the WIFI thing an intensive test! Don't worry ;)

luc-github commented 1 year ago

hmm you do changes without noticing the impact on core, you did not read the release notes, and you did not tested the esp3lib pr on boards not impacted by your pr in Marlin.

sorry, yes I am worry ^_^

but your PR is in Draft so it give you time to test and get feedback.

Good luck

quiret commented 1 year ago

hmm you do changes without noticing the impact on core, you did not read the release notes, and you did not tested the esp3lib pr on boards not impacted by your pr in Marlin.

sorry, yes I am worry ^_^

but your PR is in Draft so it give you time to test and get feedback.

Good luck

Thanks! Sometimes radical changes need a big push! I appreciate your help because you seem experienced.

Also I do notice the impact on core. You are putting a little too much prejudice here, friend ^^

If I give you clear testing instructions could you perform them on ESP32 boards that you own?

luc-github commented 1 year ago

Also I do notice the impact on core. You are putting a little too much prejudice here, friend ^^

I just reacted to you comment which showed you did not noticed what the impact :

Not sure how you gather that I am using ESP-IDF 5.0. Marlin FW is using this guy over here:

and to be honest I am not really worry because I won't investigate any issue in Esp3dlib V1.0 because I am already busy with Esp3dlib v3,.

As mentioned in Marlin PR, this will affect a lot of users but Marlin is well known to realease untested solutions, with each release being a lotery about what is work and what is broken because untested due to contributors having too much self-confidence and do not know Murphy Law.

So no worrry I won't block anything if PR is validated on Marlin side this one will be merged right away, because issues will not drop on me .

quiret commented 1 year ago

So no worrry I won't block anything if PR is validated on Marlin side this one will be merged right away, because issues will not drop on me .

Just trust me, ok! You talking to me is already an example of how good contribution to Marlin works: talking to the people making the software! I am an example of a good contributor so don't put me into the same bag as those you mentioned previously. I promise you to test the thing properly, ok? Like for real.

Also my update strategy is conservative and careful. I expect to be a lucky guy.

You sound like you want to give up on updating Marlin altogether... Like you think that bringing new things to Marlin will break it. Please don't think that way. I ask you to rethink your vision and help make Marlin the best firmware there is. That includes bringing new toolchains to it.

luc-github commented 1 year ago

well I have bad experience with Marlin dev and Marlin contributors, so I am not following Marlin anymore / neither be part of Marlin Discord and do not use it more than I need.

Also sentence like :

Just trust me, ok!

Won't work with me, this sentence is usually used when there are no argument behind, I do not say you are in same bag or self confident ,I even do not give any judgment on you, I just see facts, including above ones, I gave you feedback, and as I wrote I won't be a blocking point, so I do not need to Trust you 😄

quiret commented 1 year ago

Won't work with me (..)

Not a problem! Your software is already a good contribution to Marlin overall. I will be using the software as Marlin sees fit to create the best firmware version as of late. I am saddened about your terrible experience with Marlin and wish you the best in life. Don't let bad experience drag you down! Will be heading to other places I have to be because the update I plan is not just limitted to toolchains. If you follow me you can find new good ways to develop your own software.

luc-github commented 1 year ago

If you follow me you can find new good ways to develop your own software.

haha, That's really having high self-esteem - I wish you the best and just wait for the merging

luc-github commented 1 year ago

Man you are starting to mix several PR in one please split by purpose because it will quickly be unreadable for review Many of your changes are not related to ESP IDF version and are arguable

luc-github commented 1 year ago

latest esp32 core / platformio also bring issue with current TMC2009 drivers - board go in dead loop if using uart mode and TMC2009 - not related to ESP3D as original PR (https://github.com/MarlinFirmware/Marlin/pull/25327) is closed - I close this one too for consistency