-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PWM driver refactorization? #12381
Comments
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? |
ping @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 |
@michallenc 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. |
Frequency per channel is a reasonable requirement and it can be simply achievable by putting the
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
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 |
@michallenc I think some PWM controller with multichannel doesn't support change frequency per channel, only the duty cycle. So when adding the field |
Yes, that is a good point. We could perhaps keep frequency field in both 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. |
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
andpwm_chan_s
with the latter being used only ifCONFIG_PWM_MULTICHAN
is set. It basically looks like this: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: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 becompatible 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?
The text was updated successfully, but these errors were encountered: