PWM driver refactorization?

[michallenc]

I was looking at PWM driver API interface because of one project and some possible changes have come to my mind. The current API utilizes two structures, pwm_info_s and pwm_chan_s with the latter being used only if CONFIG_PWM_MULTICHAN is set. It basically looks like this:

#ifdef CONFIG_PWM_MULTICHAN
struct pwm_chan_s
{
  ub16_t duty;
#ifdef CONFIG_PWM_OVERWRITE
  bool ch_outp_ovrwr;
  bool ch_outp_ovrwr_val;
#endif
#ifdef CONFIG_PWM_DEADTIME
  ub16_t dead_time_a;
  ub16_t dead_time_b;
#endif
  uint8_t cpol;
  uint8_t dcpol;
  int8_t channel;
};
#endif

struct pwm_info_s
{
  uint32_t           frequency; 
#ifdef CONFIG_PWM_MULTICHAN
  struct pwm_chan_s  channels[CONFIG_PWM_NCHANNELS];
#else
  ub16_t             duty; 
#ifdef CONFIG_PWM_DEADTIME
  ub16_t dead_time_a;   
  ub16_t dead_time_b; 
#endif
#  ifdef CONFIG_PWM_PULSECOUNT
  uint32_t           count;
  uint8_t cpol;
  uint8_t dcpol;
#endif /* CONFIG_PWM_MULTICHAN */
  FAR void           *arg; 
};

The disadvantages I see in this approach are that we have a different API for one channel and multiple channels (more ifdefs for portable application) and we repeat some fields in both structures, which makes the header a bit confusing. And I am a bit afraid this will get out of hand with possible other functionalities being implemented. Also there is a risk of new option being added to one structure and not to the other (which seems to be the case of CONFIG_PWM_OVERWRITE options).
My idea is to use pwm_chan_s for both single and multiple channel configuration option. The result would be something like:

#ifdef CONFIG_PWM_MULTICHAN
#define PWM_NCHANNELS CONFIG_PWM_NCHANNELS
#else
#define PWM_NCHANNELS 1
#endif

struct pwm_info_s
{
  uint32_t           frequency; 
  struct pwm_chan_s  channel[PWM_NCHANNELS];
  FAR void           *arg; 
};

This way the application would access through the same interface regardless of what option is set. Application for NuttX with single channel configured would just access channel[0], it could be implemented as a for loop from 0 to PWM_NCHANNELS and it would be
compatible with both configuration options without ifdefs. It would also simplify existing drivers a bit.

The obvious issue is that this is an external API and the change would break it in a hard way. The question is: is it worth it? Are these changes beneficial enough to break the API? Can we even break it?

[acassis]

Hi @michallenc I think this modification makes sense! Initially NuttX didn't have support to multichannel PWM, then it was added later as an aggregation. Your proposal fixes this thing. @raiden00pl, @pkarashchenko what do you think?

[acassis]

ping @raiden00pl
ping @pkarashchenko

[raiden00pl]

+1 for this change. I came to similar conclusions in the past, but I didn't have a chance to correct it. It'll be a breaking change, but easy to fix and obvious (compilation error). I think a clean and consistent API is worth it.

EDIT: I think this can also help simplify the logic in lower-half pwm where we have a lot of #ifdef CONFIG_PWM_MULTICHAN.

[dry-75]

@michallenc
Hello Michal,

Good plan,  currently multi-channel support is awkward, and perhaps messy with ifdefs. Please consider also the following.

 - 'Advanced' peripherals where each channel's frequency can be different, for per channel (to be clear, want to change frequency/period also per channel) We would like option to treat is as one device, and potentially pass arguments to set / configure few channels at once per call, and not do N calls.  Maybe a list of settings to pass.
 
 - The frequency, and duty types  - uint32_t, and uint16_t - are awkward , in some use cases. As this is a higher /upper level interface of the driver, it would/could be nice to specify frequency and dc in exact units. And then handle the exact conversions to an unsigned of fixed bitwidth to match  (a specific) hardware register at the lowest level of the driver which knows all specifics and the state of the peripheral ( clocking details, register widths, etc..)
 
 E.g;  I would like to specify & pass to the interface  frequency of (thumb suck): 31.7686Khz. In user code, I would not want to re-scale / multiply by factor of N to pass this in as a mandated "uint32_t" type, only to potentially un-do that again in the lower part of the driver.
 
 At least for some uses, it would be nice to specify these as floats. Thus, please consider if these types should be typedef'ed per user system configuration (i.e., the Nuttx config).  
 
  - The 'implementation defined'  The FAR void           *arg;
 This could / is currently a somewhat dangerous member.  The current PWM upper part of the driver  (at least last I saw),  in set characteristics of ioctl copies entire pwm_info_s passed from user which is great.  But the arg member now may or may not be pointing to something valid in user memory, and it may become invalid after the user is done with the call.
 
 Perhaps there could be a better / safer way to handle this, by requiring also to pass a size parameter so the whole blob may be copied similarly to the struct pwm_info_s.  Maybe, upper may ask lower if it wants to copy it, if it ever expects such an argument.

[michallenc]

'Advanced' peripherals where each channel's frequency can be different, for per channel (to be clear, want to change frequency/period also per channel) We would like option to treat is as one device, and potentially pass arguments to set / configure few channels at once per call, and not do N calls. Maybe a list of settings to pass.

Frequency per channel is a reasonable requirement and it can be simply achievable by putting the frequency field to pwm_chan_s structure from the generic one. The option to change just some channels is already present, you may use values 0 (skip this channel and not configure) and -1 (skip all following channels) for channel numbers. It is not documented properly because NuttX does not have a proper peripheral documentation, but little description is at least in the header file https://github.com/apache/nuttx/blob/master/include/nuttx/timers/pwm.h#L69.

The frequency, and duty types - uint32_t, and uint16_t - are awkward , in some use cases. As this is a higher /upper level interface of the driver, it would/could be nice to specify frequency and dc in exact units. And then handle the exact conversions to an unsigned of fixed bitwidth to match (a specific) hardware register at the lowest level of the driver which knows all specifics and the state of the peripheral ( clocking details, register widths, etc..

Yes, the passing convention of duty cycle and frequency is a mess. As you also mentioned, it would be ideal from API perspective to set frequency in Hz/kHz and duty cycle probably in percentage (with 0.1 resolution probably?).

We may have special pwm_freq type that would be either some uint or float, but it should definitely be configurable as NuttX supports devices without native float/double.

The 'implementation defined' The FAR void *arg;

I honestly have never used this field so I need to check how it is used in current NuttX code. In general I am not even sure if this should be present in pwm_info_s and PWMIOC_SETCHARACTERISTICS at all. I personally consider having controller specific ioctl calls that are passed to lower layer if not recognized by upper layer as a better choice.

[acassis]

@michallenc I think some PWM controller with multichannel doesn't support change frequency per channel, only the duty cycle. So when adding the field freq to pwm_chan_s we need to consider this fact, maybe using freq = 0 to indicate it.

[michallenc]

@michallenc I think some PWM controller with multichannel doesn't support change frequency per channel, only the duty cycle. So when adding the field freq to pwm_chan_s we need to consider this fact, maybe using freq = 0 to indicate it.

Yes, that is a good point. We could perhaps keep frequency field in both pwm_info_s and pwm_chan_s. If pwm_info_s->frequency is set, then all channels would have this frequency. If set to zero (we can hide it under some nicely called define PWM_DEFINE_FREQ_PER_CHANNEL or something) then pwm_chan_s->frequency is used.

In general, it would be nice to get some controller capabilities to the application. How many channels are configured for given device? Does it have complementary outputs? Does it have dead time? Can we set frequency per channel? But I am afraid getting all of these will be a mess and a lot of ioctl calls without a device tree.

Also ping to @ppisa if he has some ideas.

[raiden00pl]

We may have special pwm_freq type that would be either some uint or float, but it should definitely be configurable as NuttX supports devices without native float/double.

I'm not a fan of changing the PWM interface to use floats. Even if it's protected by CONFIG_HAVE_FLOAT, it creates a non-portable interface (we usually avoid float when our chip doesn't have FPU), fixed-math in this case seems more appropriate.

Another problem with PWM I see is CONFIG_PWM_PULSECOUNT option. This functionality should be available per-timer instance, not enabled for all PWM drivers. It seems to me that it should be a separate driver altogether, because it has a different purpose than the general PWM. One generates a modulated signal, the other generates a finite sequence of pulses - two different functions and different applications.

[michallenc]

We may have special pwm_freq type that would be either some uint or float, but it should definitely be configurable as NuttX supports devices without native float/double.

I'm not a fan of changing the PWM interface to use floats. Even if it's protected by CONFIG_HAVE_FLOAT, it creates a non-portable interface (we usually avoid float when our chip doesn't have FPU), fixed-math in this case seems more appropriate.

Well we can make it uint16_t/uint32_t if CONFIG_HAVE_FLOAT is not set and it would remain the same as now. The problem here is PWM drivers would have to behave accordingly based on what approach is used, so some more ifdefs. The application portability is an issue even with the fixed arithmetic because of a different PWM resolution, the calculation has to be changed based on the used MCU. And I think there is currently no way to get PWM resolution via some config/API. That might be problem for some tools and libraries supporting NuttX on many different MCUs, pysimCoder to name one.

Another problem with PWM I see is CONFIG_PWM_PULSECOUNT option. This functionality should be available per-timer instance, not enabled for all PWM drivers. It seems to me that it should be a separate driver altogether, because it has a different purpose than the general PWM. One generates a modulated signal, the other generates a finite sequence of pulses - two different functions and different applications.

The separation sounds good, I think the implementation similar to standard timers would fit this better. It's however quite a big change, I counted 13 drivers with CONFIG_PWM_PULSECOUNT.

[raiden00pl]

Well we can make it uint16_t/uint32_t if CONFIG_HAVE_FLOAT is not set and it would remain the same as now. The problem here is PWM drivers would have to behave accordingly based on what approach is used, so some more ifdefs. The application portability is an issue even with the fixed arithmetic because of a different PWM resolution, the calculation has to be changed based on the used MCU. And I think there is currently no way to get PWM resolution via some config/API. That might be problem for some tools and libraries supporting NuttX on many different MCUs, pysimCoder to name one.

Kernel API should be the same regardless of the architecture used (with or without FPU). Or at least the numeric types used in the API should be compatible, so the same code using type A will also work with type B. That is what portable interface means to me. Of course, there may be cases where this is not possible, but such a solution should be the last resort.
Why not simply set frequency unit to mHz, 0.1Hz or any other practical value? If we ever need high frequency support (GHz ranges) then we can just change the frequency type to uint64_t. In the future, since POSIX now requires 64-bit time, support for uint64_t will probably become mandatory in NuttX, so every architecture will have to support this type anyway.

The separation sounds good, I think the implementation similar to standard timers would fit this better. It's however quite a big change, I counted 13 drivers with CONFIG_PWM_PULSECOUNT.

it's not that bad, grep -R PWM_PULSECOUNT lies here :) Only stm32 families, at32 (which is clone of stm32 chips) and tiva supports this feature. Stm32 ports use the same PWM implementation, so the changes will most likely be identical. This task is not easy, but it's not as big as it seems at first glance.

[ppisa]

I do not have bullet proof suggestion. There is what comes to my mind:

I agree that interface to the operating system should avid float as much as possible. The real hardware is a discrete system with some resolution, sometimes frequency, PLL etc. can be defined by ration which could complicate the use single number to specify the value. But for frequencies, it is not common to use some really small ones, so resolution of 1 Hz is OK. As for the structure, I think that it is better to have some idea about channels grouping and mapping to lower level peripherals so using single level (one minor number) to specify PWM channel across whole system hides important information. Grouping is critical in cases when parameters have to be updated at once, which is possible for some hardware for channels within the group. Mutual channels synchronization for example for motor control is critical as well. In that cases, usually even synchronization with ADC conversions is important. Another case are devices which has only single time base (and and as result frequency) for channels within the group.

So it would be useful to provide some API which allows to retrieve structure and above discusses capabilities for devices and channels. May it be, that if retrieve of these information is possible than final PWM channels could be mapped into linear space.... But we have use for special IOCTLs for some devices providing channels groups already....

So I would agree with frequency unit 1 Hz and for special cases there could be alternative interface with micro Hz in 64-bit number for example. When the value is specified then it should be rounded and it should be possible to read rounded value back. On the other hand, there is problem that actual resolution of duty cycle is critical in many cases.... So most appropriate interface is then to provide hardware maximal duty cycle resolution, then list of possible PWM peripheral base frequencies, range for prescaler, if it is linear or power of 2 and range of divisors and left the rest to compute to user of peripheral. But for the simple applications that gets too complicated...

[raiden00pl]

@ppisa the question is whether it's even possible to create a general interface that can support all complex PWM cases?
It seems to me that this is not possible without significantly complicating the entire interface. In addition, many of these use cases are highly architecture specific.

Motor control is a perfect example here. In that case we have to control duty cycle (and ideally update all duty at once) and get ADC samples that are in sync with this PWM. Additionally, there are issues such as current sense path calibration or selecting appropriate PWM mode, which is highly architecture specific.
I think in such cases using a general PWM and ADC drivers is a bad solution. That's why in the FOC implementation in NuttX, there is a separate upper-half driver for FOC that combines in a single driver PWM, ADC, and synchronization mechanism for user space (https://github.com/apache/nuttx/tree/master/include/nuttx/motor/foc).

It should be possible to generalize this implementation even more and create a general "adcpwm.c" driver that implements in a single device n-channels PWM with m-channels ADC and synchronization for application. Two separate drivers in this case doesn't look good for me (/dev/adcX and /dev/pwmX), because the coupling between these two is too large and one is basically useless without the other.