PWM driver refactorization?
- Dominant language
- C
- Stars
- 4k
- Forks
- 1.7k
- Avg merge
- 1d 17h
- Merged PRs (30d)
- 237
Description
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?
Contributor guide
Assessment
This issue has not been assessed yet.