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

PSRAM pins in ESP32 are now reserved only if needed #2022

Merged
merged 2 commits into from
Sep 6, 2021

Conversation

Ellerbach
Copy link
Member

@Ellerbach Ellerbach commented Aug 30, 2021

Description

Removing reserved pins setup on ESP32 for SPI flash in the GPIO controller.

Rational: if SPI Flash is used, then some of the pins will be used by SPI anyway. Not all the pins will be used all the time. The design of some of the ESP32 board (like M5 Stick) uses pins 9 and 10 as they are not used in SPI Flash while the board has some. And those 2 pins are used for IR and Red led.

Motivation and Context

How Has This Been Tested?

Screenshots

Types of changes

  • Improvement (non-breaking change that improves a feature, code or algorithm)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Config and build (change in the configuration and build system, has no impact on code or features)
  • Dependencies (update dependencies and changes associated, has no impact on code or features)
  • Unit Tests (work on Unit Tests, has no impact on code or features)
  • Documentation (changes or updates in the documentation, has no impact on code or features)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link
Member

@josesimoes josesimoes left a comment

Choose a reason for hiding this comment

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

I would try to make this a bit "smarter" and fool proof...
Meaning that the pin reservation should be there when the SPI flash it's present.

@Ellerbach
Copy link
Member Author

Meaning that the pin reservation should be there when the SPI flash it's present.

I honestly don't see a point of reserving pins. If they are used by SPI flash, you won't anyway be able to open them, you'll get an error. So reserving pins that anyway will return an error if you want to open them doesn't feel logical.

@josesimoes
Copy link
Member

josesimoes commented Sep 2, 2021

Meaning that the pin reservation should be there when the SPI flash it's present.

I honestly don't see a point of reserving pins. If they are used by SPI flash, you won't anyway be able to open them, you'll get an error. So reserving pins that anyway will return an error if you want to open them doesn't feel logical.

🤔 I don't think that's the case... you can reassign the GPIO pins used by SPI flash without getting any exception. The result is that, inadvertently, a developer can screw the execution and make the CLR unusable. Having this check layer was one of the motivations for adding all this CPU pins layer code.

@AdrianSoundy your input here, please.

@AdrianSoundy
Copy link
Member

AdrianSoundy commented Sep 2, 2021

Yes the problem is you don't get an error when pins are used that shouldn't be used. The reserved pins was to make sure that doesn't happen and developer gets a clear message the pin was used elsewhere.
For this case we just need to remove reserved pins for the flash pins if there is no PSRAM.

Could set a flag in memory.cpp or use :-
size_t spiramMaxSize = heap_caps_get_largest_free_block(MALLOC_CAP_8BIT | MALLOC_CAP_32BIT | MALLOC_CAP_SPIRAM);
To test if there is any psram memory available. There may also be an existing flag from bootloader code that can be used.

@josesimoes @Ellerbach

@josesimoes
Copy link
Member

Precisely! That check should be all it takes to reserve those pins only when they are being used by PSRAM. 👍🏻

@Ellerbach
Copy link
Member Author

Precisely! That check should be all it takes to reserve those pins only when they are being used by PSRAM. 👍🏻

I would argue that you always have the board pin availability. So as a developer, you can only pins that are physically accessible. If they are used by the psram, they are not physically accessible, so you 1. have no reason to use them, 2. if ever you want to use them, when trying to open them, you'll get an error.

So I really don't think that reserving them is less confusing than not reserving them as anyway, you'll get an error at the end of the day.

@Ellerbach
Copy link
Member Author

So I think I finally got it. The detection of PSRAM is done at boot time, before this code is executed. So at this stage, it's about understanding if there is some. And if there is some reserve the pins otherwise, just skip!

@nfbot
Copy link
Member

nfbot commented Sep 2, 2021

@Ellerbach there are issues with the code style on the source files.
A PR was submitted with the code style fixes. Please click #2023, review the changes if you want and merge it.

Make sure to follow the project code style. Check the details here on how it works and the tools required to help you with that.

@josesimoes
Copy link
Member

@Ellerbach this is building OK now. Care to check if it's working on your test app?

@Ellerbach
Copy link
Member Author

@Ellerbach this is building OK now. Care to check if it's working on your test app?

any already built nanoCLR I can just download and test? (otherwise, will pull and build locally)

@josesimoes
Copy link
Member

@Ellerbach this is building OK now. Care to check if it's working on your test app?

any already built nanoCLR I can just download and test? (otherwise, will pull and build locally)

Sure! Just follow the URL to Azure Pipeline build and grab the bin from there. (nanoFramework it's developer friendly) 😉

@Ellerbach
Copy link
Member Author

Works like a charm on en ESP32 PICO (M5 Stick) blinking the led on GPIO 10. Congratulations!

@josesimoes josesimoes changed the title Removing reserved pins PSRAM pins in ESP32 are now reserved only if needed Sep 6, 2021
@josesimoes josesimoes added Series: ESP32 Everything related specifically with ESP32 series targets Type: enhancement and removed Type: bug labels Sep 6, 2021
@josesimoes josesimoes merged commit 338b4a6 into develop Sep 6, 2021
@josesimoes josesimoes deleted the fix-esp32-reserved-pins branch September 6, 2021 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Series: ESP32 Everything related specifically with ESP32 series targets Type: enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ESP32 Pin 6 to 11 not released when SPI flash not present
4 participants