Some things about PWM driver

[albi]

Hi, Giovanni

I've find some imprecisions about the (current) pwm driver:

in the PWM_COMPUTE_ARR(pwmclk, pwmperiod) macro, the pwmclk argument means the desidered timer counter clock, but the real one achieved can be considerably different because the truncation of division in the psc configuration and then an significantly different pwm period from that indicated in pwmperiod argument.
my code arrangment (a new macro):

Code: Select all

#define _TIMFREQ(clksrc, pwmclk)  ((clksrc)/((clksrc)/(pwmclk)))
#define _PNS2FRQ(pwmperiod)       (1e9/(pwmperiod))

#define PWM_COMPUTE_ARR_PNS(clksrc, pwmclk, pwmperiod)                      \
  ((uint16_t)((_TIMFREQ(clksrc, pwmclk) / _PNS2FRQ(pwmperiod)) - 1))

in the macro PWM_FRACTION_TO_WIDTH(pwmp, numerator, denominator) the names "numerator" and "denominator" are swapped compared with calculation (only the means of argument names, the calculation is correct)

I've integrated in to pwm driver the support of complementary outputs of TIM1, and some little improvements.
In this job i've used (and experimented) a two step inclusion of pwm_lld.h in pw.h to risove the the problem of 'hen egg, for add the low level parts of types definition to high level part of the same.
Because the low level driver include file, is called only from hig level driver, i think that this approach is secure.
IMHO, i think it can be usefull in the chibiOS driver structure.
For your convenience, i add my files, you can give them a look and tell me what you think
http://www.megaupload.com/?d=MMVJ9JUO
(files updated at 30/03 15:37)

Alberto

[Giovanni]

Hi Alberto, I will verify everything in the weekend, probably this has impact also in the ICU driver I am working on.

BTW, you can attach files to your posts, no need to use other sites.

Giovanni

[Giovanni]

Probably be I am missing something, can you make an example of the three parameters where the old macro fails to produce the correct result and the new one succeed?

Basically you replaced pwmclk with ((clksrc)/((clksrc)/(pwmclk))) that is equivalent to pwmclk as in the original macro assuming that pwmclk is the intended value.

I will comment on the other changes later.

Giovanni

[albi]

In general, it is not an bug, but only of a situation could be improved

I make as example an borderline case:

suppose STM32_TIMCLK1 equal to 48MHz

/* 25MHz PWM2 clock frequency. */
#define PWM2_CLK 25e6

and in the configuration...

.pc_psc = PWM_COMPUTE_PSC(STM32_TIMCLK1, PWM2_CLK),
.pc_arr = PWM_COMPUTE_ARR(PWM2_CLK, 1000000)), /* PWM period 1mS (in nS). */

The psc division factor will be 1 (48e6/25e6 = 1), the PWM_COMPUTE_ARR macro use as first argument the value of PWM2_CLK, due to integer division, the nearest value of timer counter colck will be 48MHz and not 25MHz as desidered, but the ARR value will be 25000 and the achieved pwm period will be 521uS (1920 HZ instead of 1000Hz)

Using the PWM_COMPUTE_ARR_PNS(clksrc, pwmclk, pwmperiod) macro we can avoid this error

.pc_arr = PWM_COMPUTE_ARR_PNS(STM32_TIMCLK1, PWM2_CLK, 1000000)

the ((clksrc)/((clksrc)/(pwmclk))) part of macro calculate the exact value of timer counter clock as the effective output of prescaler and then the calculate pwm period will be exact.

Alberto

[Giovanni]

I see your point, the PWM clock was not supposed to be a rounding of the desired value but one of the values that is possible to obtain using the timer prescaler.

The idea could also be used but I was thinking to change the GPT, ICU and PWM drivers to be all configured in the same way (and possibly share some code related to hardware timers). I am oriented to modifying the ICU and PWM drivers to use the configuration method currently used in the GPT.

The timer clock is specified in the configuration structure simply as frequency (not as a value of the PSC register as it is now), this requires no conversion macros BUT moves a 32bits integer division into the driver code, not a huge problem in exchange of simplicity. The GPT driver refuses impossible frequency values by using assertions so rounding is simply never allowed, you have to specify a possible frequency.

Another change is to specify the cycle period as number of ticks rather than ARR value (almost the same but not the same), the macros will perform conversions. This will somehow break compatibility but will make configuration much simpler and will increase consistency among the drivers.
The above change has also the advantage to make the initialization structure for those drivers mostly platform independent.

About the TIM1 handling, it is a good idea but are we supposed to do bit operations on enumeration fields? probably the field will have to become a normal integer and the symbols will have to become #defines. I know the C allows it but it looks not so clean.

Finally, the double invocation of the low level header, too complicated IMO, I prefer to have the modifiable declarations into the low level header. Low and high level headers are in the same page into the documentation anyway so the information is presented in the correct context. Another problem is that to implement such a change we would have to change all the device drivers in all platforms and all the templates, too much effort and too high the odds of adding new bugs.

About the exchanged parameters, thanks for finding the error.

Giovanni

[Giovanni]

Hi Alberto,

I merged the changes regarding BTRD and complementary outputs to the trunk, I made some optimizations and changes required because the names changes into the 2.3.x drivers.

The change to the macros has been made unnecessary by the new configuration structure (now very similar between the GPT, ICU and PWM drivers).

Do you mind giving a try to the merged features?

Giovanni

[albi]

Do you mind giving a try to the merged features?

Sure! the next week.

Alberto

[albi]

Hi Giovanni
why, in pwm_lld_change_period, have you disabled the channels when update the pwm frequency?

In general, in power conversion applications
a pwm channel, could be used in fixed frequency and variable duty-cycle mode but also in fixed "time on" and variable frequency mode.
I think that the pwm frequency must be varied on the fly in the same manner of duty-cycle
Alberto

[Giovanni]

It is because it would have to recalculate the duty cycle for all channels, the information is not stored in the driver but only into the comparator registers. It would be possible to scale the values into the comparators but there would be issues with rounding or truncation.

The setting is effective starting next cycle so there should be the time to re-enable the channels too, depending on when the change is started.

Of course if you have an idea about this, please go ahead and let's discuss it.

Giovanni

[Giovanni]

Uhm, about the fixed time-on, is it a good idea to assume it as default for variable frequency mode? supporting this mode would be simple.

Giovanni