cpu/nrf52: PWM fixes #15117

chrysn · 2020-09-29T13:55:22Z

Contribution description

These fixes simplify code in the nRF52 driver:

The number of used channels being assigned to the CNT register broke PWM when <4 channels were in active use
The setting of general PIN_CNF was only partially correct (it didn't capture that the pins could be in P1 instead of P2), and needless (as while PWM is active, the pins are driven immaterial of PIN_CNF)

They are grouped in one PR as they're in close proximity, and require the same documentation (https://infocenter.nordicsemi.com/pdf/nRF52840_PS_v1.1.pdf#page=256) to verify.

Testing procedure

As they went unnoticed so far, the bugs can't easily be shown with the current code base; even #15115 in its latest iterations uses all 4 channels and thus doesn't trigger the bug any more.

The easiest test is to take a board with PWM support (eg. nrf5240dongle when #15115 is applied), run the osci command in tests/periph_pwm and see that things still work.

To show the original problem, you can replace one of the GPIOs in the board's periph_conf.h with GPIO_UNINIT (then, without this patch, PWM would not work at all on this device). For the removed code, I don't have any better testing other than "hey it still works"

Issues/PRs references

These were discovered while looking through why an earlier version of #15115 (that only assigned the RGB LED to channels) did not work.

benpicco · 2020-09-29T19:21:35Z

Hm, when I use

static const pwm_conf_t pwm_config[] = {
    { NRF_PWM0, { GPIO_PIN(0, 8), GPIO_PIN(1, 9), GPIO_PIN(0, 12) } }
};

on nrf52840dongle it also works on master.

chrysn · 2020-09-29T19:34:00Z

An uninitialized struct entry is 0, so you're driving pin P0.00. Try setting the fourth to GPIO_UNDEF.

benpicco · 2020-09-29T19:54:31Z

Ok, but then why do we have to set CNT to the full amount?

cpu/nrf52/periph/pwm.c

chrysn · 2020-09-30T06:01:00Z

why do we have to set CNT to the full amount?

The device doesn't think of it as the full amount but rather "just a single entry" -- just that it expresses its entries in half-words and an entry for the Single mode is 4 half-words long. The previous code tacitly assumed that a) channels would always be active-first-and-then-inactive (no reason, it could be GPIO_UNDEF, GPIO(0, 1), GPIO(0, 2), GPIO(1, 22)) and b) that the channel value loading would work by just picking a value for all the active channels, whereas really the device has no concept of active channels but only of connected and disconnected output pins.

chrysn · 2020-09-30T06:06:56Z

errata ad a): The RIOT PWM documentation does specify that unused pins must be at the right side -- but that still doesn't make nRF52 work that way.

chrysn · 2020-09-30T06:16:19Z

Mh, twice wrong -- sorry: The documentation I read was only pertaining to nRF52. As that's really unnecessary, I've removed the note and replaced it with a warning against situations like #15117 (comment).

benpicco · 2020-09-30T10:25:03Z

cpu/nrf52/include/periph_cpu.h

- * @note    define unused pins only from right to left, so the defined channels
- *          always start with channel 0 to x and the undefined ones are from x+1
- *          to PWM_CHANNELS.


This is still valid from an API perspective.
tests/periph_pwm iterates from 0 to pwm_channels(), so if there were gaps, it would miss some channels.

(And since you can pretty much map any pin to any function on nrf52, there is really no reason for gaps)

That test justifies leaving it in at least for the scope of this (still adding the warning as well).

I see some further need for cleanup here (differentiating in the documentation between generic rules and per-cpu peculiarities, currently the pwm_conf_t are a conglomerate of both)) and would also consider this left-to-right filling for reevaluation (for the EFM32 doesn't have such flexible routing, so rather than iterating from channel 0 to pwm_channels(dev) it may be better to iterate from 0 to PWM_CHANNELS and only act if pwm_is_active(dev, channel)), but that's not for now.

benpicco · 2020-09-30T10:36:34Z

Please squash

benpicco · 2020-09-30T10:38:52Z

cpu/nrf52/periph/pwm.c

@@ -90,9 +90,6 @@ uint32_t pwm_init(pwm_t pwm, pwm_mode_t mode, uint32_t freq, uint16_t res)

    /* pin configuration */
    for (unsigned i = 0; i < PWM_CHANNELS; i++) {
-        if (pwm_config[pwm].pin[i] != GPIO_UNDEF) {
-            NRF_P0->PIN_CNF[pwm_config[pwm].pin[i]] = PIN_CNF_SET;
-        }
        /* either left aligned pol or inverted duty cycle */
        pwm_seq[pwm][i] = (POL_MASK & mode) ? POL_MASK : res;
        dev(pwm)->PSEL.OUT[i] = pwm_config[pwm].pin[i];


I take setting OUT to 0xFF is the right thing to do then if we don't want to use it?

Reading the data sheet, I think we should set it to UINT32_MAX (or just 1 << 31) to properly disconnect the output in the GPIO_UNDEF case.

GPIO_UNDEF, in its 0xffffffff definition, has just the right property of having a bit 1 in the 'C' field...

Just that -- and until now I just missed that -- on this platform our gpio_t is uint8_t and 0xff is the undefined value, so we've all been driving P1.31 unless the hardware caught that.

The fix I propose and will send soon is to cast the uint8_t up to the uint32_t via int8_t and int32_t, thereby getting sign extension that cheaply gets us the 0xffffffff that's actually disabling the output.

Oh, cross-post -- yes, we should, and patch is on the way.

The fix I propose and will send soon is to cast the uint8_t up to the uint32_t via int8_t and int32_t, thereby getting sign extension that cheaply gets us the 0xffffffff that's actually disabling the output.

🤯

Granted, the savings are not as great as I had hoped, as the comparison operation can be acted on without further branching.

cpu/nrf52/periph/pwm.c

Once PWM is running, the active PWM module overrides the PIN_CNF state; only when PWM is stopped the pin returns into its PIN_CNF state (which, then, the board may set to whatever idle means there). (Moreover, the code there only worked for P0 registers, not eg. the P1.09 of the nrf52840dongle's RGB LED).

The PWM decodeer, in LOAD=Single mode, loads data in groups of 4 half-words. Previously, if any channel was not connected to a pin (a connection that's immaterial to the loader), a smaller number would be given resulting in the decoder not loading anything at all. See-Also: https://infocenter.nordicsemi.com/pdf/nRF52840_PS_v1.1.pdf#page=256

Working with less-than-full PWMs has shown that a warning is indicated here, as just missing an item would result in inadvertently driving P0.00.

chrysn · 2020-09-30T11:19:55Z

Rebased as requested; the squashing resulted in a reworded commit message ("warn against").

(Then again, this can now all be tested on the nrf52840 dongle more easily).

chrysn · 2020-09-30T11:29:36Z

Tested as a whole again just to be sure: With one pin set to GPIO_UNDEF, ENABLE_DEBUG(1) in periph/pwm.c and the debug statement modified to actually read back OUT[i], it reads back as 0xffffffff on the uninitialized pin.

chrysn added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 29, 2020

chrysn requested a review from MrKevinWeiss as a code owner September 29, 2020 13:55

chrysn self-assigned this Sep 29, 2020

chrysn mentioned this pull request Sep 29, 2020

saul_pwm: auto-init'able RGB LEDs and dimmers #15106

Merged

benpicco reviewed Sep 29, 2020

View reviewed changes

cpu/nrf52/periph/pwm.c Show resolved Hide resolved

chrysn requested review from aabadie and haukepetersen as code owners September 30, 2020 06:15

kaspar030 changed the title ~~nRF52 PWM fixes~~ cpu/nrf52: PWM fixes Sep 30, 2020

benpicco reviewed Sep 30, 2020

View reviewed changes

cpu/nrf52/periph/pwm.c Show resolved Hide resolved

chrysn added 4 commits September 30, 2020 13:14

cpu/nrf52: Warn against implicit 0 in PWM channels

65d4c88

Working with less-than-full PWMs has shown that a warning is indicated here, as just missing an item would result in inadvertently driving P0.00.

cpu/nrf52: Ensure that uninitialized PWM outputs are disabled

2053a17

chrysn force-pushed the nrf52-pwm-fixes branch from 0da2c53 to 2053a17 Compare September 30, 2020 11:19

benpicco approved these changes Sep 30, 2020

View reviewed changes

benpicco merged commit add0814 into RIOT-OS:master Sep 30, 2020

chrysn deleted the nrf52-pwm-fixes branch October 1, 2020 07:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cpu/nrf52: PWM fixes #15117

cpu/nrf52: PWM fixes #15117

chrysn commented Sep 29, 2020

benpicco commented Sep 29, 2020

chrysn commented Sep 29, 2020 via email

benpicco commented Sep 29, 2020 •

edited

Loading

chrysn commented Sep 30, 2020

chrysn commented Sep 30, 2020

chrysn commented Sep 30, 2020

benpicco Sep 30, 2020

chrysn Sep 30, 2020

benpicco commented Sep 30, 2020

benpicco Sep 30, 2020

benpicco Sep 30, 2020

chrysn Sep 30, 2020

chrysn Sep 30, 2020

benpicco Sep 30, 2020

chrysn Sep 30, 2020

chrysn commented Sep 30, 2020

chrysn commented Sep 30, 2020

cpu/nrf52: PWM fixes #15117

cpu/nrf52: PWM fixes #15117

Conversation

chrysn commented Sep 29, 2020

Contribution description

Testing procedure

Issues/PRs references

benpicco commented Sep 29, 2020

chrysn commented Sep 29, 2020 via email

benpicco commented Sep 29, 2020 • edited Loading

chrysn commented Sep 30, 2020

chrysn commented Sep 30, 2020

chrysn commented Sep 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benpicco commented Sep 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrysn commented Sep 30, 2020

chrysn commented Sep 30, 2020

benpicco commented Sep 29, 2020 •

edited

Loading