cpu/sam3x8e: Add pwm implementation

[A-Paul]

cpu/sam3x8e: Add pwm implementation.

As a starting contribution here's is my pwm driver code.

After an almost complete rewrite of the pwm channel handling, any number (up to 4) can be used and arbitrary mapping between physical and interface channel numbers can be defined in "periph_conf.h".
For now I stick with the "oldschool" style, using conditional preprocessor cascades. If all is working well it may be a good base for further optimization.
"periph_conf.h" has a sample output configuration which reflects pin assignment described here.

• The application in "tests/peroph_pwm" is happy with the interface.
• A scope shows a full range ratio sweep with precise 1kHz base frequency on all predfined channels.
• pwm_stop() and pwm_poweroff() leave the ouputs with 0V and no modulation.
• pwm_start() (and also pwm_poweroff()) continue operation where it was stopped.
• (Re-)initializing at rutime with different resolution and frequency using pwm_init() works.
• All required by interface description in drivers/include/periph/pwm.h

There is still room for coding improvements ( as always):

• In "pwm_init()" I left two conditional blocks seperated as one ist writing in PWM and the other in GPIO registers. They may be melted together, and further optimized without affecting functionality.
• ... your suggestions

I would be pleased if you spend some time and give me our feedback.

[PeterKietzmann]

Can you explain this value? Is there maybe a more self-explaining name? I'm wondering if it's reasonable to define this macro in the periph_conf

[A-Paul]

It's the maximum binary logarithm for the divisor of the prescaled clocks (MCK/2^n -> MCK/1...MCK/1024). Hence the "LB" in the name.
And it seems reasonable to put it in "pwm.c".
There were several macros where I was unsure whether to place them in "pwm.c" or "periph_conf.h". I'm still not got the context of "periph_conf.h" completely.
Is it just board specific -desptite that the cpu ist on board also-, or belongs to peripheral devices on the microcontroller too?

[PeterKietzmann]

Ok I'm fine with the position and also with the name of that macro. I will trust you :-). Well, the periph_conf is meant to abstract the RIOT-driver from hardware specifics in general. In most cases that contains board specific things like pin mappings, but could also contain microcontroller attributes. If you compare #3095, this approach might change a bit in the midterm future, but I suggest to implement to "old" or "common" structure.

[PeterKietzmann]

@A-Paul congrats to your first PR :-) ! There are some comments inline. Just in general: 1) IMO it's not necessary to comment everything. Please unify the style and remove the "internal" comments from the doxygen. 2) Keep in mind that the driver should be usable for more or less any configuration in principle (pin, port, channel, ..). 3) When it comes to testing I need my board back :-)

[A-Paul]

Advice needed

Obviously I made some major mistakes with rebase before I sent the pull request. I would like to have a cleaner start when I begin committing requested changes.
Is it O.K. to sqash this whole beast down to one or two commits? Or will oher things be messed up doing this?

[miri64]

As long as the commits stay thematically seperated is even desired to have as few commits as possible in a PR.

[PeterKietzmann]

@A-Paul I think for now it's ok to do so if it simplifies things for you. I guess I'm the only one that looked at you PR right now

[PeterKietzmann]

@A-Paul give me a sign when the PR can be tested!

[PeterKietzmann]

Please change to max. four channels

[PeterKietzmann]

I would suggest not to introduce doxygen comments from the driver implementation. @haukepetersen what's you opinion on that?

[kaspar030]

same here!

[kaspar030]

Looks OK generally, but please re-do everything that uses the preprocessor:

• use structs to hold configuration
• calculate offsets instead of using preprocessor to iterate over defines

[PeterKietzmann]

@kaspar030 I discussed personally with @A-Paul today. He is at the beginning with RIOT and currently has some deadlines so we decided to merge this PR in the old and inefficient style when it's tested and do the rework when #3095 is merged and the first drivers had been adapted. Is this ok for you?

[haukepetersen]

I am on the same page as @PeterKietzmann: I say lets merge this PWM driver in the current style for now (after all comments by @PeterKietzmann are addressed), and then lets adjust it once #3095 is merged.

[PeterKietzmann]

I successfully tested the four implemented channels. Signals look good! @A-Paul please address the last small comments so we can merge this PR soon. If you are still interested then, I would suggest you to do a rework of the driver once #3095 is merged :-)

[A-Paul]

As arranged, I implemented and commited the last changes. And my tests are running as before.

Thank you @haukepetersen and @PeterKietzmann for showing thumbs up. As already mentioned I will put some effort in refactoring towards the new periph driver interface, so @kaspar030 can stop waving his fist. ;)

I intentionally didn't rebase and sqash for now. Please tell me if I should do it, or has to be done by the "elders of RIOT".

[PeterKietzmann]

@A-Paul please do so! And please understand that there was no fist because we are the friendly OS for the IoT 😸 (<- you see?)

[PeterKietzmann]

PS: after squashing I will put on Travis to see if compile tests are passing

[A-Paul]

Rebased and squashed.

[kaspar030]

On 06/11/15 16:08, Hauke Petersen wrote:

lets adjust it once #3095 is merged.

fine with me.

[PeterKietzmann]

and go