Skip to content
This repository has been archived by the owner on Feb 4, 2023. It is now read-only.

Duty cycle as integer rather than float #6

Closed
Laserjones opened this issue Apr 16, 2022 · 19 comments
Closed

Duty cycle as integer rather than float #6

Laserjones opened this issue Apr 16, 2022 · 19 comments
Labels
enhancement New feature or request

Comments

@Laserjones
Copy link

As far as I understand, currently the duty cycle needs to be specified as a float value bewteen 0.0 and 100.0 (percent), which is then turned into a counter compare value (integer) by the library. This means that several floating point operations are required each time the duty cycle is changed. This can add up to quite a lot of floating point operations if PWM is used, for example, to output analog waveforms at high frequencies, where the duty cycle can change thousands of times per second. And as far as I understand, these operations consume quite some time, as the RP2040 does not have a floating point unit.

Could it therefore make sense to add an option to provide the duty cycle as an integer directly (i.e. between 0 and the TOP value)? In many cases, e.g. when waveforms are read from a lookup table of samples, the programmer already knows the absolute integer values anyway, and it would make little sense to first convert them into a float percentage value and then immediately reverse that conversion in the library.

An error should then be issued if the duty cycle value exceeds the TOP value.

@khoih-prog
Copy link
Owner

khoih-prog commented Apr 16, 2022

This can add up to quite a lot of floating point operations if PWM is used, for example, to output analog waveforms at high frequencies, where the duty cycle can change thousands of times per second.

You're correct about this. I didn't think that this demanding use-case would use this library at all, but would write the registers directly using look-up table.

But the idea is interesting and I'll add this function

bool setPWM_manual(uint8_t pin, uint16_t top, uint8_t div, uint16_t level, bool phaseCorrect = false)

to bypass all the calculations / displays.

An error should then be issued if the duty cycle value exceeds the TOP value.

If level/duty_cycle > TOP, it will be trimmed automatically to TOP.


You have to prepare a lookup table similar to

typedef struct 
{
  uint16_t top;
  uint8_t div;
  uint16_t level;
} PWD_Data;

PWD_Data PWM_data[NUM_PWM_POINTS] =
{
  { top1, div1, level1 },
  { top2, div2, level2 },
  ......
};

then call the function manually like

for (int index = 0; index < NUM_PWM_POINTS; index++)
{
  setPWM_manual(pin, PWM_data[index].top, PWM_data[index].div, PWM_data[index].level, true);
  // delay something here
  ...
}

Is this OK to you ??

I also need you to test using the current version to see how the float calculation would affect the waveform. Then compare to the new release to be sure if it's much better and worth to use.

I'll publish the new release within some hours.

khoih-prog added a commit that referenced this issue Apr 16, 2022
### Releases v1.2.0

1. Add efficient `setPWM_manual()` function to use in wafeform creation using PWM. Check [Duty cycle as integer rather than float #6](#6)
2. Add example [PWM_Waveform](https://github.com/khoih-prog/RP2040_PWM/tree/main/examples/PWM_Waveform) to demonstrate how to use new `setPWM_manual()` function in wafeform creation
3. Optimize library code and examples by using **reference-passing instead of value-passing**.
khoih-prog added a commit that referenced this issue Apr 16, 2022
### Releases v1.2.0

1. Add efficient `setPWM_manual()` function to use in wafeform creation using PWM. Check [Duty cycle as integer rather than float #6](#6)
2. Add example [PWM_Waveform](https://github.com/khoih-prog/RP2040_PWM/tree/main/examples/PWM_Waveform) to demonstrate how to use new `setPWM_manual()` function in wafeform creation
3. Optimize library code and examples by using **reference-passing instead of value-passing**.
khoih-prog added a commit that referenced this issue Apr 16, 2022
### Releases v1.2.0

1. Add efficient `setPWM_manual()` function to use in wafeform creation using PWM. Check [Duty cycle as integer rather than float #6](#6)
2. Add example [PWM_Waveform](https://github.com/khoih-prog/RP2040_PWM/tree/main/examples/PWM_Waveform) to demonstrate how to use new `setPWM_manual()` function in wafeform creation
3. Optimize library code and examples by using **reference-passing instead of value-passing**.
@khoih-prog
Copy link
Owner

Hi @Laserjones

The new RP2040_PWM releases v1.2.0 has just been published. Your contribution is noted in Contributions and Thanks

Try and modify the new PWM_Waveform example according to your use-case and report back if there is any bug to fix or enhancement to add

Best Regards,


Releases v1.2.0

  1. Add efficient setPWM_manual() function to use in wafeform creation using PWM. Check Duty cycle as integer rather than float #6
  2. Add example PWM_Waveform to demonstrate how to use new setPWM_manual() function in wafeform creation
  3. Optimize library code and examples by using reference-passing instead of value-passing.

@khoih-prog khoih-prog added the enhancement New feature or request label Apr 16, 2022
@Laserjones
Copy link
Author

Wow, the three coffees I bought you really kept you awake! :-D

This will surely help, thanks a lot! I'll check it out in detail later.

I'm just wondering whether it is actually necessary to pass all four parameters each time (which probably means that four registers are written to each time, too?). Maybe I underestimate the scope of possible PWM applications, but all use cases I can imagine would require only the duty cycle to be changed frequently, whereas the TOP and the frequency would be set only once to initialize the PWM output (not sure about the phase correction, as I have no idea what its practical use is). So three out of four registers would unnecessarily be written to in spite of staying unchanged.

So, from a humble Arduino programmer's point of view, who likes everything as simple as possible, I would wish for something like:

PWM_init(uint8_t pin, uint16_t top, float frequency, uint16_t startlevel, bool phaseCorrect) // to be called once

PWM_write_absolute(uint8_t pin, uint16_t level) // level as integer, 0 to counter TOP
PWM_write_percent(uint8_t pin, float level) // level in percent, 0.0 to 100.0

That would be close to the familiar AnalogWrite() function of Arduino, which basically does the same (but in software with only 256 duty cycle values).

@Laserjones
Copy link
Author

Even regardless of any performance advantage, I find it definitely useful to have this option of duty cycle as integer, because - as mentioned - in many applications the values used by the programmer will be integers of a known range anyway, so the conversion to float percent and back not only adds unnecessary computing load, but also unneccesary calculations in the program code. For example, audio waves usually are simply an array of 8-bit or 16-bit values, which can directly be handed over to the PWM output if the TOP is configured to be above the maximum wave amplitude. No need for percent values.

@khoih-prog
Copy link
Owner

This is the reason why I use float instead of uint16_t for duty-cycle

khoih-prog/ESP8266_PWM#1 (comment)

I'm similarly trying to change the duty cycle with a fixed frequency. Since PWM_DutyCycle is a uint32_t , this can only be done in 100 steps. I suggest to have an alternative function where you can specify Period/OnTime to be able to get the full duty cycle resolution. Or change PWM_DutyCycle to a float (I think 0. to 1. would be easier to use than 0-100 if you do this).

Using uint16_t only is wasting the resolution of TOP 16-bit register.

I think I'll use either

  1. uint16_t with 100 * duty_cycle => 50% == 50 * 100 = 5,000
  2. uint32_t with 1000 * duty_cycle => 50% == 50 * 1000 = 50,000

I prefer option 2. to have perfect resolution without adding burden to the MCU

@khoih-prog
Copy link
Owner

I'm just wondering whether it is actually necessary to pass all four parameters each time (which probably means that four registers are written to each time, too?). Maybe I underestimate the scope of possible PWM applications, but all use cases I can imagine would require only the duty cycle to be changed frequently, whereas the TOP and the frequency would be set only once to initialize the PWM output (not sure about the phase correction, as I have no idea what its practical use is). So three out of four registers would unnecessarily be written to in spite of staying unchanged.

It's possible to optimize like that, but I'm against the idea of modifying the individual register by accessing it directly. It's always better to use the SDK-provided functions to be compatible across platforms and releases.

It's always possible the core's functions has been optimized to modify the registers only if changed.

If the use-case is so demanding to be optimized up to each register access, you have to write the code to access the registers directly, without using the core SDK and the library. Even using assembly code, not C/C++.

So. I won't go to that extreme and make the library so hard to use.

Anyway, don't hesitate to propose the new ideas. Without discussion, we all have the tunnel-vision and don't have a clear picture of what's better.

@Laserjones
Copy link
Author

It's possible to optimize like that, but I'm against the idea of modifying the individual register by accessing it directly. It's always better to use the SDK-provided functions to be compatible across platforms and releases.

As far as I can see, there's no need to access registers directly, because the SDK provides functions for each parameter separately (https://raspberrypi.github.io/pico-sdk-doxygen/rp2__common_2hardware__pwm_2include_2hardware_2pwm_8h.html). In fact, looking at your code, it seems like you are doing just that: Your function receives all the parameters at once, but then sets each register individually with SDK calls such as pwm_set_gpio_level(...).

It's always possible the core's functions has been optimized to modify the registers only if changed.

I hope so. But your function still has to handle all the parameters, even if the SDK functions then decide that the registers need not be touched. On the other hand, duplicating all the individual SDK functions in your library would not make a lot of sense. Though at least for the duty cycle, I'd prefer a function that takes care of only this one parameter.

So. I won't go to that extreme and make the library so hard to use.

Actually my thoughts were intended to make it easier to use, not harder. ;-)

But I get your point, and the more I look at your code, the more I think that I might as well call the SDK functions directly, or maybe write my own PWM library, which would be a nice challenge.

By the way, could you help me understand why you are able to call the SDK functions directly in your code without any #include or prefix? As far as I understand the explanation at the bottom of https://github.com/arduino/ArduinoCore-mbed, an mbed:: prefix should be required.

Phew, I never thought I would dive so deeply into this stuff after only some weeks of programming the Pico, with almost no previous C++ or Arduino experience. A steep curve, but definitely fun.

@Laserjones
Copy link
Author

By the way, could you help me understand why you are able to call the SDK functions directly in your code without any #include or prefix?

Sorry, I overlooked #include "hardware/pwm.h" in your code. I assume that's the answer?

@khoih-prog
Copy link
Owner

It's so good that you dive deeply into the SDK, core, library, etc. and had a lot of fun and experience thru it. You also make me reread them as I almost forgot after some time not looking at.

The difference is just these 2 lines to program the config

RP2040_PWM/src/RP2040_PWM.h

Lines 206 to 207 in 51e1826

pwm_config_set_clkdiv_int(&config, _PWM_config.div);
pwm_config_set_wrap(&config, _PWM_config.top);

I can create a new function to let you not passing the top and div, just the level as

bool setPWM_manual(const uint8_t& pin, uint16_t& level, bool phaseCorrect = false);

The compiler will auto selects the correct function accordingly.

A new release will be published soon with these features

  1. uint32_t with 1000 * duty_cycle => 50% == 50 * 1000 = 50,000
  2. this new setPWM_manual function

khoih-prog added a commit that referenced this issue Apr 17, 2022
### Releases v1.3.0

1. Add `setPWM_manual(pin, level)` function for efficiency in wafeform creation using PWM. Check [Duty cycle as integer rather than float #6](#6)
2. Add example [PWM_Waveform_Fast](https://github.com/khoih-prog/RP2040_PWM/tree/main/examples/PWM_Waveform_Fast) to demonstrate how to use new `setPWM_manual(pin, level)` function.
3. Add `setPWM_Int()` function for optional `uint32_t dutycycle = real_dutycycle * 1000`. Check [Duty cycle as integer rather than float #6](#6)
4. Add example [PWM_DynamicDutyCycle_Int](https://github.com/khoih-prog/RP2040_PWM/tree/main/examples/PWM_DynamicDutyCycle_Int) to demonstrate how to use new `setPWM_Int()` function.
5. Rewrite many functions to take advantage of new features.
@khoih-prog
Copy link
Owner

Hi @Laserjones

The new RP2040_PWM releases v1.3.0 has just been published. Your contribution is noted in Contributions and Thanks

Try and modify the new PWM_Waveform_Fast example according to your use-case and report back if there is any bug to fix or enhancement to add

Best Regards,


Releases v1.3.0

  1. Add setPWM_manual(pin, level) function for efficiency in wafeform creation using PWM. Check Duty cycle as integer rather than float #6
  2. Add example PWM_Waveform_Fast to demonstrate how to use new setPWM_manual(pin, level) function.
  3. Add setPWM_Int() function for optional uint32_t dutycycle = real_dutycycle * 1000. Check Duty cycle as integer rather than float #6
  4. Add example PWM_DynamicDutyCycle_Int to demonstrate how to use new setPWM_Int() function.
  5. Rewrite many functions to take advantage of new features.

@Laserjones
Copy link
Author

Thank you very much! I'll check it all out when I'm ready. First I'll need to implement some hardware in order to actually monitor the waves (PWM output filter, PC audio interface, software oscilloscope). Can take a while, as this is just a hobbyist project at this time.

@Laserjones
Copy link
Author

Laserjones commented Apr 17, 2022

I don't really understand this part:

uint32_t with 1000 * duty_cycle => 50% == 50 * 1000 = 50,000

The hardware PWM counter (and thus the maximum PWM resolution) is 16-bit, so passing the duty cycle as a 32-bit value should not bring an advantage regarding resolution. So if we pass the DC directly as an integer (capture compare value), uint16_t would make most sense, and if we pass it as a percentage value, float would make most sense (to be converted into uint16_t mathematically). I don't see any need for uint32_t here. But my feeling is that I didn't get your point.


EDIT: It just dawned on me that maybe you want to provide an option to pass the DC as an integer instead of float, but still as a percentage value rather than the direct capture compare value?

Example: If I want an 82.5% duty cycle, I'll pass an integer value of 82500? In that case, of course, 32 bits would be required for the full resolution. And it would avoid floating point operations. Nice idea.

I hadn't thought of such an option. When creating waveforms, I already know their range of possible values. For example, if my waves have a 16-bit resolution, I'll set the TOP to 65535 (max. 16-bit range) and pass the wave samples to PWM as they are without thinking about percentage values. A level value of 32767 would then correspond to a 50% duty cycle.

@khoih-prog
Copy link
Owner

That's very good you can read my idea. Certainly you must have an investigative mind, then just take some short time to read the library code and figure out.

For normal user, it's easier to specify DC, than has to calculate / map the DC to 16-bit value. And the uint16_t can't store 100% * 1000 = 100,000.

Why DC * 1000? I'd like to take advantage of the full resolution of 16-bit TOP register. If DC * 100, we have max 100% * 100 = 10000, so we'll loose the full 16-bit resolution.

IMO, with the 32-bit MCU, using the native uint32_t will even be faster than uint16_t.

@Laserjones
Copy link
Author

I may have caused all the confusion myself by writing "Duty cycle as integer rather than float", while what I actually meant was "Pass the 16-bit capture compare value directly as an integer rather than calculating it from a float percentage value". For me, "duty cycle" and "capture compare value" were the same thing, but of course, technically the duty cycle is always a percentage value, whereas the capture compare value is what is actually written to the register. So I might have accidentally triggered an improvement to the library that I didn't even intend. ;-)

@Laserjones
Copy link
Author

For normal user, it's easier to specify DC, than has to calculate / map the DC to 16-bit value.

That's true in many cases, e.g. if someone wants to control the brightness of an LED or the speed of a motor using PWM. They will automatically think in percentage values in that case.

But in my case, when creating waveforms from a table of samples, I'm dealing with the actual sample values anyway, and converting them to percentage and back would just be a detour. So it's definitely good to have both options.

@khoih-prog
Copy link
Owner

You have to calculate once anyway and store in the the lookup table, not directly for every samples. So calculate either the 16-bit level or DC is almost similar task.

@Laserjones
Copy link
Author

You have to calculate once anyway and store in the the lookup table, not directly for every samples. So calculate either the 16-bit level or DC is almost similar task.

I'm not sure what you mean. If I know that all my samples are 16-bit values and the TOP is set to the maximum 16-bit range, I can write the values of my samples directly to the capture compare register – no need for additional calculations. Correct me if I'm wrong.

For example, for a sawtooth wave (45-degree ramp upwards), I would use an array of 65536 16-bit values, where wave[0]=0, wave[1]=1, wave[2]=2, etc. – and then repeatedly pass these as level values to the PWM without any conversion.

image

@khoih-prog
Copy link
Owner

That's a special use-case in waveform creation, where you can have the level ramping up value directly in your mind. 1-2-3-...

How about other use-cases when you can have to calculate the level and put in a lookup table?

But anyway, it's so easy for me to add a function for that simple direct level use-case. But possible not urgent now as I have to do something else more interesting. Wait for next release.

@Laserjones
Copy link
Author

That's a special use-case in waveform creation, where you can have the level ramping up value directly in your mind. 1-2-3-...
How about other use-cases when you can have to calculate the level and put in a lookup table?

I only used this as a simple example. Of course, in most cases, I would either need to calculate the samples or use a prepared lookup table (for a sine wave, for example). Of course, I could then as well write percentage values to that table rather than absolute values, but it would have no advantage at all – on the contrary: For full resolution, I would then need 32-bit or float values in my table rather than 16-bit, for the reasons you explained above. That would consume twice the memory.

But anyway, it's so easy for me to add a function for that simple direct level use-case. But possible not urgent now as I have to do something else more interesting. Wait for next release.

I thought that you had already done just that with the setPWM_manual function. But in fact, a new function is not really necessary for me, as your code showed me that I could easily use a direct SDK call of pwm_set_gpio_level(). Of course, for users not wanting to deal with the SDK, it would still be convenient to add such a library function (e.g. setPWM_directlevel) and document it.

Well, I now understand why you said at the beginning that users like me would rather use the SDK directly anyway. ;-)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants