PWM driver: pca9685_pwm_out improvement

[SalimTerryLi]

Describe problem solved by this pull request
Fixed some bugs about that driver. Listed here:

• Switched CDev name so that "/dev/pca9685" is pointed to I2C driver and "/dev/pwm_outputX" is pointed to the function wrapper
• Parameters doesn't take effect when change back to default -1.

Added new features:

• Now supports esc calibration executed from QGC(not being tested, for that my new ADC driver is not merged into upstream).But it works only if the driver is the only one that registered "/dev/pwm_outputX" due to commander logic.

Some critical changes:
-New parameters are defined. this will make PCA9685 able to run together with other PWM output devices.

Note this driver allows parameters to be changed run-time without reboot, which will lead to some performance issue. But it is not critical on embed Linux.

Test data / coverage
This driver still works properly on my RPi.

Additional context
This driver has a "wrapper" layer which is abstracted from the real driver. Those code can be easily picked up and quickly be deployed with other pwm drivers, such as Navio2, BBB and OCPOC. I mean, we are able to purge linux_pwm_out by setting up board specific pwm drivers in their board folder. This driver can be the template.

[SalimTerryLi]

Ready.

[SalimTerryLi]

BTW, I have figured out how we can purge src/drivers/pca9685 safely. That implement can be fully replaced with this driver. I'd like to start another PR for that once this gets merged. @dagar

[dagar]

Generally looks good, but I'm hesitating on the parameter change. Doesn't it make the simple case (only output is pca9685) more complicated from a user perspective?

I was considering extending the mixer module lib for the PWM case.

In terms of the pca9685 as an additional output (or having multiple) aren't we just missing visibility into the mapping (MAIN, AUX, etc)?

[SalimTerryLi]

Generally looks good, but I'm hesitating on the parameter change. Doesn't it make the simple case (only output is pca9685) more complicated from a user perspective?

I was considering extending the mixer module lib for the PWM case.

In terms of the pca9685 as an additional output (or having multiple) aren't we just missing visibility into the mapping (MAIN, AUX, etc)?

It will. But currently no board depends on it as the only output. Those parameters are already defined here. I just implemented them. Of course, users can simply ignore those parameters if they don't need them.

PWM channel mapping is done though mixer files, as it always be. Simply bind the mixer to this driver with mixer command should work.

How MAIN and AUX are defined? I don't think we should split them up, as they both are common PWM output interfaces.

[LorenzMeier]

@SalimTerryLi Couldn't we just use the PWM_MAIN parameters for this? All the user documentation explains how to use them and the ground station software knows how to set them. That is a lot better for an end user than having a new set of parameters.

Why did you introduce new ones and what problem are you trying to solve?

[LorenzMeier]

Because these new parameters make it very hard to set up the system through a GCS and they are not documented, I'm not convinced this is an improvement. I would retain the PWM_MAIN parameter mapping instead.

[SalimTerryLi]

@SalimTerryLi Couldn't we just use the PWM_MAIN parameters for this? All the user documentation explains how to use them and the ground station software knows how to set them. That is a lot better for an end user than having a new set of parameters.

Why did you introduce new ones and what problem are you trying to solve?

This driver may have two use case:

1. as the only output device for some new boards( actually I'm in this way
2. as an extra pwm output device, to extends the boards which has limited pwm channels( most people is in this way.

It is OK to reuse old parameters if I try to use it on my own board. but definitely can not if someone want to extend PWM channels because of conflicts. There is already a person who want to use PCA9685 on Pixhawk Cube to extend max PWM channels to control a complex VTOL.

[SalimTerryLi]

Because these new parameters make it very hard to set up the system through a GCS and they are not documented, I'm not convinced this is an improvement. I would retain the PWM_MAIN parameter mapping instead.

I'm willing to update those document, although there may be not too much people will use this driver.

[LorenzMeier]

You can't update the GCS source code and it doesn't make sense for us to carry documentation that a low number of users are actually using.

Back to the question you haven't answered: Why can't you just use the existing PWM_MAIN parameters and read those? Why do you need your own?

[SalimTerryLi]

You can't update the GCS source code and it doesn't make sense for us to carry documentation that a low number of users are actually using.

Back to the question you haven't answered: Why can't you just use the existing PWM_MAIN parameters and read those? Why do you need your own?

That will lead to conflicts if I want to use this driver on standard Pixhawk which has its own outputting driver.

[LorenzMeier]

Are you using the driver on a Pixhawk? Because the better solution would be to go straight to UAVCAN (albeit not instant).

[SalimTerryLi]

Are you using the driver on a Pixhawk? Because the better solution would be to go straight to UAVCAN (albeit not instant).

I propose these changes on behalf of at least two people.

For myself, it is ok to just read old parameters. as I use PCA9685 as the primary PWM out.
There is also someone on slack who want to use it with Pixhawk Cude to extend maximum available PWM channels. For him, new parameters are necessary.

Moreover, a badly designed pca9685 driver already exists in PX4, named src/drivers/pca9685. I want to make this new driver a better replacement of that driver. Also this driver has a developed wrapper which can be ported to other similar devices, such as linux_pwm_out. @ dagar expects to purge it, as this PR maybe a part of that action.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

[SalimTerryLi]

Switch back to original parameter names. Please review it.