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

Remove ESP_CONSOLE_UART from sdkconfig for 666 S3 Display panels #9583

Merged
merged 3 commits into from
Sep 18, 2024

Conversation

RetiredWizard
Copy link

This fixes #9569 boot loop for the Hacktablet and Makerfabs 7" TFT ESP32-S3 666 Dotclock display boards.

It looks to me like in addition to the Hacktablet and Makerfabs TFT 7" boards, the Espressif ESP32S3_LCD_EV and possibly (I don't know if non-S3 boards are affected) the Lilygo twatch_2020_v3 may be affected by this issue. I don't have either of those devices to test with but could add the same sdkconfig changes for those boards if that makes sense?

@dhalbert dhalbert requested a review from jepler September 3, 2024 13:57
Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this! I have some questions though...

  • Didn't this configuration work once upon a time?
  • Does removing the sdk uart configuration have any downsides, like preventing visibility of early boot messages?
  • I found 4 boards with CONFIG_ESP_CONSOLE_UART=y in sdkconfig but this PR changes 2. Any reason to leave the other two unchanged?

@RetiredWizard
Copy link
Author

  • Didn't this configuration work once upon a time?

Yes it did, the boot loop didn't occur in 9.1.2/9.1.3.

  • Does removing the sdk uart configuration have any downsides, like preventing visibility of early boot messages?

On the two boards I have/tested, I was still able to connect to the REPL over the UART but you are right, the early boot messages are no longer visible 😢

  • I found 4 boards with CONFIG_ESP_CONSOLE_UART=y in sdkconfig but this PR changes 2. Any reason to leave the other two unchanged?

Since I didn't know or track down the reason for the change, I went back and forth about adding the other boards since I couldn't test them as well, but I certainly could add them to the PR.

@dhalbert
Copy link
Collaborator

dhalbert commented Sep 3, 2024

The early boot messages are no longer visible

I think this is fine, because this is just noise to most users anyway. I would prefer these be suppressed for ordinary use.

@RetiredWizard
Copy link
Author

RetiredWizard commented Sep 3, 2024

I would prefer these be suppressed for ordinary use.

Okay, but remember that at least on the boards I'm testing, their are two USB connectors, for ordinary use you would connect to the connector labeled USB which brings up the CIRCUITPY drive and shows up as /dev/ttyACM0, that connection doesn't show the early boot messages anyway. If you're connecting to the UART connector /dev/ttyUSB0 you don't get the CIRCUITPY drive and you're probably doing some debugging. That's the connection that I assume is affected by this PR.

EDIT Actually on further testing, the USB connector (ACM0, not affected by this PR) does show some of the early boot messages but (using tio) the connection disconnects almost immediately after starting the early boot log (cutting off most of the output) and then reconnects at which point only the code.py output is displayed.

@dhalbert
Copy link
Collaborator

dhalbert commented Sep 3, 2024

@RetiredWizard Ah, sorry, I misunderstood. Maybe I am seeing the boot messages on the non-full-USB boards.

@RetiredWizard
Copy link
Author

RetiredWizard commented Sep 3, 2024

@RetiredWizard Ah, sorry, I misunderstood. Maybe I am seeing the boot messages on the non-full-USB boards.

Now I'm not sure, I did some retesting and there is a smaller set of the boot messages on the normal port.... But I don't think that output is impacted by this PR.

@dhalbert
Copy link
Collaborator

I would merge this but I think it's worth seeing which other similar boards still have ESP_CONSOLE_UART.

@RetiredWizard
Copy link
Author

It looks to me like in addition to the Hacktablet and Makerfabs TFT 7" boards, the Espressif ESP32S3_LCD_EV and possibly (I don't know if non-S3 boards are affected) the Lilygo twatch_2020_v3 may be affected by this issue. I don't have either of those devices to test with but I'm happy to update the PR to include the same changes for those two boards as well.

I would feel better about changing the boards I can't test if I had an idea why this change is necessary for 9.2.x and not 9.1.2.

@RetiredWizard
Copy link
Author

I've been testing #9634 using main (9.2.0-alpha.2351-39-g1e36e05380-dirty on 2024-09-16; MakerFabs-ESP32-S3-Parallel-TFT-With-Touch-7inch with ESP32S3) as a base and haven't been seeing the boot looping that this was supposed to address. I'm not sure why but I'll do some more testing to see if this has been fixed by something since I was seeing the issue.

@RetiredWizard
Copy link
Author

Okay, I'm not seeing the issue anymore using Absolute Newest:

Adafruit CircuitPython 9.2.0-alpha.2351-37-g015a5cf922 on 2024-09-16; MakerFabs-ESP32-S3-Parallel-TFT-With-Touch-7inch with ESP32S3
Adafruit CircuitPython 9.2.0-alpha.2351-37-g015a5cf922 on 2024-09-16; ESP32-S3-DevKitC-1-N8R8-with-HACKTABLET with ESP32S3

@dhalbert
Copy link
Collaborator

@RetiredWizard Could you make the same change on espressif_esp32s3_lcd_ev? That board also specifies GPIO43 and GPIO44. However, the LILYGO watch board uses special UART pins, so I'd be inclined to leave that alone.

I would have made the same change myself but you have disallowed upstream maintainers from pushing to your repo.

@RetiredWizard
Copy link
Author

While I'm at it, is circuitpy a good LWIP hostname for the espressif_esp32s3_lcd_ev board?

# LWIP
#
CONFIG_LWIP_LOCAL_HOSTNAME="circuitpy"
# end of LWIP

@RetiredWizard
Copy link
Author

I just noticed that #9608 adds a new version 1.5 of the s3-lcd-ev board and in that PR Scott suggested along with all the lines this PR is removing from sdkconfig, the CONFIG_LWIP_LOCAL_HOSTNAME can be dropped as well.

Lots of boards have that configured, but I could go through and cut it from all the S3 boards as long as we're updating these three?

@dhalbert
Copy link
Collaborator

the CONFIG_LWIP_LOCAL_HOSTNAME can be dropped as well.

That is OK too, there are plenty of boards without that. It doesn't serve a lot of purpose.

@RetiredWizard
Copy link
Author

Okay, I'll put the removal of the LWIP hostnames from S3 boards in a new PR so it doesn't hold this one up.

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Thanks!

@dhalbert dhalbert merged commit dee71a9 into adafruit:main Sep 18, 2024
15 checks passed
@RetiredWizard RetiredWizard deleted the uartbootloop branch September 18, 2024 01:27
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.

boot loop on Hacktablet and Makerfabs matouch 7" S3 display boards
3 participants