Skip to content

Add support for ESP32S3 #106

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

Merged
merged 3 commits into from
May 29, 2024
Merged

Conversation

rhysperry111
Copy link
Contributor

@rhysperry111 rhysperry111 commented Mar 15, 2024

Starting to try and add support for the ESP32S3. Main issues with this was the board does not have a DAC so PWM has to be used as a crude substitute.

While modifying the ESP32 file I also made the PWM_CONTROLLER implementation more flexible, moving away from FastPWM which seems to be abandoned over to the more standard LEDC.

Would appreciate some help as in order to use the required LEDC channel initiation function, esp32:esp32 core >3.0.0 must be used. I have made an attempt to switch over to the development library sources, but I'm just getting a bunch of undefined reference errors when compiling.

Changes made:

  • Remove fastPWM dependency as it seems unmaintained and has issues with duty cycle 0% and 100% (at least on my board). Switch to the LEDC library which is bundled with the ESP32 core.
    • This also allowed for static channel<->pin<->timer mappings in esp32.cpp to be removed. Now the PWM_CONTROLLER block will simply take the board pin as the channel, and will auto assign a timer from those available. (BREAKING)
    • Move to dev version of ESP32 core as current version doesn't support auto assigning timers. (see 3.0.0 version Migration related issues espressif/arduino-esp32#8796)
  • Add the ESP32-S3 board
    • Use PWM to emulate analog outputs (at 4khz) on boards without a DAC

@rhysperry111 rhysperry111 marked this pull request as draft March 15, 2024 15:00
@rhysperry111 rhysperry111 force-pushed the master branch 3 times, most recently from e112c4f to 43ed1fb Compare March 16, 2024 13:19
@OliverB21
Copy link

Any update on inclusion of this feature?

@rhysperry111
Copy link
Contributor Author

AFAIK it should be all good to be merged, however I have been unable to test everything as a whole because of compile errors when using the dev ESP32 core. I have no idea how to fix the issues with compilation in the current state, however you can track the progress of the 3.0.0 core release using the milestone on espressif/arduino-esp32.

You shouldn't need the specific hardware to reproduce the compilation error, so help from anyone with Arduino compiler knowledge would be appreciated.

@rhysperry111
Copy link
Contributor Author

@me-no-dev helped fix the compilation over in an issue in the espressif repo. Will add a commit to change some flags and then after some testing this should be ready for merge.

@rhysperry111
Copy link
Contributor Author

rhysperry111 commented Apr 24, 2024

Seems to be working - although I've had to do a hacky workaround because subprocess.Popen really doesn't seem to like the idea of having an argument to a command with a space in it. Will see what I can do tomorrow

note for tomorrow me: some basic "$@" arg debugging seems to suggest getting rid of quotes might help

@rhysperry111 rhysperry111 force-pushed the master branch 2 times, most recently from f8bb811 to 65fe87c Compare April 30, 2024 08:04
@rhysperry111
Copy link
Contributor Author

Looks all good from my side although I haven't been able to test anything on an OS other than Linux. I ended up removing the quotes from the compile args as they seemed to be messing with things.

@rhysperry111 rhysperry111 marked this pull request as ready for review April 30, 2024 08:19
@rhysperry111 rhysperry111 changed the title WIP: Add support for ESP32S3 Add support for ESP32S3 Apr 30, 2024
@thiagoralves
Copy link
Owner

@rhysperry111 Thank you so much for all your changes. I took a look at them and they look good, however I have a main (huge) concern. If I understood it correctly, you're replacing the ESP32 board file source with another source from Espressif that is basically a dev branch. Is that right? I'm worried about making that change and in one way or another break all the other ESP32 boards that are currently working fine on OpenPLC. Could we make that change exclusive for the ESP32S3?

@rhysperry111
Copy link
Contributor Author

rhysperry111 commented Apr 30, 2024

If I understood it correctly, you're replacing the ESP32 board file source with another source from Espressif that is basically a dev branch. Is that right?

Yeah that's correct. It's because the new esp32.cpp file uses functions which are only available in the 3.0.0 release which is currently in rc2. If it eases your conscience the dev sources aren't nightly but just the release candidates.

I'm worried about making that change and in one way or another break all the other ESP32 boards that are currently working fine on OpenPLC. Could we make that change exclusive for the ESP32S3?

Not really an easy way to do this without splitting off esp32.cpp as well so I'd rather not. I don't see any reason why other boards would break (although there is a breaking change in that PWM_CONTROLLER now maps channels to pins differently).

I'm happy to wait until 3.0.0 is in a stable release if it's a huge problem.

@thiagoralves
Copy link
Owner

Yeah, I believe waiting for a final release would be better. I'm just thinking about a million of other things that can go wrong, especially some crucial things like Serial that directly affects live monitoring.

@rhysperry111
Copy link
Contributor Author

3.0.0 has just been released upstream - not sure if there's any testing of other areas of code (serial, WiFi) that you want to do. I'll send in a commit switching back to stable sources soon.

@thiagoralves
Copy link
Owner

Awesome! If you can, just make sure that a simple program compiles for the ESP32 and that Modbus serial and TCP work

@thiagoralves
Copy link
Owner

It seems that ESP32 core 3.0.0 is broken. I updated my core and it won’t compile the blink example. See https://openplc.discussion.community/post/esp32-compilation-failed-13378321?pid=1339594513

@me-no-dev
Copy link

3.0.0 is not broken. There is something wrong on your end @thiagoralves

@rhysperry111
Copy link
Contributor Author

Probably due to the overriding of compile flags. One of the commits in this PR fixes that

@thiagoralves
Copy link
Owner

@rhysperry111 is this ready to merge or do you still need to change the board source file back to the original?

@rhysperry111
Copy link
Contributor Author

rhysperry111 commented May 29, 2024

Already changed the board file back with the force push two days ago. I've not done any testing to verify modbus over RTU, however I had modbus over TCP working.

Probably worth merging so at least ESP32 will compile again and then fixing anything that crops up with another PR. More than happy to help with that.

@thiagoralves
Copy link
Owner

Awesome! Merging now. Thanks again!

@thiagoralves thiagoralves merged commit 8cca7c0 into thiagoralves:master May 29, 2024
@rhysperry111
Copy link
Contributor Author

rhysperry111 commented May 29, 2024

If you plan on doing a release with this in, it's worth mentioning that existing ESP32 projects that use PWM_CONTROLLER blocks will need to be tweaked slightly.

The pin input to the block now directly maps to the output PWM pin rather than being an index to an arbitrary mapping

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