Skip to content

Conversation

@lfelten
Copy link
Contributor

@lfelten lfelten commented Oct 1, 2024

enable support for the SDHC controller to use the micro SD card slot

Copy link
Contributor

@danieldegrasse danieldegrasse left a comment

Choose a reason for hiding this comment

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

Can you add the sdhc tag to the list of features this board supports so the SDHC tests run on it?

Also, we should update the documentation page for this board to indicate SDHC support

@lfelten lfelten force-pushed the lilygo_ttgo_lora32_sdhc branch from c4568d0 to 91b86a0 Compare October 3, 2024 20:07
@lfelten
Copy link
Contributor Author

lfelten commented Oct 3, 2024

Thanks for you feedback!

Can you add the sdhc tag to the list of features this board supports so the SDHC tests run on it?
added

Also, we should update the documentation page for this board to indicate SDHC support
It mentioned SD before, I updated it to SDHC

Copy link
Contributor

@sylvioalves sylvioalves left a comment

Choose a reason for hiding this comment

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

Now, add ' sdhc' to the ignore list in ttgo_lora32_esp32_appcpu.yaml.

@lfelten lfelten force-pushed the lilygo_ttgo_lora32_sdhc branch from 91b86a0 to 0d2ad00 Compare October 4, 2024 13:38
@lfelten
Copy link
Contributor Author

lfelten commented Oct 4, 2024

Now, add ' sdhc' to the ignore list in ttgo_lora32_esp32_appcpu.yaml.

Added it.
I also added CONFIG_ESP32_USE_UNSUPPORTED_REVISION=y in the procpu defconfig, which is required since I rebased to main

raffarost
raffarost previously approved these changes Oct 4, 2024
@lfelten
Copy link
Contributor Author

lfelten commented Oct 5, 2024

This fails during linking, undefined symbols are ctime_r and asctime_r.
I'll rebase to main.

@lfelten lfelten force-pushed the lilygo_ttgo_lora32_sdhc branch from 0d2ad00 to ee92484 Compare October 5, 2024 06:18
@sylvioalves
Copy link
Contributor

This fails during linking, undefined symbols are ctime_r and asctime_r. I'll rebase to main.

Indeed. Fix in #79455.

@sylvioalves
Copy link
Contributor

@lfelten please rebase.

@lfelten lfelten force-pushed the lilygo_ttgo_lora32_sdhc branch from ee92484 to 1e38289 Compare October 9, 2024 14:00
danieldegrasse
danieldegrasse previously approved these changes Oct 15, 2024
@sylvioalves
Copy link
Contributor

@lfelten Is this completed?

@lfelten lfelten dismissed stale reviews from danieldegrasse and raffarost via 0af3237 November 16, 2024 05:05
@lfelten lfelten force-pushed the lilygo_ttgo_lora32_sdhc branch from 1e38289 to 0af3237 Compare November 16, 2024 05:05
Copy link
Contributor

@kartben kartben left a comment

Choose a reason for hiding this comment

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

A nit and an actual request for change. Thanks for rebasing, too!

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment on why this is needed (if this is needed?). This way it's not only clearer to the user but also helps knowing when we might be able to remove it in the future

Copy link
Contributor

Choose a reason for hiding this comment

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

At a minimum, makes this a different (and first) commit, similar to 956edd1 since my understanding is that it is an unrelated (but needed) change

Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw i was not suggesting to add a comment to the git commit (although it's not a bad idea to do it too :) ) but rather right in the defconfig file so that people understand where this is coming from. BTW I am not sure this really belongs to the defconfig as this is not something that should be modifiable, no? Shouldn't it be selected in the Kconfig file instead? cc @sylvioalves

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It says "unsupported revision" - at least my board uses this version of the chip.
Maybe newer boards (it's still in production) use a newer version.
I'm not sure this setting has any impact when selected for a chip revision > 1

I added a comment and also trimmed the commit log line length.

Copy link
Contributor

@sylvioalves sylvioalves Nov 20, 2024

Choose a reason for hiding this comment

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

config ESP32_USE_UNSUPPORTED_REVISION
	bool "Use unsupported ESP32 revision (Rev 0/1)"
	help
	  ESP32 SoC has multiple revisions, some of which are not supported by the current
	  implementation, as such as REV0 and REV1. In case those revisions are required,
	  set this option to enable support for them. Note that this is not recommended and
	  may lead to unexpected behavior.

rev0 and rev1 ESP32 SocS (old versions) have rom bugs that we haven't and will not address. So if the board uses such old chip revision, this config must be set.

@lfelten lfelten force-pushed the lilygo_ttgo_lora32_sdhc branch 2 times, most recently from 91d20f5 to 3ce99ab Compare November 20, 2024 12:52
device tree:
enable support for the SDHC controller to use the micro SD card slot

documentation:
- added instructions for SD card and OLED samples
- added links to code samples

defconfig:
added CONFIG_ESP32_USE_UNSUPPORTED_REVISION=y to
ttgo_lora32_esp32_procpu_defconfig

The chip on the board is a ESP32 chip revision 1.
The board will not boot, it displays the following warning at boot:

I (35) boot: chip revision: v1.0
E (38) boot: You are using ESP32 chip revision (1) that is unsupported.
While it may work, it could cause unexpected behavior or issues.
E (50) boot: Proceeding with this ESP32 chip revision is not recommended
unless you fully understand the potential risk and limitations.
E (62) boot: If you choose to continue, please enable the
'CONFIG_ESP32_USE_UNSUPPORTED_REVISION' in your project configuration.
E (73) boot: HW init failed, aborting

In order to prevent a boot loop, CONFIG_ESP32_USE_UNSUPPORTED_REVISION=y
was added to the defconfig.

Signed-off-by: Lothar Felten <[email protected]>
@lfelten lfelten force-pushed the lilygo_ttgo_lora32_sdhc branch from 3ce99ab to 895680a Compare November 20, 2024 14:06
@aescolar aescolar merged commit e7c3434 into zephyrproject-rtos:main Nov 21, 2024
17 checks passed
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.

6 participants