Skip to content

Temporarily disable settable pystack for esp32s3 & esp32c3. #7689

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

Closed
wants to merge 9 commits into from

Conversation

bill88t
Copy link

@bill88t bill88t commented Mar 6, 2023

For more details please refer to #7643.
I'm pretty sure I didn't forget any boards.

This will be reverted once the proper fix is found.

@@ -53,6 +53,12 @@
#define MICROPY_NLR_SETJMP (1)
#define CIRCUITPY_DEFAULT_STACK_SIZE 0x6000

// Temporarily disable settable pystack
#if defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32S3)
#undef CIRCUITPY_SETTABLE_PYSTACK
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to undef this?

Copy link
Author

@bill88t bill88t Mar 9, 2023

Choose a reason for hiding this comment

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

It's already defined up in py/circuitpy_mpconfig.h. Without the undef it errors out.

Copy link
Collaborator

@dhalbert dhalbert Mar 9, 2023

Choose a reason for hiding this comment

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

We try not to test on defined/undefined. We use #if SOME_FLAG to test, and throw warnings if SOME_FLAG is not defined.

Two things to do:

  1. in circuitpy_mpconfig.h, do:
#ifndef CIRCUITPY_SETTABLE_PYSTACK
#define CIRCUITPY_SETTABLE_PYSTACK   (1)
#endif

and move it down into the conditionalized settings in that file, alphabetically.

  1. Here in mpconfigport.h, do:
#if defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32S3)
#define CIRCUITPY_SETTABLE_PYSTACK     (0)
#endif

mpconfigport.h is included before circuitpy_mpconfig.h, so circuitpy_mpconfig.h will pick up the turn-off setting to 0 and not set it to 1.

@@ -53,6 +53,12 @@
#define MICROPY_NLR_SETJMP (1)
#define CIRCUITPY_DEFAULT_STACK_SIZE 0x6000

// Temporarily disable settable pystack
#if defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32S3)
#undef CIRCUITPY_SETTABLE_PYSTACK
Copy link
Collaborator

@dhalbert dhalbert Mar 9, 2023

Choose a reason for hiding this comment

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

We try not to test on defined/undefined. We use #if SOME_FLAG to test, and throw warnings if SOME_FLAG is not defined.

Two things to do:

  1. in circuitpy_mpconfig.h, do:
#ifndef CIRCUITPY_SETTABLE_PYSTACK
#define CIRCUITPY_SETTABLE_PYSTACK   (1)
#endif

and move it down into the conditionalized settings in that file, alphabetically.

  1. Here in mpconfigport.h, do:
#if defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32S3)
#define CIRCUITPY_SETTABLE_PYSTACK     (0)
#endif

mpconfigport.h is included before circuitpy_mpconfig.h, so circuitpy_mpconfig.h will pick up the turn-off setting to 0 and not set it to 1.

@bill88t
Copy link
Author

bill88t commented Mar 9, 2023

circuitpy_mpconfig.h is still applied before the port's mpconfigport.h, hence this change still results in error.
I did make sure to move it down (passed #include "mpconfigboard.h"). But it still fails.

./mpconfigport.h:58: error: "CIRCUITPY_SETTABLE_PYSTACK" redefined [-Werror]
 #define CIRCUITPY_SETTABLE_PYSTACK (0)
 
In file included from ./mpconfigport.h:38,
                 from ../../py/mpconfig.h:45,
                 from ../../py/gc.h:31,
                 from ../../py/qstr.c:31:
../../py/circuitpy_mpconfig.h:544: note: this is the location of the previous definition
 #define CIRCUITPY_SETTABLE_PYSTACK (1)

@dhalbert
Copy link
Collaborator

dhalbert commented Mar 9, 2023

mpconfigport.h is included before circuitpy_mpconfig.h, so circuitpy_mpconfig.h will pick up the turn-off setting to 0 and not set it to 1.

Sorry, this is wrong. I think it will work if you put this:

#if defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32S3)
#define CIRCUITPY_SETTABLE_PYSTACK     (0)
#endif

above the #include "circuitpy_mpconfig.h" in mpconfigport.h. Try it locally first.

@bill88t
Copy link
Author

bill88t commented Mar 10, 2023

Since S2 is also affected, I suggest it's best to just disable it for all esp for now.

@bill88t
Copy link
Author

bill88t commented Mar 16, 2023

Well, since it's an optimisation bug it will be pretty soon fixed. No need for this to go through.

@bill88t bill88t closed this Mar 16, 2023
@bill88t bill88t deleted the temp_disable_settable branch March 16, 2023 16:17
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.

4 participants