New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for nrf52 pwm issues #7779

Merged
merged 2 commits into from Sep 6, 2018

Conversation

Projects
None yet
9 participants
@MateuszMaz
Contributor

MateuszMaz commented Aug 13, 2018

Description

This PR contains fix for issues #7707 and #7743

#Issue #7743
Deleted lines that caused the problem.
Note, in nrf_drv_pwm_init there are lines that check if pwm instance is already running, so we don't really need to check it in nordic_pwm_init.
nrf_drv_uninit should be used in nordic_pwm_restart.

#Issue #7707
Waveform polarity is driven by most significant bit of sequence. The solution is about setting the right value there during initialization, and secure it against overwriting.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Feature
[ ] Breaking change

MateuszMaz added some commits Aug 13, 2018

Fix for issue #7743 NRF52 Cannot initialize PWM
Deleted lines that caused the problem. Note that, in nrf_drv_pwm_init there are lines that check if pwm instance is already running, so we don't even need to check it in nordic_pwm_init. 
nrf_drv_uninit should be used in nordic_pwm_restart.
Fix for issue #7707 PwmOut inverted
since obj->sequence = &obj->pulse
and most significant bit of sequence denotes the polarity, we should set it.

This was referenced Aug 13, 2018

@cmonr

cmonr approved these changes Aug 13, 2018

LGTM

@cmonr cmonr requested review from marcuschangarm and pan- Aug 13, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 13, 2018

@marcuschangarm @pan- Would y'all mind taking a look? The commits do a good job of explaining some of the code changes.

@cmonr cmonr added the needs: review label Aug 13, 2018

@jrobeson

This comment has been minimized.

Contributor

jrobeson commented Aug 14, 2018

PWM doesn't work for me at all with this. I don't see anything on my scope either. It's like nothing happens at all.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 14, 2018

@jrobeson Interesting.... Making sure, does PWM somewhat work on master?

@jrobeson

This comment has been minimized.

Contributor

jrobeson commented Aug 14, 2018

I guess you could say somewhat. the output stuck at 3v (the mcu voltage, but i do see flickers on the scope where it's trying to do stuff. I see absolutely nothing with the patch applied.

This is the simple code I'm using in a loop

void tune(PwmOut name, int period, int beat)
{
    int delay = beat * 63;
    name.period_us(period);
    name.write(0.50f);
    wait_ms(delay);    // 1 beat
    name.period_us(0);
}

from a tune library on the mbed site.

@MateuszMaz

This comment has been minimized.

Contributor

MateuszMaz commented Aug 14, 2018

The problem here is @jrobeson pass PwmOut object by value. It works for me if I pass the argument by reference.

void tune(PwmOut &name, int period, int beat)

The question is: should it be possible to create multiple objects refering to one pwm instance?

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Aug 14, 2018

But should't PwmOut be NonCopyable? @ARMmbed/mbed-os-hal

@jrobeson

This comment has been minimized.

Contributor

jrobeson commented Aug 14, 2018

@MateuszMaz : thanks for that. that's interesting because this same code used to work. I'm not sure when the last time i used it though, probably much earlier in the 5.x dev cycle.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 14, 2018

Oh, that's weird. Should definitely be looked at by @ARMmbed/mbed-os-hal.

I was under the assumtion that all HAL objects were pass-by-reference as well.

@bulislaw

This comment has been minimized.

Member

bulislaw commented Aug 14, 2018

That's actually the driver @ARMmbed/mbed-os-core @pan- As for NonCopyable in theory it should be, but I remember it was reported to cause troubles for some users.

@cmonr cmonr requested a review from ARMmbed/mbed-os-core Aug 14, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 20, 2018

That's actually the driver @ARMmbed/mbed-os-core @pan- As for NonCopyable in theory it should be, but I remember it was reported to cause troubles for some users.

That is correct it was and reverted. Looking at the changes here, this is not related to this patch, let's create a separate issue for this (if there is not already one!).

@MateuszMaz How did you test this PR?

@MateuszMaz

This comment has been minimized.

Contributor

MateuszMaz commented Aug 21, 2018

I tested this with ci_test_shield, and manually with logic analyzer to check if it's not inverted, and to see if PwmOut methods works.

ci_test_shield tests I have done:

  1. tests-api-pwm_rise_fall
  2. tests-api-pwm_rise
  3. tests-api-pwm_fall
  4. tests-assumptions-pwm
  5. tests-assumptions-pwmout

Tests after fix are equal to tests before regression, that means all tests passed expect cases in which we want to set 100 ms period.
I see that this problem occurs for large number of devices. It is because the frequency of pwm clk source is too high to get that long period.

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Aug 22, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 22, 2018

/morph build

1 similar comment
@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 22, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 22, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 22, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 23, 2018

Restarting CI

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 23, 2018

Build : SUCCESS

Build number : 2882
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7779/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 23, 2018

Pausing Test CI until 5.9.6 PR is merged.
Will restart CI shortly after.

@NirSonnenschein

This comment has been minimized.

Contributor

NirSonnenschein commented Aug 27, 2018

/morph build

1 similar comment
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 27, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 27, 2018

Build : SUCCESS

Build number : 2920
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7779/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 27, 2018

Error: L6200E: Symbol __asm___13_lwip_ethip6_c____REV16 multiply defined (by mbed-os/features/lwipstack/lwip/src/core/ipv6/lwip_icmp6.o and mbed-os/features/lwipstack/lwip/src/core/ipv6/lwip_ethip6.o).
Error: L6200E: Symbol __asm___13_lwip_ethip6_c____REVSH multiply defined (by mbed-os/features/lwipstack/lwip/src/core/ipv6/lwip_icmp6.o and mbed-os/features/lwipstack/lwip/src/core/ipv6/lwip_ethip6.o).
Error: L6200E: Symbol __asm___13_lwip_ethip6_c____RRX multiply defined (by mbed-os/features/lwipstack/lwip/src/core/ipv6/lwip_icmp6.o and mbed-os/features/lwipstack/lwip/src/core/ipv6/lwip_ethip6.o).

Huh. Haven't seen this error in a while.

/morph export

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 27, 2018

/morph mbed2-build

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 28, 2018

/morph export

4 similar comments
@NirSonnenschein

This comment has been minimized.

Contributor

NirSonnenschein commented Aug 29, 2018

/morph export

@NirSonnenschein

This comment has been minimized.

Contributor

NirSonnenschein commented Aug 29, 2018

/morph export

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 29, 2018

/morph export

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Aug 29, 2018

/morph export

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 30, 2018

Holding off on restarting export test due to 5.10 priority.

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Sep 5, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 5, 2018

Build : SUCCESS

Build number : 3015
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7779/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr merged commit 201ec14 into ARMmbed:master Sep 6, 2018

14 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0.0%) RAM(+0.0%)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 583 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10233 cycles (+944 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9960B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@0xc0170 0xc0170 removed the needs: CI label Sep 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment