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

Fix ESP32 I2S initialization order #21019

Merged

Conversation

simon-jouet
Copy link
Contributor

@simon-jouet simon-jouet commented Feb 7, 2021

Description

The current order of initialization for the ESP32 isn't quite right because the arduino-esp32 library is buggy. Even if you redefine the Serial ports to be on a different port through the GPIO matrix when the Serial is initialized the native pins (pre matrix redirect) will be repurposed. In this case if you have the I2S peripheral initialized on the native pins of either the Serial1 or Serial2 the I2S doesn't work.

The changes are quite simple, just init the I2S peripheral after the Serial ports are initialized.

It also prevents the I2S peripheral from being initialized if the I2S_STEPPER_STREAM isn't enabled as there isn't much point.

Requirements

That's for an ESP32 board with the I2S_STEPPER_STREAM enabled, and to see any difference from the current behaviour you need to have the I2S stream on a port used by a serial port. In my case with the ESP32Controller R2 I have the following which conflicts with Serial2

#define I2S_STEPPER_STREAM
#define I2S_WS                                25
#define I2S_BCK                               26
#define I2S_DATA                              27

Benefits

You can still use the serial ports and the i2s stream at the same time. In my case it was to get Serial2 working as well a Serial1 to have up to 8 TMC2209 through UART.

@simon-jouet
Copy link
Contributor Author

Hey @felixstorm I saw you did some refactor some time ago on the HAL init for the ESP32 board, any particular reasons for the i2s stream to remain in hal_init instead of moving it to hal_init_board()?

@felixstorm
Copy link
Contributor

Hey @simon-jouet Nice to see that you're working on Marlin for ESP32 again. I still use my old simple single-layer THT self-designed ESP32 mainboard with my Ender 3 for day to day work, but since I do not really use WiFi (last time I tried the throughput was not really as fast as switching SD card to the computer and back by hand) and also since I had not found any time myself to continue working on it, I had almost been considering switching boards to some other standard design again fearing that upgrading to a more recent version of Marlin with the ESP32 might require a lot of adoption work again. Therefore it's very nice for me to see that you also seem to be still using it 😃

Looking back at the commit I guess you're referring to (804609c), I think that I primarily wanted to move the call to wifi_init() to a point in time where I was already able to print status messages to Serial, and therefore started to implement HAL_init_board for it. And I think that I just left the call to i2s_init() in HAL_init because it had been there before and I did not see any reason to change this, but not moving it to HAL_init_board had most likely not been intentional at all.
So to answer your question: No, I do not see any particular reasons for the i2s stream to remain in hal_init (and you have anyway been far deeper into the ESP32 details than I will ever be), so if it works for you then it should certainly be fine to move it.

@simon-jouet
Copy link
Contributor Author

simon-jouet commented Feb 7, 2021

Thanks @felixstorm! Good to know that I didn't miss something with the HAL :).

I'm looking at building a toolchanger for my hypercube at the moment and had to build a bit on top of the current hardware :). I've hacked 2 boards together and using the I2S stepper stream I can control 10 steppers which is nice (although with 4 TMC2209 per UART i'm limited to 8 tmc2209). I really do need to put 2 more boards together for you and @thinkyhead, I think I might still have enough leftovers from last year to make them.

Similarly to you time is quite limited so it's been very slow, but I've been running 3 printers with the ESP32 R2 board (i2s) and luc's esp3d and it's been working very well. I've bumped one of my printer to the latest bugfix firmware last weekend without any major problem, some changes have been done with the serial works which is making esp3d a bit funky but it's still working reasonably well. Regarding the SD card yeah it's very very slow, I need to spend some time to look into it but it's very convenient, I have the printers somewhere else in the room and with ESP3D I can just deal with all of it just from my PC (I even had a PoC to have an octoprint compatible endpoint to directly print from within cura/prusaslicer -- but I haven't tried that in a while)

I'm waiting for the ESP32-S3 to be released in the coming months (hopefully) to make a new board with some fixes for the R2, and the extra 10 GPIO of the S3 will be a great addition (hopefully with a better ADC too). I'm considering making one with directly 10(8?) slots for the drivers, it doesn't cost much more and it will avoid a lot of fiddling to add more drivers...

@thinkyhead thinkyhead added A: ESP32 T: HAL & APIs Topic related to the HAL and internal APIs. PR: Workaround labels Feb 7, 2021
@thinkyhead thinkyhead merged commit af4e8b1 into MarlinFirmware:bugfix-2.0.x Feb 7, 2021
@felixstorm
Copy link
Contributor

@simon-jouet A toolchanger definitely sounds interesting and thanks for keeping me in mind about building another board, but I am completely fine with what I have. The design is simple, but it has all the functionality of my stock Ender 3 board plus TMC2008s and BLTouch support and it's been working flawlessly for almost two years now, so there is no need for any change at the moment: https://github.com/felixstorm/Creality_Ender_3_ESP32_Board/blob/master/Creality_Ender_3_ESP32_Board.jpg

And yes, the ESP32-S3 definitely sounds promising!

@vivian-ng
Copy link
Contributor

Yes, the ESP32-S3 sounds interesting with more GPIO pins and USB support. With more pins, it does remove the need for I2S. Hopefully I can find time to work on a board when the ESP32-S3 is released.

rmpel added a commit to rmpel/Marlin that referenced this pull request Feb 11, 2021
* bugfix-2.0.x: (177 commits)
  [cron] Bump distribution date (2021-02-11)
  chmod and paths
  [cron] Bump distribution date (2021-02-10)
  Reheat bed first
  Ender 3 V2 DWIN cleanup (MarlinFirmware#21026)
  Update M808 comment
  MAX Thermocouples rework (MarlinFirmware#20447)
  [cron] Bump distribution date (2021-02-09)
  Serial refactor. Default 8-bit ECHO to int, not char (MarlinFirmware#20985)
  Fix STM32F1 emergency parser (MarlinFirmware#21011)
  Allow SERVO0_PIN override on Creality Melzi (MarlinFirmware#21007)
  Fix animated boot screen
  Fix: Unsupported use of %f in printf (MarlinFirmware#21001)
  Fix mini12864 v2.1 + PSU control + NeoPixel backlight (MarlinFirmware#21021)
  [cron] Bump distribution date (2021-02-08)
  Fix LVGL "more" menu user items (MarlinFirmware#21004)
  Fix TEMP_0_TR_ENABLE, rename temp conditions (MarlinFirmware#21016)
  Fix ESP32 I2S init placement (MarlinFirmware#21019)
  Improve RPi host kernel panic mitigation
  Melzi, comments cleanup
  ...
rmpel added a commit to rmpel/Marlin that referenced this pull request Feb 13, 2021
…rmpel/Marlin into rmpel/bugfix-2.0.x/ender-3-skr-14-turbo

* 'rmpel/bugfix-2.0.x/ender-3-skr-14-turbo' of github.com:rmpel/Marlin: (177 commits)
  [cron] Bump distribution date (2021-02-11)
  chmod and paths
  [cron] Bump distribution date (2021-02-10)
  Reheat bed first
  Ender 3 V2 DWIN cleanup (MarlinFirmware#21026)
  Update M808 comment
  MAX Thermocouples rework (MarlinFirmware#20447)
  [cron] Bump distribution date (2021-02-09)
  Serial refactor. Default 8-bit ECHO to int, not char (MarlinFirmware#20985)
  Fix STM32F1 emergency parser (MarlinFirmware#21011)
  Allow SERVO0_PIN override on Creality Melzi (MarlinFirmware#21007)
  Fix animated boot screen
  Fix: Unsupported use of %f in printf (MarlinFirmware#21001)
  Fix mini12864 v2.1 + PSU control + NeoPixel backlight (MarlinFirmware#21021)
  [cron] Bump distribution date (2021-02-08)
  Fix LVGL "more" menu user items (MarlinFirmware#21004)
  Fix TEMP_0_TR_ENABLE, rename temp conditions (MarlinFirmware#21016)
  Fix ESP32 I2S init placement (MarlinFirmware#21019)
  Improve RPi host kernel panic mitigation
  Melzi, comments cleanup
  ...
rmpel added a commit to rmpel/Marlin that referenced this pull request Feb 13, 2021
* bugfix-2.0.x: (177 commits)
  [cron] Bump distribution date (2021-02-11)
  chmod and paths
  [cron] Bump distribution date (2021-02-10)
  Reheat bed first
  Ender 3 V2 DWIN cleanup (MarlinFirmware#21026)
  Update M808 comment
  MAX Thermocouples rework (MarlinFirmware#20447)
  [cron] Bump distribution date (2021-02-09)
  Serial refactor. Default 8-bit ECHO to int, not char (MarlinFirmware#20985)
  Fix STM32F1 emergency parser (MarlinFirmware#21011)
  Allow SERVO0_PIN override on Creality Melzi (MarlinFirmware#21007)
  Fix animated boot screen
  Fix: Unsupported use of %f in printf (MarlinFirmware#21001)
  Fix mini12864 v2.1 + PSU control + NeoPixel backlight (MarlinFirmware#21021)
  [cron] Bump distribution date (2021-02-08)
  Fix LVGL "more" menu user items (MarlinFirmware#21004)
  Fix TEMP_0_TR_ENABLE, rename temp conditions (MarlinFirmware#21016)
  Fix ESP32 I2S init placement (MarlinFirmware#21019)
  Improve RPi host kernel panic mitigation
  Melzi, comments cleanup
  ...
kpishere pushed a commit to kpishere/Marlin that referenced this pull request Feb 19, 2021
zillarob pushed a commit to zillarob/Marlin that referenced this pull request Feb 25, 2021
vyacheslav-shubin pushed a commit to vyacheslav-shubin/Marlin that referenced this pull request Mar 10, 2021
vyacheslav-shubin pushed a commit to vyacheslav-shubin/Marlin that referenced this pull request Mar 10, 2021
W4tel-BiDi pushed a commit to W4tel-BiDi/Marlin that referenced this pull request Apr 5, 2021
thinkyhead pushed a commit that referenced this pull request Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: ESP32 PR: Workaround T: HAL & APIs Topic related to the HAL and internal APIs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants