[WIP] Beginnings of a more structured esp32 pwm

[pljakobs]

** this is not currently functional code **
I'm adding this PR to get feedback on the direction.

After thinking about the inital implementation I wrote for PWM on the ESP32, I thought about what would make sense for this platform within the existing interface. Especially, I thought about the fact that there is a differenciation between properties of a timer (frequency, bit width) and properties of a channel (timer, pin, duty cycle). The interface in Sming really only knows about pins, but through the common update() function has a rudimentary idea of grouping channels.

So, my Idea is this: I'll try to keep the existing interface as much as possibe, but will extend the idea by encouraging to use multiple HardwarePWM objects that automatically use the existing hardware.

That is: a program could create one HardwarePWM object to drive three led channels at 400kHz and 10 Bits and another one to drive something else, maybe two motor channels, at 100kHz and 8 Bits (just examples).

My original implementation would not allow that because it has no concept of hardware that is already in use. In order to globally keep track of what is already in use and what is still available, I've added the two singleton classes for channels and timers. They provide an interface to get the number of a timer and a channel and mark that timer and channel as used so another class does not use the same.

It would probably be even better if the class would keep the configuration object for the timer and the channel, but that is beyond my current understanding of how to handle objects in singletons (this already took me quite a bit of learning).

The downside of the current implementation is, that one has to manually keep track of the fact whether a channel / timer is in the high or low speed block, which is messy and prone to error.

It would be better to handle them as objects providing the information in ledc_channel_config_t and ledc_timer_config_t. I'll think about that some more

typedef struct {
    int gpio_num;                   /*!< the LEDC output gpio_num, if you want to use gpio16, gpio_num = 16 */
    ledc_mode_t speed_mode;         /*!< LEDC speed speed_mode, high-speed mode or low-speed mode */
    ledc_channel_t channel;         /*!< LEDC channel (0 - 7) */
    ledc_intr_type_t intr_type;     /*!< configure interrupt, Fade interrupt enable  or Fade interrupt disable */
    ledc_timer_t timer_sel;         /*!< Select the timer source of channel (0 - 3) */
    uint32_t duty;                  /*!< LEDC channel duty, the range of duty setting is [0, (2**duty_resolution)] */
    int hpoint;                     /*!< LEDC channel hpoint value, the max value is 0xfffff */
} ledc_channel_config_t;

/**
 * @brief Configuration parameters of LEDC Timer timer for ledc_timer_config function
 */
typedef struct {
    ledc_mode_t speed_mode;                /*!< LEDC speed speed_mode, high-speed mode or low-speed mode */
    union {
        ledc_timer_bit_t duty_resolution;  /*!< LEDC channel duty resolution */
        ledc_timer_bit_t bit_num __attribute__((deprecated)); /*!< Deprecated in ESP-IDF 3.0. This is an alias to 'duty_resolution' for backward compatibility with ESP-IDF 2.1 */
    };
    ledc_timer_t  timer_num;               /*!< The timer source of channel (0 - 3) */
    uint32_t freq_hz;                      /*!< LEDC timer frequency (Hz) */
    ledc_clk_cfg_t clk_cfg;                /*!< Configure LEDC source clock.
                                                For low speed channels and high speed channels, you can specify the source clock using LEDC_USE_REF_TICK, LEDC_USE_APB_CLK or LEDC_AUTO_CLK.
                                                For low speed channels, you can also specify the source clock using LEDC_USE_RTC8M_CLK, in this case, all low speed channel's source clock must be RTC8M_CLK*/
} ledc_timer_config_t;

So here are my questions:

• is this a sensible way to develp the implementation? I feel it would get a lot more weight to the idea of having a group of channels belonging together
• would it be helpful to have the singleton baseclass as part of the core to maybe use with other config objects?
• woudl it be helpful to have this channel/timer abstraction as part of the core? different platforms could implement it according to their hardware and the actual HardwarePWM class would be mostly hardware independent
• if not: where actually does an architecture specific .h file need to go so it is found by the linker?

If this is generally a good direction, I'll try to develop it further and actually use it in the HardwarePWM code.

Also: is there a better way to ask for feedback than a PR?

thanks in advance

pj

[what-the-diff]

• Added a new file singelton.cpp
• Added a new file singleton.h
• Modified HardwarePWM class to use the above files for PWM functionality

[pljakobs]

So I have reconsidered my initial thoughts on this and written two intermediary classes:
ledc_channel and ledc_timer that deal with the configuration of channels and timers, respectively.
When instanciating a timer, the constructor takes the parameters mode, duty resolution, frequency and clock source config. It gets the number of a free timer from the singleton class timer (Timer::instance()->getTimer(mode) ), and configures the timer. The object can then be used to change the frequency, stop/resume/reset the timer, set the Frequency or reconfigure the timer (although I might still break that up into specific functions for clock config and duty resolution) and get the timer properties.
The ledc_channel class works analogous for channels.
When creating a HardwarePWM, the constructor will first verify that there are enough free channels available in the selected speed mode (I wish I had a way to return an error if there are not, suggestions on how to handle that case are welcome) and, if enough are available, first create a timer object and then create n channel objects associated with that timer object.
This should take care of the whole setup for the necessary components and also make sure that this object is a logical unit.
The timer related functions (getPeriod()/setPeriod() ) will only have to deal with one single timer, and configuring multiple pwm objects with different timer parameters (timer resolution, frequency, for ESP32 and ESP32S3 different clock sources) allows for more flexibility when pwm is needed for more than one use case (maybe leds and motor control)

One limitation is, that one object can not span the speed groups available in the larger esp32 models, so the largest possible number of channels in one instance is 8 (or 6 on the ESP32C3), but I believe that's a logical limitaition, as the two speed groups are also divided in hardware and have at least partially dissimilar capabilities.
It does, though, limit the usability a bit, as, despite of the ESP32 having 16 PWM channels, they won't be usable as 3x5 channels (RGB warm white cold white) for example unless the application itself bunches them together.

Again: feedback on the thoughts and architecture is very welcome, especially as I know that some (many?) regard singletons as an anti pattern. I thought it is the easiest way to make sure that the hardware is managed correctly, at least in a single threaded environment.

[mikee47]

I've chewed on this one for a while and have come up with an alternative arrangement.
You can find it in my fork https://github.com/mikee47/Sming/tree/feature/ledc-control/Sming/Libraries/LedControl. It's based on a rewrite of your code.

#include <LedControl.h>

LEDC::HSTimer timer;
LEDC::Channel led1(timer);
LEDC::Channel led2(timer);
LEDC::Channel led3(timer);

LEDC::LSTimer timer2(3); // 
LEDC::Channel otherPwm(timer2);

void test()
{
    timer.setFrequency(400000);
    led1.configure({
        .gpio = 2,
        .duty = 4000,
    });
    led2.configure({
        .gpio = 3,
        .duty = 5000,
    });
    led3.configure({
        .gpio = 4,
        .duty = 6000,
    });

    // Checking for validity

    if(!timer2) {
        // Failed to allocate free timer (in this example always succeeds)
    }

    if(!otherPwm) {
        // ....
    }
}

Some notes.

Move code into a separate library, e.g. Sming/Libraries/LedControl.

Leave the HardwarePWM class as-is. The LEDC library can provide a better implementation anyway so applications wouldn't use both.

Timer isn't a singleton, but does track allocation of timers and channels.
Prefer to use HSTimer or LSTimer instance as required.

Use LEDC namespace.

Follow Sming coding style; this helps to differentiate between low-level 'C' code (e.g. identifiers_using_underscores) versus upper CamelCaseTypes or lower camelCaseVariableNames.
Note also that a method/functions without parameters should be declared () as in C++ it's the same as (void). (NB. not true for C).

To reduce overhead, put simple code directly in class header, rather than source module,
so compiler can optimise more efficiently. Also simplifies maintainance.

Use bool return values. If required, debug output can be used within the library.

There are no plans for Sming to support FreeRTOS, quite the reverse in fact: if we could eliminate it entirely that would be a benefit.

Sming is intended to keep 'close to the metal', therefore the framework avoids using
the higher levels of the ESP32 SDK wherever possible to avoid bloating the code un-necessarily
with mutexes, tasks, critical sections, etc.
PR #2371 discusses how Sming interacts with FreeRTOS tasks.

The UART driver (Sming/Arch/Esp32/Components/driver/uart.cpp) avoids the SDK uart driver and goes directly to the hardware,
using the (low-level) SDK HAL calls to maintain compatibility between SoC variants.
Clearly this means we cannot use the SDK uart driver at the same time, which means we avoid using any components which depend on it (console, vfs).