ports/esp32: Use capability defines to configure features. #15862
Conversation
03666d9 to
dc8a191
Compare
|
Hi @projectgus the soc capability changes commit has been moved here and I think is ready for review / test again. |
0917e27 to
a7a424e
Compare
|
@projectgus I've rebased this now that C6 has merged, but haven't tested anything beyond it being able to compile for C6 |
There was a problem hiding this comment.
These changes look good, thanks @andrewleech. I have a couple of comments but it's pretty straightforward.
I can help test this PR on some of the other SoCs, although I haven't done this yet.
a7a424e to
b745173
Compare
|
Thanks @projectgus I've fixed that adc issue and reworked the PWM over in #16090 as suggested. |
|
@andrewleech When you have a moment to rebase this and drop the PWM change then it'd be great to get it merged. |
b745173 to
4a708b6
Compare
|
Ah yes, thanks for the reminder @projectgus I've rebased it now. |
There was a problem hiding this comment.
This looks great, @andrewleech! I have one small suggestion but otherwise looks good.
I can help do some testing on various boards and then I think we should get this in, should streamline future ESP32 additions. 🚀
| @@ -147,30 +147,34 @@ static void machine_sleep_helper(wake_type_t wake_type, size_t n_args, const mp_ | |||
| esp_sleep_enable_timer_wakeup(((uint64_t)expiry) * 1000); | |||
| } | |||
|
|
|||
| #if !(CONFIG_IDF_TARGET_ESP32C3 || CONFIG_IDF_TARGET_ESP32C6) | |||
|
|
|||
| #if SOC_PM_SUPPORT_EXT0_WAKEUP | |||
There was a problem hiding this comment.
I think we should leave this for an additional PR, but should we also do more filtering in machine_pin_irq (machine_pin.c) where the user sets the different wake up types? It looks like some invalid settings will fail, but I feel like some will be silently accepted (and then do nothing.)
There was a problem hiding this comment.
Oh yeah that's a good point. Pin irq / sleep settings that do / don't work seem to come up in issues regularly so a concerted effort to clean up and test these would be valuable. This arguably applies to all ports though, just just esp32...
4a708b6 to
980cc14
Compare
980cc14 to
22157a3
Compare
There was a problem hiding this comment.
Great work @andrewleech! I think we should merge this to progress some of the other ESP32-related PRs that add new chips or tweak features.
(I ended up doing less testing than I anticipated, although did try out the ADC on the ESP32-C6 which seems to work correctly. Have re-read the other diff items and I don't see any potential for subtle bugs like the PWM clock source - the I2C clock sources are disjoint so that one shouldn't change anything.)
This updates esp32 code where appropriate to replace ifdef's based on a list of specific chips with a feature SOC_* definition. This should simplify adding new esp32-* chips in future, deferring chip feature support to the IDF. Signed-off-by: Andrew Leech <andrew@alelec.net>
22157a3 to
9441ce6
Compare
Summary
Was broken out from #11869
This updates esp32 code where appropriate to replace ifdefs based on a list of specific chips with a feature SOC_* definition, eg.
to
Testing
TBD - each of these changes will requires review and test to confirm no unexpected regressions.
Trade-offs and Alternatives
This should simplify adding new esp32-* chips in future, deferring chip feature support to the IDF.