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.