cpu/samd21: added peripheral PWM driver #3127
Conversation
haukepetersen
added
Platform: ARM
haukepetersen
force-pushed
the
add_samd21_pwm
branch
from
f5a0225 to
14727fc
Compare
| #if PWM_0_EN | ||
| {TCC1, { | ||
| {(PortGroup *)0x41004400, 6, 4, 0}, /* port 0, pin 6, af E, chan 0 */ | ||
| {(PortGroup *)0x41004400, 7, 4, 1} /* port 0, pin 7, af E, chan 1 */ |
There was a problem hiding this comment.
This is maybe a stupid question but where do I find these pins?
There was a problem hiding this comment.
Ok really stupid. Forget about it :-)
There was a problem hiding this comment.
Please don't forget that we also need some pins for the ADC #2063. (Currently I'm not thinking about the "new-peripheral" design)
There was a problem hiding this comment.
I know. I actually needed the 4 PWM pins to be on the same connector for my car project, but I can remap them for this PR and create my project-specific mapping on the side.
There was a problem hiding this comment.
Dammit! Sorry for the confusion, look downwards!
|
@haukepetersen I successfully tested the pwm on all channels with different cycles and left/right aligned mode. With my cheap logic analyzer I saw some short dropouts (i.e. not longer than one cycle) of the signal. These appeared really sporadic so maybe it's caused by my setup. I don't know if we should merge this PR now, as #3095 seems to be ongoing and this approach would introduce a somehow different driver-design, comparing to others. |
|
Actually this PR is completely independent from #3095. Also it is not a different design, just a mere more efficient implementation (with more room for improvement when changing the interface very slightly). The main idea about the driver model was always, that each CPU is free to implement the peripheral drivers as it sees fit (aka the most efficient way possible) while keeping to the API. So this is what this PWM implementation makes use of. Regarding the dropouts: I was not able to see them at my (cheap) logic analyzer, so my guess would be a measurement problem. I was also running the PWM for a while to control a motor and a server which both worked without trouble, thus strengthen that idea. Further there is nothing in the driver explaining this behavior, right?! |
|
Well, basically you are right so I won't block this improvement. Also nobody else cares about this "different" approach which is a good sign. I agree that there is nothing in the driver explaining this behaviour but as you know, sometimes things happen that you don't have expected! Anyway, I'm convinced that these dropouts were caused by a measurement problem |
|
Alright, so shoud I squash? I would suggest then to merge this and take the pinout discussion somewhere else. |
OlegHahm
added
the
CI: needs squashing
|
Öhm, can we at least solve to conflict of |
|
of course (just forgot to do so before my last comment). Moved GPIO_3 to PB03. |
|
ACK when squashed and happy Travis. @kaspar030 do you want to comment on the parameter notation in the "periph_conf.h`? |
Looks good, but now that you're asking, I would take the values out of the comments. I'll PR to show. Parameter notation looks good to me. port + pin maybe can be one variable once hauke's new gpio implementation is merged. |
Yes, that is the plan. |
haukepetersen
removed
the
CI: ready for build
haukepetersen
force-pushed
the
add_samd21_pwm
branch
from
1a2aba6 to
92e37e4
Compare
haukepetersen
added
CI: ready for build
|
added @kaspar030's fix and squashed |
|
And regarding the configuration in the periph_conf.h: Those structs that are defined there should of course also be moved into the cpus |
|
Then let's ACK and go! |
cpu/samd21: added peripheral PWM driver
|
Hello, |
This PR is based on the work of @PeterKietzmann, thanks for your prior work! Great job!
I started a new PR which supersedes #2473 as it is easier to handle for me and so I can offload some work from @PeterKietzmann...
@kaspar030: have a look at the parameter notation in the
periph_conf.h, what do you think about this? When we would now allow the CPU to define thepwm_ttype totypedef pwm_conf_t pwm_t, we could even simplify the code a little further...