apache / apache/nuttx

PWM driver refactorization?

Open
#12,381 23 comments 0 reactions 2 assignees Claimed by @raiden00pl View on GitHub
breaking change
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.