Skip to content

Add basic LEDC support for esp32, esp32c3, esp32s2 and esp32s3#114

Merged
JurajSadel merged 1 commit into
esp-rs:mainfrom
JurajSadel:feature/LEDC
Jul 30, 2022
Merged

Add basic LEDC support for esp32, esp32c3, esp32s2 and esp32s3#114
JurajSadel merged 1 commit into
esp-rs:mainfrom
JurajSadel:feature/LEDC

Conversation

@JurajSadel
Copy link
Copy Markdown
Contributor

@JurajSadel JurajSadel commented Jul 23, 2022

Basic LEDC support with examples for esp32, esp32c3, esp32s2 and esp32s3.

TODO:

  • Hardware fade support
  • Interrupts

@JurajSadel JurajSadel force-pushed the feature/LEDC branch 6 times, most recently from bf1e4d1 to 4cc7065 Compare July 25, 2022 11:18
@JurajSadel
Copy link
Copy Markdown
Contributor Author

JurajSadel commented Jul 25, 2022

We had a discussion with @bjoernQ about what should we do next with ESP32S2 and ESP32S3. My initial plan was to patch S2 and S3 and implement ledc for them like for ESP32 and ESP32C3. However, C3, S2, and S3 have only one mode (Slow) so there is a possibility to add stand-alone implementation for these three and remove C3 from this PR.
Our SVD for C3 is different from the TRM - in SVD we do have LowSpeed i.e LS in the register/field names, which is confusing and doesn't make sense to patch S2 and S3 to look like C3. @bjoernQ sugessted we can patch C3 SVD to match S2/S3 ones and remove these LS from them.
I'll start implementing S2 and S3 as a stand-alone implementation and when finished, we can decide what should we do with C3.
The question is, what should we do with this PR? Should I remove C3 part and merge it just as ESP32?
What are your opinions @bjoernQ @jessebraham @MabezDev?

@bjoernQ
Copy link
Copy Markdown
Contributor

bjoernQ commented Jul 25, 2022

I'd vote for removing C3 from this PR and have a common implementation for C3, S2, S3 without an intermediate step for the C3 - then we can already get the ESP32 implementation in

Copy link
Copy Markdown
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

I agree, lets get the awkward one in now (esp32) and implement the shared one together in another PR.

Comment thread esp-hal-common/src/gpio/esp32.rs
@jessebraham
Copy link
Copy Markdown
Member

I'm not understanding why this needs to be split up into multiple PRs. There don't appear to be any differences between the C3/S2/S3 other than number of channels (which can just be #[cfg()]'d) based on the TRM for this peripheral, so if it's already working for the C3 then S2/S3 support should be trivial.

@bjoernQ
Copy link
Copy Markdown
Contributor

bjoernQ commented Jul 25, 2022

I'm not understanding why this needs to be split up into multiple PRs. There don't appear to be any differences between the C3/S2/S3 other than number of channels (which can just be #[cfg()]'d) based on the TRM for this peripheral, so if it's already working for the C3 then S2/S3 support should be trivial.

I guess there will be one separate PR for S2/S3/C3 ... but currently there is an implementation for C3 here which needs to get removed since currently the SVD for the C3 (in regards to LEDC) looks ..... interesting. So we should remove C3 from this PR and get a PR for everything but ESP32 and use this for ESP32 only ... hope that makes sense 😄

@jessebraham
Copy link
Copy Markdown
Member

jessebraham commented Jul 25, 2022

It does make sense, I was just under the impression it was working for the ESP32 and ESP32-C3 and so assumed that ESP32-S2/S3 were right behind them, and had no idea there were issues with the SVDs. I have already merged one set of patches for this peripheral, not sure why we didn't just patch the other offending SVDs at the same time.

@bjoernQ
Copy link
Copy Markdown
Contributor

bjoernQ commented Jul 25, 2022

I have already merged one set of patches for this peripheral, not sure why we didn't just patch the other offending SVDs at the same time.

I think it just turned-out during today's review activity that the ESP32-C3 SVD wasn't in shape regarding LEDC - that patch was for ESP32 only I think. So I guess limiting this to ESP32 and then have a separate PR (or two since at least C3 needs SVD patches) is the best to move on here

@jessebraham
Copy link
Copy Markdown
Member

jessebraham commented Jul 25, 2022

I've looked at the SVDs for the C3, S2, and S3. It looks like only the C3 needs a small patch and they are all otherwise the same.

Already working on a fix. Changes to the C3's SVD will be:

  • The LS prefixes will be stripped from the register names
  • The CH_*_CONF0, CH_*_HPOINT, CH_*_DUTY, CH_*_CONF1, CH_*_DUTY_R, TIMER_*_CONF, and TIMER_*_VALUE registers will be collected into arrays

EDIT: will also be patching some field names for the S2 and S3 which still have _CH0 suffixes, etc.

@JurajSadel JurajSadel force-pushed the feature/LEDC branch 2 times, most recently from 4ef4975 to 55111e2 Compare July 29, 2022 09:20
@JurajSadel JurajSadel changed the title Add basic LEDC support for esp32 and esp32c3 Add basic LEDC support for esp32, esp32c3, esp32s2 and esp32s3 Jul 29, 2022
@JurajSadel
Copy link
Copy Markdown
Contributor Author

I split the implementation into 2 separate ones - one for esp32 and the second for the rest of the chips (esp32c3, esp32s2, esp32s3). I know there is a lot of duplicated code right now but I think it is "better" than having one with lots of conditionals (because of 2 modes of esp32 and a different field name in the timer register).

Copy link
Copy Markdown
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread esp32c3-hal/examples/ledc.rs
Comment thread esp32c3-hal/examples/ledc.rs
Comment thread esp32c3-hal/examples/ledc.rs
Comment thread esp-hal-common/src/ledc/timer.rs Outdated
Comment thread esp32c3-hal/examples/ledc.rs Outdated
@bjoernQ bjoernQ requested a review from MabezDev July 29, 2022 10:43
Comment thread esp-hal-common/src/ledc/esp32/timer.rs
Copy link
Copy Markdown
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

LGTM!

@JurajSadel JurajSadel merged commit 4d4b60a into esp-rs:main Jul 30, 2022
@bjoernQ
Copy link
Copy Markdown
Contributor

bjoernQ commented Jul 30, 2022

🎉

@JurajSadel JurajSadel deleted the feature/LEDC branch February 28, 2023 07:51
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