Initial functioning version of ESP32 HardwarePWM. #2599

pljakobs · 2023-01-01T08:31:27Z

a few weeks ago I said I would take a swing at implementing HardwarePWM for the ESP32 Platform and here it is.
This is very preliminary and does not extend the api a lot (in fact, the only public function is curently HardwarePWM::getFrequency(uint8_t pin) which I used in debugging).
I would go ahead and extend the interface to expose the more advanced functions of ledc_ bt would appreciate your input on how to do so. My current thinking is that I put conditionals into the master HardwarePWM.h - but conditionals can make code very hard to read if the number of platforms gets too large.

what-the-diff · 2023-01-01T08:31:36Z

Added HardwarePWM.cpp to the project
Changed #include <HardwarePWM.h> in HardwarePWM.cpp to #include "HardwarePWM.h"
3a) Removed all references of EspDigitalPin from code (not needed for esp32, and not available on esp8266 anyway)
- this is a temporary fix until we have an updated digital pin map that includes both platforms!
  3b) Replaced ESP_ERROR_CHECK with debugf("error message") as it's more readable when debugging issues during development phase; will be removed later once everything works fine again...

pljakobs · 2023-01-01T15:21:20Z

I've added a number of toughts as a comment to the cpp file but don't want to open a new PR for just that, so I post it here for discussion

` * the ESP32 PWM Hardware is much more powerful than the ESP8266, allowing wider PWM timers (up to 20 bit)
 * as well as much higher PWM frequencies (up to 40MHz for a 1 Bit wide PWM)
 * 
 * Overview:
 * +------------------------------------------------------------------------------------------------+
 * | LED_PWM                                                                                        |
 * |  +-------------------------------------------+   +-------------------------------------------+ |
 * |  | High_Speed_Channels¹                      |   | Low_Speed_Channels                        | |
 * |  |                   +-----+     +--------+  |   |                   +-----+     +--------+  | |
 * |  |                   |     | --> | h_ch 0 |  |   |                   |     | --> | l_ch 0 |  | |
 * |  | +-----------+     |     |     +--------+  |   | +-----------+     |     |     +--------+  | |
 * |  | | h_timer 0 | --> |     |                 |   | | l_timer 0 | --> |     |                 | |
 * |  | +-----------+     |     |     +--------+  |   | +-----------+     |     |     +--------+  | |
 * |  |                   |     | --> | h_ch 1 |  |   |                   |     | --> | l_ch 1 |  | |
 * |  |                   |     |     +--------+  |   |                   |     |     +--------+  | |
 * |  |                   |     |                 |   |                   |     |                 | |
 * |  |                   |     |     +--------+  |   |                   |     |     +--------+  | |
 * |  |                   |     | --> | h_ch 2 |  |   |                   |     | --> | l_ch 2 |  | |
 * |  | +-----------+     |     |     +--------+  |   | +-----------+     |     |     +--------+  | |
 * |  | | h_timer 1 | --> |     |                 |   | | l_timer 1 | --> |     |                 | |
 * |  | +-----------+     |     |     +--------+  |   | +-----------+     |     |     +--------+  | |
 * |  |                   |     | --> | h_ch 3 |  |   |                   |     | --> | l_ch 3 |  | |
 * |  |                   |     |     +--------+  |   |                   |     |     +--------+  | |
 * |  |                   | MUX |                 |   |                   | MUX |                 | |
 * |  |                   |     |     +--------+  |   |                   |     |     +--------+  | |
 * |  |                   |     | --> | h_ch 4 |  |   |                   |     | --> | l_ch 4 |  | |
 * |  | +-----------+     |     |     +--------+  |   | +-----------+     |     |     +--------+  | |
 * |  | | h_timer 2 | --> |     |                 |   | | l_timer 2 | --> |     |                 | |
 * |  | +-----------+     |     |     +--------+  |   | +-----------+     |     |     +--------+  | |
 * |  |                   |     | --> | h_ch 5 |  |   |                   |     | --> | l_ch 5 |  | |
 * |  |                   |     |     +--------+  |   |                   |     |     +--------+  | |
 * |  |                   |     |                 |   |                   |     |                 | |
 * |  |                   |     |     +--------+  |   |                   |     |     +--------+  | |
 * |  |                   |     | --> | h_ch 6 |  |   |                   |     | --> | l_ch 6²|  | |
 * |  | +-----------+     |     |     +--------+  |   | +-----------+     |     |     +--------+  | |
 * |  | | h_timer 3 | --> |     |                 |   | | l_timer 3 | --> |     |                 | |
 * |  | +-----------+     |     |     +--------+  |   | +-----------+     |     |     +--------+  | |
 * |  |                   |     | --> | h_ch 7 |  |   |                   |     | --> | l_ch 7²|  | |
 * |  |                   |     |     +--------+  |   |                   |     |     +--------+  | |
 * |  |                   +-----+                 |   |                   +-----+                 | |
 * |  +-------------------------------------------+   +-------------------------------------------+ |
 * +------------------------------------------------------------------------------------------------+
 * ¹ High speed channels are only available when SOC_LEDC_SUPPORT_HS_MODE is defined as 1
 * ² The ESP32C3 does only support six channels, so 6 and 7 are not available on that SoC
 * 
 * The nomenclature of timers in the high speed / low speed blocks is a bit misleading as the idf api 
 * speaks of "speed mode", which, to me, implies that this would be a mode configurable in a specific timer
 * while in reality, it does select a block of timers. I am considering re-naming that to "speed mode block"
 * in my interface impmenentation.
 * 
 * As an example, I would use
 * setTimerFrequency(speedModeBlock, timer, frequency);
 * 
 * ToDo: see, how this can be implemented to provide maximum overlap with the RP2040 pwm hardware so code does 
 * not become overly SoC specific.
 * 
 * Maximum Timer width for PWM:
 * ============================
 * esp32   SOC_LEDC_TIMER_BIT_WIDE_NUM  (20)
 * esp32c3 SOC_LEDC_TIMER_BIT_WIDE_NUM  (14)
 * esp32s2 SOC_LEDC_TIMER_BIT_WIDE_NUM  (14)
 * esp32s3 SOC_LEDC_TIMER_BIT_WIDE_NUM  (14)
 * 
 * Number of Channels:
 * ===================
 * esp32   SOC_LEDC_CHANNEL_NUM         (8)
 * esp32c3 SOC_LEDC_CHANNEL_NUM         (6)
 * esp32s2 SOC_LEDC_CHANNEL_NUM         (8)
 * esp32s3 SOC_LEDC_CHANNEL_NUM 		(8)
 * 
 * Some SoSs support a mode called HIGHSPEED_MODE which is essentially another full block of PWM hardware 
 * that adds SOC_LEDC_CHANNEL_NUM channels. 
 * Those Architectures have SOC_LEDC_SUPPORT_HS_MODE defined as 1.
 * In esp-idf-4.3 that's currently only the esp32 SOC 
 * 
 * Supports highspeed mode:
 * ========================
 * esp32 SOC_LEDC_SUPPORT_HS_MODE	(1)
 *
 * ToDo: implement awareness of hs mode availablility
 * ==================================================
 * currently, the code just uses a % 8 operation on the pin index to calculate whether to assign a pin to either
 * high speed or low speed pwm blocks. This doesn't make a whole lot of sense since it makes it impossible
 * for Sming devs to actually use the functionality behind it. 
 * Also, it currently does not reflect the fact that different SOCs have a different number of channels per block
 * (specifically, the esp32c3 only has six channels and no highspeed mode). 
 * I will continue in two ways: 
 * - implement the "vanilla" Sming HardwarePWM interface that will hide the underlying architecture but allow up to 16 
 *   channels on an ESP32 
 * - implement overloads for the relevant functions that allow selecting hs mode where applicable. 
 * 
 * ToDo: implement PWM bit width control
 * =====================================
 * the current HardwarePWM implementation does not care about the PWM timer bit width. To leverage the functionality 
 * of the ESP32 hardware, it's necessary to make this configurable. As the width is per timer and all the Sming defined
 * functions are basically per pin, this needs to be an extension of the overal model, exposing at least timers. 
 * This, too, will require a compatible "basic" interface and an advanced interface that allows assiging pins (channels) 
 * to timers and the configuration of the timers themselves. 
 * The esp_idf does not provide a way to read the bit width configured for a channel, but I think it'll be useful to be able
 * to read back this value, not least to find the value for getMaxDuty() for a channel. It will either have to be stored in the
 * module or maybe read from the hardware directly (LEDC_[HL]STIMERx_CONF_REG & 0x1f)
 * 
 * hardware technical reference: 
 * =============================
 * https://www.espressif.com/sites/default/files/documentation/esp32_technical_reference_manual_en.pdf#ledpwm
 * 
 * Overview of the whole ledc-system here: 
 * https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/peripherals/ledc.html
 * 
 * the ledc interface also exposes some advanced functions such as fades that are then done in hardware. 
 * ToDo: implement a Sming interface for fades
 * `

Not sure what a good implementation for a more abstract interface is. Maybe a baseclass / SoC specific implementation class scheme to implement generic functions in software and provide SoC specific capabilities plus extensions where available? That's how the adafruit gfx library builds it for specific hardware.

mikee47

Great start, thank you! In general just a little tidying.

Sming/Arch/Esp32/Core/HardwarePWM.cpp

Sming/Core/HardwarePWM.h

Sming/Arch/Esp32/Core/HardwarePWM.cpp

pljakobs · 2023-02-01T10:59:06Z

(sorry for takign some more time, I was travelling and then sick for two weeks - will get to this soon)

pljakobs · 2023-02-12T12:59:33Z

ok, I'm sorry if this is now very muddled, but I believe I got lost in git here - for reasons I don't understand, both, the .h as well as the .cpp file were repeatedly lost by git and no longer tracked. I think, now it's all there

…e ESP32C3 with up to 5kHz pwm frequency.

This reverts commit 7279c66.

…ter goofing up with git Sigh)

mikee47

I've rebased and done some tidying but code functionality hasn't been changed.
Couple of points to consider regarding return values for invalid period/frequency and how update() works.

Sming/Arch/Esp32/Core/HardwarePWM.cpp

pljakobs · 2023-03-24T09:42:14Z

what's that remaining change request? I can't seem to see it.

mikee47

Great, just remove those unused variables

mikee47 · 2023-03-24T10:18:39Z

Sming/Arch/Esp32/Core/HardwarePWM.cpp

+	uint32_t io_info[SOC_LEDC_CHANNEL_NUM][3];	// pin information
+	uint32_t pwm_duty_init[SOC_LEDC_CHANNEL_NUM]; // pwm duty
+	for(uint8_t i = 0; i < no_of_pins; i++) {
+		pwm_duty_init[i] = 0; // Start with zero output


io_info and pwm_duty_init aren't used, please remove

Sming/Arch/Esp32/Core/HardwarePWM.cpp

slaff · 2023-04-05T08:49:55Z

@pljakobs Please run the coding style fixer. The following file has an issue:

--- a/Sming/Arch/Esp32/Core/HardwarePWM.cpp
+++ b/Sming/Arch/Esp32/Core/HardwarePWM.cpp
@@ -310,4 +310,3 @@ uint32_t HardwarePWM::getFrequency(uint8_t pin)
 {
 	return ledc_get_freq(pinToGroup(pin), pinToTimer(pin));
 }
-' ']'
++ echo '!!! Coding Style issues Found!!!'
++ echo '    Run: '\''make cs'\'' to fix them. '
++ echo 'diff --git a/Sming/Arch/Esp32/Core/HardwarePWM.cpp b/Sming/Arch/Esp32/Core/HardwarePWM.cpp
index f6aefe8..38ef1ad 1006[44](https://github.com/SmingHub/Sming/actions/runs/4582860432/jobs/8149817907?pr=2599#step:7:45)
--- a/Sming/Arch/Esp32/Core/HardwarePWM.cpp
+++ b/Sming/Arch/Esp32/Core/HardwarePWM.cpp
@@ -310,4 +310,3 @@ uint32_t HardwarePWM::getFrequency(uint8_t pin)
 {
 	return ledc_get_freq(pinToGroup(pin), pinToTimer(pin));
 }
-'
++ exit 1

slaff · 2023-04-05T08:54:02Z

@pljakobs Also fix the typos reported here: https://github.com/SmingHub/Sming/pull/2599/files#diff-647d65279a6f999683c4cd320288f28dd4f901ca8a4eba5a75f1c9e2ad4ff451

slaff · 2023-04-05T08:58:47Z

Sming/Core/HardwarePWM.h

@@ -122,6 +122,11 @@ class HardwarePWM
 	 */
 	void update();

+	/** @brief Get PWM Frequency


Add also
@param pin GPIO to get frequency for

pljakobs · 2023-04-05T20:19:04Z

you guys are thorrough! hope I caught it all by now.

Also - can I get some feedback on the other implementation that I've started (#2624)?

Sming/Arch/Esp32/Core/HardwarePWM.cpp

…t iteraton as they are already underway

slaff

Overall looks good to me. There are still few coding style and return value issues that have to be fixed before merging this PR.

slaff · 2023-04-13T07:18:16Z

Sming/Arch/Esp32/Core/HardwarePWM.cpp

+
+uint32_t periodToFrequency(uint32_t period)
+{
+	return (period == 0) ? -1 : (1000000 / period);


Please, change the return value to be a valid uint32_t. -1 is not a valid unsigned int.

slaff · 2023-04-13T07:18:23Z

Sming/Arch/Esp32/Core/HardwarePWM.cpp

+
+uint32_t frequencyToPeriod(uint32_t freq)
+{
+	return (freq == 0) ? -1 : (1000000 / freq);


Please, change the return value to be a valid uint32_t. -1 is not a valid unsigned int.

Sming/Arch/Esp32/Core/HardwarePWM.cpp

slaff · 2023-04-13T07:21:45Z

Sming/Arch/Esp32/Core/HardwarePWM.cpp

+{
+	for(uint8_t i = 0; i < channel_count; i++) {
+		if(channels[i] == pin) {
+			// debug_d("getChannel %d is %d", pin, i);


Either remove the commented line or uncomment it. I would prefer to have it removed if it is not improving the debug output.

slaff · 2023-04-13T07:32:47Z

Sming/Arch/Esp8266/Core/HardwarePWM.cpp

 #include <Clock.h>
 #include "ESP8266EX.h"
 #include <debug_progmem.h>

-#include <HardwarePWM.h>
-
 #define PERIOD_TO_MAX_DUTY(x) (x * 25)

 HardwarePWM::HardwarePWM(uint8_t* pins, uint8_t no_of_pins) : channel_count(no_of_pins)


Please, keep close to Sming's coding style: https://sming.readthedocs.io/en/latest/information/develop/coding-style.html?highlight=Variables#naming.

Local variables, instance variables, and class variables must also be written in lowerCamelCase. Variable names must not start with, end with or contain underscore (_) or dollar sign ($) characters.

For example in the code here uint8_t no_of_pins should be renamed to uint8_t numberOfPins, uint32_t io_info[PWM_CHANNEL_NUM_MAX][3]; -> uint32_t ioInfo[PWM_CHANNEL_NUM_MAX][3];, uint32_t pwm_duty_init[PWM_CHANNEL_NUM_MAX] -> uint32_t pwmDutyInit[PWM_CHANNEL_NUM_MAX] and so on.

so no_of_pins is part of the core interface definition, if I rename that, I should probably also rename it for the other architectures - should this be part of this PR or may be a PR of it's own?
I believe, all other occurences of underscored names are defined in the esp32 ledc interface.

should this be part of this PR or may be a PR of it's own

Leave it for another PR. This one waited already for too long to be merged.

I believe, all other occurences of underscored names are defined in the esp32 ledc interface.

Please, rename all local variables in the code that you have introduced to use lowerCamelCase . Example:

uint32_t io_info[PWM_CHANNEL_NUM_MAX][3]; // pin information uint32_t pwm_duty_init[PWM_CHANNEL_NUM_MAX]; // pwm duty // ... const int initial_period = 1000;

and so on.

I don't use those, they are part of the original ESP8266 version and are used both there and in the RP2040 implementation. I'll go ahead and rename them all and send a separate PR.

slaff · 2023-04-13T07:33:53Z

Sming/Arch/Esp8266/Core/HardwarePWM.cpp

 uint8_t HardwarePWM::getChannel(uint8_t pin)
 {
 	for(uint8_t i = 0; i < channel_count; i++) {
 		if(channels[i] == pin) {
-			//debugf("getChannel %d is %d", pin, i);
+			// debug_d("getChannel %d is %d", pin, i);


Remove commented debug statements.

is this really part of my PR? I have not touched the 8266 code

That was me! See commit eaf77f9 - there's a new getFrequency method which is required.

ah, I was really confused for a moment now. I have renamed the local vars to comply with camelCase now, not yet removed the debug calls. I think some of them are worthy of being there and functional, so I will uncomment them

slaff · 2023-04-13T07:33:59Z

Sming/Arch/Esp8266/Core/HardwarePWM.cpp

 			return i;
 		}
 	}
-	//debugf("getChannel: can't find pin %d", pin);
+
+	// debug_d("getChannel: can't find pin %d", pin);


Remove commented debug statements.

slaff · 2023-04-13T07:34:23Z

Sming/Arch/Esp8266/Core/HardwarePWM.cpp

 	}
+
+	debugf("Duty cycle value too high for current period.");


Replace debugf with debug_w. And include also the duty cycle value to make the debug statement more useful.

Sming/Arch/Esp32/Components/driver/include/driver/pwm.h

…ect SoC config

pljakobs · 2023-04-23T12:21:00Z

sice I've deleted my earlier, confused comment, I'll say it again:
I really am impressed and I appreciate the diligence, professionality and support you are showing here. As I am sure you could tell, I don't do any of my coding at that level so it's an interesting experience for me, albeit I fear I've created more work for you than I actually did.

slaff · 2023-04-28T15:07:36Z

I really am impressed and I appreciate the diligence, professionality and support you are showing here.

@pljakobs Thank you for your contribution and congratulations for your first merged PR :)

slaff requested a review from mikee47 January 1, 2023 09:15

slaff changed the title ~~initial functioning version of ESP32 HardwarePWM.~~ [WIP] Initial functioning version of ESP32 HardwarePWM. Jan 1, 2023

mikee47 requested changes Jan 4, 2023

View reviewed changes

mikee47 reviewed Jan 4, 2023

View reviewed changes

Sming/Arch/Esp32/Core/HardwarePWM.cpp Outdated Show resolved Hide resolved

pljakobs and others added 15 commits February 21, 2023 07:43

initial functioning version of ESP32 HardwarePWM. Tested to run on th…

c7e7e33

…e ESP32C3 with up to 5kHz pwm frequency.

some documentation updates

806b9de

more documentation

34ca146

more documentation

246744e

implemented requested changes from initial PR

30b5f1a

implemented requested changes from initial PR (now *with* changes. Sigh)

f67fb1a

Revert "more documentation"

21ecf9a

This reverts commit 7279c66.

implemented requested changes from initial PR (now *with* changes. af…

be7d04a

…ter goofing up with git Sigh)

fixed some things I didn't understand at first.

9e9928c

HardwarePWM.h was not part of the last commit

b48fcd9

.cpp was also missing. why?

9fe74a1

Apply coding style

f510bba

Rebase, tidy

84e69f1

Tidy Esp8266 implementation, add new 'getFrequency' method

eaf77f9

Add missing include

333db82

mikee47 force-pushed the esp32_pwm branch from b6c3484 to 333db82 Compare February 21, 2023 08:20

mikee47 added 2 commits February 21, 2023 08:51

Tidy driver/pwm.h

b90ea67

Use static initializers

3c9785f

mikee47 requested changes Feb 21, 2023

View reviewed changes

Sming/Arch/Esp32/Core/HardwarePWM.cpp Show resolved Hide resolved

Sming/Arch/Esp32/Core/HardwarePWM.cpp Outdated Show resolved Hide resolved

Sming/Arch/Esp32/Core/HardwarePWM.cpp Outdated Show resolved Hide resolved

minor updates as suggested by mikee47

fc2fca2

mikee47 requested changes Mar 24, 2023

View reviewed changes

pljakobs added 3 commits March 30, 2023 22:34

minor updates as suggested by mikee47

d96bc2f

removed unused variables

8b82fe0

missed one occurrence and also messed up in a merge - cleaned out now

d03688b

mikee47 reviewed Mar 31, 2023

View reviewed changes

Sming/Arch/Esp32/Core/HardwarePWM.cpp Outdated Show resolved Hide resolved

removed a piece of code that was doubled

0e06be0

mikee47 approved these changes Apr 4, 2023

View reviewed changes

slaff added the 3 - Review label Apr 4, 2023

slaff reviewed Apr 5, 2023

View reviewed changes

fixed typos and coding style

b3489a1

mikee47 reviewed Apr 8, 2023

View reviewed changes

Sming/Arch/Esp32/Core/HardwarePWM.cpp Outdated Show resolved Hide resolved

removed ToDos from the comments that are to be implemented in the nex…

e5c1138

…t iteraton as they are already underway

slaff changed the title ~~[WIP] Initial functioning version of ESP32 HardwarePWM.~~ Initial functioning version of ESP32 HardwarePWM. Apr 13, 2023

slaff added this to the 4.8.0 milestone Apr 13, 2023

slaff requested changes Apr 13, 2023

View reviewed changes

pljakobs added 4 commits April 19, 2023 17:09

fixed return values

5457b4a

changed some coding style issues in esp8266 code

dcf2834

removed unnecessary and improved useful debug output

bbacc5a

corrected the PWM_CHANNEL_NUM_MAX define in pwm.h to reflect the corr…

e1413ff

…ect SoC config

Small coding style fixes.

f03ce14

slaff removed the 3 - Review label Apr 28, 2023

slaff merged commit 5eb7426 into SmingHub:develop Apr 28, 2023

slaff mentioned this pull request May 3, 2023

Preparation for Release 5.0 (was 4.8.0) #2597

Closed

5 tasks

Initial functioning version of ESP32 HardwarePWM. #2599

Initial functioning version of ESP32 HardwarePWM. #2599

Conversation

pljakobs commented Jan 1, 2023

what-the-diff bot commented Jan 1, 2023

pljakobs commented Jan 1, 2023

mikee47 left a comment

Choose a reason for hiding this comment

pljakobs commented Feb 1, 2023

pljakobs commented Feb 12, 2023

mikee47 left a comment

Choose a reason for hiding this comment

pljakobs commented Mar 24, 2023

mikee47 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slaff commented Apr 5, 2023

slaff commented Apr 5, 2023

Choose a reason for hiding this comment

pljakobs commented Apr 5, 2023

slaff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pljakobs commented Apr 23, 2023

slaff commented Apr 28, 2023