PR Summary

• Implemented Phase Shift Support: The upgrade involves an enhancement to the existing 'HardwarePWM' component, enabling it to support the phase shifting feature, a method used for adjusting the timing of signals.
• Introduced a Calculation Functionality: A new function, dubbed 'hpointForPin', has been introduced, capable of determining 'hpoint', a specific point in each PWM (Pulse Width Modulation) channel, relating to phase shift and channel number.
• Constructor Transformation: The constructor, a special method used for initializing newly created objects, has seen updates to not only initialize a new element, _usePhaseShift, but also provide a debug message if phase shifting is enabled.
• Incorporated Dynamic Value Calculation: The assignment of hpoint values in the LEDC (LED Controller) channel settings has been modified. Instead of a fixed zero, it will now utilize the newly introduced function for phase shifting.
• Improved Frequency Setting Procedure: The process within the setPeriod function responsible for setting the duty cycle has gotten an update. Now it decides whether to apply phase shift or set frequency directly based on the new _usePhaseShift variable.
• Updated Documentation: The 'HardwarePWM' header, a section containing explanatory information, has been updated to include the new constructor design, and definitions have been added for the phase shift and spread spectrum elements.

oh heck, adding the optional parameter breaks all other arch's ?
Makes sense of course. What's a good way to implement this? I'm pretty sure the feature could be implemented for RP2040 as well, but it's certainly not something that esp8266 can do.

update: I have updated the signatures of esp8266/Host/rp2040, but leave the optional parameter unhandled there.

oh heck, adding the optional parameter breaks all other arch's ? Makes sense of course. What's a good way to implement this? I'm pretty sure the feature could be implemented for RP2040 as well, but it's certainly not something that esp8266 can do.

update: I have updated the signatures of esp8266/Host/rp2040, but leave the optional parameter unhandled there.

I had a similar problem extending the UART driver. This is the original call:

smg_uart_t* smg_uart_init(
    uint8_t uart_nr,
    uint32_t baudrate,
    smg_uart_format_t format,
    smg_uart_mode_t mode,
    uint8_t tx_pin,
    size_t rx_size,
    size_t tx_size = 0);

This already has way too many parameters for a simple function call, and it's also too easy to pass them in the wrong order. So I added a second constructor function:

struct smg_uart_config_t {
	uint8_t uart_nr;
	uint8_t tx_pin;
	uint8_t rx_pin;
	smg_uart_mode_t mode;
	uart_options_t options;
	uint32_t baudrate;
	smg_uart_format_t format;
	size_t rx_size;
	size_t tx_size;
};

smg_uart_t* smg_uart_init_ex(const smg_uart_config_t& cfg);

I'd suggest a similar approach with HardwarePWM, adding a second constructor with a single structure reference as the argument:

class HardwarePWM
{
    /**
     * @brief Configuration for extended constructor
     *
     * Allows use of additional hardware feature for those platforms which support it.
     */
    struct Config {
        /**
         * @brief  Array of pins to control
         */
        const uint8_t* pins;
        /**
         * @brief Number of pins in array
         */
        uint8_t numberOfPins;
        /**
         * @brief ESP32 (and possibly RP2040) support phase shifting to control EMI
         */
        bool usePhaseShift{false};
        /**
         * @brief Enable minimal spread spectrum modulation to further reduce EMI
         */
        bool useSpreadSpectrum{false};
    };

    /**
     * @brief Constructor to access advanced hardware capabilities
     */
    HardwarePWM(const Config& config);

    // ... etc.
};

And in user code:

HardwarePWM pwm({
    .pins = ...,
    .numberOfPins = ...,
    .usePhaseShift = ...,
});

If we only implement the ESP32 version (for now) then other architectures will fail to build if the extended constructor is used.

There is the question of how to handle unsupported configurations.
Perhaps here we can just print warnings and assume that the user knows what they're doing?

It might even be worth defining architecture-specific configuration; that could go in Sming/Arch/Esp32/Components/driver/include/pwm.h. For example, phase shifting might be more than just a bool and express how much.

good points.
I believe that RP2040 already supports phase shifting and also phase offsets, I just don't use that platform, so I can't test an implementation.
Given how flexible the RP2040 is, it may actually also allow for spread spectrum PWM (that is: the PWM base frequency is modulated by a lower frequency signal to further reduce EMI) which is something I also want to implement for the esp32, which does not natively support it.

The Rp2040 hardware abstraction is straightforward and adds no bloat so I just use it directly rather than trying to abstract it. If I ever did anything serious with the Esp32 I'd probably end up rewriting the driver code, just to get things closer to the metal.

It might even be worth defining architecture-specific configuration; that could go in Sming/Arch/Esp32/Components/driver/include/pwm.h. For example, phase shifting might be more than just a bool and express how much.

With this approach perhaps adding Sming/Arch/Esp32/Core/pwm_arch.h for architecture-specific definitions - driver code might be used by raw C code.

I was just pondering this, my LED controller firmware uses https://github.com/pljakobs/RGBWWLed which implements 10 bit PWM fades for Esp8266 and does so pretty well (along with color space conversion and color correction).
Ideally, I would love to use the ledc_ functions that esp32 already offers for that, but that would require architecture specific implementations here as well.
How about we #define a set of capabilites for the pwm driver for a platform (like PWM_HAS_PHASE_SHIFT, PWM_HAS_SPREAD_SPECTRUM and PWM_HAS_FADE) to be used by user code to decide if they can use a specific interface extension? I admit right here that doesn't sound too clean, though.

How about we #define a set of capabilites for the pwm driver for a platform (like PWM_HAS_PHASE_SHIFT, PWM_HAS_SPREAD_SPECTRUM and PWM_HAS_FADE) to be used by user code to decide if they can use a specific interface extension? I admit right here that doesn't sound too clean, though.

I'd prefer not to introduce swathes of feature flags if possible. In C++ I think we'd probably define a separate interface for advanced functions, and that would be on a per-architecture basis. The esp32 IDF includes many useful capability flags (soc_caps.h) which can be useful.

The usePhaseShift parameter (or even a variable phaseShift setting) can be considered optional as it won't prevent less capable hardware from working, albeit sub-optimally.

So with that in mind perhaps a slight revision:

class HardwarePWM
{
    /**
     * @brief Configuration for extended constructor
     *
     * Allows use of additional hardware feature for those platforms which support it.
     */
    struct Options {
        /**
         * @brief ESP32 (and possible RP2040) support phase shifting to control EMI
         */
        bool usePhaseShift{false};
        /**
         * @brief Enable minimal spread spectrum modulation to further reduce EMI
         */
        bool useSpreadSpectrum{false};
    };

	/** @brief  Instantiate hardware PWM object
     *  @param  pins Pointer to array of pins to control
     *  @param  no_of_pins Quantity of elements in array of pins
     *  @param  options Archtecture-specific options to further refine operation
     */
	HardwarePWM(const uint8_t* pins, uint8_t no_of_pins, const Options& options);
};

One other question is whether we might want to adjust these options whilst PWM is running?

passing an architecture-specific options structure is a lean way to keep the current interface intact.
If I was to further extend the interface though, let's say fade-functions (thinking about pulling the per-channel fades from the RGBWWLed code into the HardwarePWM driver for esp8266, too) - wouldn't it be more flexible to make the current class definition an interface only and have the HardwarePWM inherit from it? AdafruitGFX is built like this, providing a pretty consistent API across all hardware implementations.
This way, the child classes could also implement new member functions to better expose hardware capabilities and where there are similar capabilities, they would still be abstracted in a common API.

so what I'm thinking is:
Core/IHardwarePWM.h:

class IHardwarePWM
{
  //common interface definition for all platforms - essentially the current ESP8266 function set
}

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

struct PWM_Options{
    bool usePhaseShift = false; // Use ledc's hpoint phase shift feature to stagger the rising edge of the PWM signals evenly across the period. This is useful for driving multiple LEDs connected to the same power rail to reduce the peak current draw and help with electromagnetic interference.
    bool useSpreadSpectrum = false; // implement minimal spread spectrum modulation to further reduce EMI. This feature uses a hardware timer to modulate the base frequency of the PWM signal. Depending on the base frequency, this may cause quite some system load, so choosing a good balance between CPU resources and acceptable EMI is important.
};

architecture specific class definition and implementation:
Arch//Core/HardwarePWM.h

 class HardwarePWM : public IHardwarePWM
 {
 public:
     /** @brief  Instantiate hardware PWM object
      *  @param  pins Pointer to array of pins to control
      *  @param  no_of_pins Quantity of elements in array of pins
      *  @param  usePhaseShift Parameter ignored on ESP8266 (present for API compatibility)
      */
     HardwarePWM(const uint8_t* pins, uint8_t no_of_pins);

This would, I guess, on one hand maintain compatibility with all current code while providing the freedom to extend the interface for specific hardware capabilities and/or software based implementations.

Or is it too large a change?

I will close this and continue in #2956 which is based on a larger rewrite