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

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

Closed
wants to merge 4 commits into from

Conversation

quiret
Copy link

@quiret quiret commented Feb 2, 2023

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: bdring/Grbl_Esp32#782

This is required to get Marlin FW updated to the latest ESP32 toolchains. See here: MarlinFirmware/Marlin#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.

quiret added a commit to quiret/Marlin that referenced this pull request Feb 2, 2023
…32 to fix compilation issues on the latest Espressif toolchains

When the PR luc-github/ESP3DLib#51 has been merged then please change it back!

- (temporarily) updated the "ESP Async WebServer" dependency for MKS TinyBee / ESP32 to fix compilation issues on the latest Espressif toolchains

Please watch PR me-no-dev/ESPAsyncWebServer#1142 and change back once merged.

- fixed another ESP32 bug due to toolchain update
@luc-github
Copy link
Owner

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

@luc-github
Copy link
Owner

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

@luc-github
Copy link
Owner

luc-github commented Feb 2, 2023

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)

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

@luc-github
Copy link
Owner

Did you heavily tested your changes ?

@quiret
Copy link
Author

quiret commented Feb 3, 2023

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
Copy link
Author

quiret commented Feb 3, 2023

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
Copy link
Owner

luc-github commented Feb 3, 2023

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
Copy link
Author

quiret commented Feb 3, 2023

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
Copy link
Author

quiret commented Feb 3, 2023

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
Copy link
Owner

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
Copy link
Author

quiret commented Feb 3, 2023

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
Copy link
Owner

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
Copy link
Owner

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

my concern is not the toolchain , but core itself

@quiret
Copy link
Author

quiret commented Feb 3, 2023

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
Copy link
Author

quiret commented Feb 3, 2023

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
Copy link
Owner

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
Copy link
Author

quiret commented Feb 3, 2023

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
Copy link
Owner

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
Copy link
Author

quiret commented Feb 3, 2023

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
Copy link
Owner

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
Copy link
Author

quiret commented Feb 3, 2023

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
Copy link
Owner

luc-github commented Feb 3, 2023

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
Copy link
Author

quiret commented Feb 3, 2023

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
Copy link
Owner

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
Copy link
Author

quiret commented Feb 3, 2023

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
Copy link
Owner

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

…re using preprocessor definitions shared by the Marlin build environment (WIFI_HOSTNAME, DEFAULT_WIFI_HOSTNAME; ordered in priority)
…thing/webserver really needs a proper template system, which I already have in my SDK...)
@luc-github
Copy link
Owner

luc-github commented Feb 15, 2023

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

Copy link
Owner

@luc-github luc-github left a comment

Choose a reason for hiding this comment

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

  • updated the location of the (latest) Marlin firmware (oh boy, this …
    …thing/webserver really needs a proper template system, which I already have in my SDK...)

Yes this has been done long time ago ...
The code is far away from being perfect and there are typo - glitch everywhere
But such comment is rude without actually checking the V3 code : https://github.com/luc-github/ESP3DLib/blob/3.0/embedded/src/menu.js

This is because of such attitude I do not want to be part of Marlin devt

Copy link
Owner

@luc-github luc-github left a comment

Choose a reason for hiding this comment

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

split pr or justify

Copy link
Owner

@luc-github luc-github left a comment

Choose a reason for hiding this comment

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

spiit pr

Copy link
Owner

@luc-github luc-github left a comment

Choose a reason for hiding this comment

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

split pr

@luc-github
Copy link
Owner

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 (MarlinFirmware/Marlin#25327) is closed - I close this one too for consistency

@luc-github luc-github closed this Mar 24, 2023
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.

2 participants