apache
PWM driver refactorization?
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?
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?
+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.
@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.
'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.
@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 I think some PWM controller with multichannel doesn't support change frequency per channel, only the duty cycle. So when adding the field
freqto 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.
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.
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 avoidfloatwhen 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_PULSECOUNToption. 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.
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.
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...
@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.
I agree that creating a general API for all PWM usages is basically impossible. I think we should just focus on some basic capabilities like frequency, duty cycle, polarity etc. And other functionalities would have to be either in a separate drivers as FOC or configured during a compile time or with chip specific ioctl calls. At least for now. If make a good design for basic capabilities now, we can always add ioctl for other stuff later.
I currently face a similar problem with SAMv7 PWM and its fault inputs. I would need some ioctl that would get me the status of the fault inputs and another that would clear them (passing a mask as an argument probably). Thought this could probably be a common ioctl for all PWM drivers. But still the configuration of these fault inputs would have to be from Kconfig (polarity, which input is assigned to which PWM etc.)
Regarding frequency and duty cycle types: setting a unit to mHz or something like that and passing unsigned integer seems like a good option to have a better resolution of these.
@raiden00pl, @michallenc I only confirm that design of universal API for all purposes is problematic. In this respect and respect of hardware defined grouping, I am OK with actual design with multiple PWMs controlled through one device. I would suggest to define some common IOCTLs which could be used by multiple hardware drivers. Mainly to query and check capabilities. There should be upper layer drivers as FOC etc. for combined uses.
On the other hand, I have even counterexample. Our LX_RoCoN which firmware is not (yet) based on NuttX, even that I have tested and added NuttX support into mainline. https://www.pikron.com/pages/products/motion_control/lx_rocon.html
It has linear space of 16 PWM channels implemented in FPGA. These channel sequence can be divided into groups from left to right according to motor{actuator connected, two consecutive for DC motor, three for PMSM/BLDC, four for stepper with or without feedback and one for special single PWM purposes. There is available electric commutation coprocessor ( Park and Clarke inverse and forward) which can process DQ PWM requests and accumulate and convert currents into DQ space. It is capable to support up to four channels. But even in this case I would lean to map the 16 channels to the one multichannel device and add some configuration API which could map groups to FOC or other peripherals. Even in this case, it is possible, that there are another PWM peripherals directly n the MCU etc. which do not belong to this group. So I would keep actual NuttX design.
But as I know, Linux kernel has single linear mapping of PWMs to device minors...
@michallenc , as for the unification of the structure, I would suggest small swap
#ifdef CONFIG_PWM_MULTICHAN
#define PWM_NCHANNELS CONFIG_PWM_NCHANNELS
#else
#define PWM_NCHANNELS 1
#endif
struct pwm_info_s
{
FAR void *arg;
uint32_t frequency;
uint32_t channels_filled;
struct pwm_chan_s channel[PWM_NCHANNELS];
};
Or even use open array member at the end, but it would be tricky for many users. Anyway, if the single channel is used in this case then the structure can be truncated. Or the structure should be defined in two levels pwm_info_head_s and then pwm_info_multi_s and pwm_info_single_s with the same head which would be compatible with single IOCTL. This way only overhead for the single channel devices or operation on multichannel devices is need to fill 1into channels_filled field and that single unsigned location per structure. If the args pointer is needed, I suggest to start with it to have dense packing of the structure on 64/bit systems. channels_filled can be even something shorter than uint32_t but this way it ensures no unfilled location in the request probably for all considered architectures. Other option is to make it something smaller uint16_t and leave there space for some flags or other info which could be usable in the future.
struct pwm_info_head_s
{
FAR void *arg;
uint32_t frequency;
uint32_t channels;
};
struct pwm_info_single_s
{
pwm_info_head_s head;
struct pwm_chan_s channel[1];
}
struct pwm_info_multi_s
{
pwm_info_head_s head;
struct pwm_chan_s channel[PWM_NCHANNELS];
}
The ifdefs are gone this way and peripheral drivers for the single channel can be left almost unchanged to actual state, only fields names change, would include one more dot.
We also have to think about different frequencies for channels. That is not supported in all MCUs, but some have the capability to set a different frequency for different channels. We initially discussed here that we could put frequency field to
struct pwm_chan_s, but this would cause a big overhead for MCUs with single possible frequency per channel (and even bigger if we would use uint64_t for better frequency resolution).
After a discussion with @ppisa, we could keep the frequency field in struct pwm_info_head_s and use channels field as a mask to which channels the field applies. This means the user would have to call two ioctls to set two different frequencies for some channels, but there would be no overhead regarding additional fields. And it is safe to assume the user will set frequency mostly during startup and only duty cycle at runtime.
A also agree with the addition of flags field to the header, it can come in handy. One that immediately comes to mind is a flag specifying if the frequency should be set. This would allow to use a single ioctl across all channels for duty cycle change even if these channels have different frequency. Similarly these can be applied to deadtime, default channel polarity and so on. And these are also for discussion, whether to put them together with a duty cycle and frequency or to a separate ioctl call. It's a trade of between a simple and efficient structure and more ioctl calls and one big structure in one ioctl call. The overhead for deadtime and polarity is not big now, but maybe it is worth separating it to avoid a bigger overhead if other fields are added in the future (the fault inputs come to my mind). But we should try not to introduce many unnecessary ioctl calls to API.
The suggestions mentioned above have one issue though. The current implementation of PWM upper layer starts the output immediately after PWMIOC_SETCHARACTERISTICS call. It does not wait for PWMIOC_START ioctl, these two are basically the same from the lower layer drivers point of view. It would have to be split into two different functions in the lower layer - one ioct call only sets the register, start ioctl then starts it. And it should be possible to modify it on run just with set characteristics ioctl. This is the design I would prefer anyway, but it complicates the rewrite as it affects the drivers a lot.
@acassis @cederom Hey guys, do you know how we stand with Apache on Google Summer of Code this year? I think the proposals will be open in the near future and this could be an interesting GSoC project. I will get into this even if we don't find anyone for GSoC, but probably not in the upcoming months anyway, so why not try GSoC if there will be interest.
Hey there @michallenc :-) Yes I have sent already a GSoC 2025 related email some time ago and reminder right now to our dev@ mailing list where proposals can be reported :-)
Here are Apache rules for GSoC https://community.apache.org/gsoc/ :-)
Please send proposal to the mailing list, we will note it on our wiki and report to Apache :-)
I started working on removing PWM_MULTICHAN some time ago: https://github.com/raiden00pl/nuttx/tree/pwm_refactor
IMO this doesn't seem like a very interesting topic for GSoc :) boring work with no room for creativity (at least the part where CONFIG_PWM_MULTICHAN needs to be removed).
I started working on removing PWM_MULTICHAN some time ago: https://github.com/raiden00pl/nuttx/tree/pwm_refactor
Nice!
IMO this doesn't seem like a very interesting topic for GSoc :) boring work with no room for creativity (at least the part where
CONFIG_PWM_MULTICHANneeds to be removed).
I don't think I completely agree. Sure, the driver's rewrite is a bit boring and a lot of repetitive coding, but the API design and implementation might be interesting. But I am not forcing the topic, we have a lot of other (and probably more important) topics as device tree for example.
Related: https://github.com/apache/nuttx/pull/15912 implementing fault clear IOCTL.