Fix for nrf52 pwm issues #7779
Fix for nrf52 pwm issues #7779
Conversation
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.
since obj->sequence = &obj->pulse and most significant bit of sequence denotes the polarity, we should set it.
|
@marcuschangarm @pan- Would y'all mind taking a look? The commits do a good job of explaining some of the code changes. |
|
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. |
|
@jrobeson Interesting.... Making sure, does PWM somewhat work on master? |
|
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. |
|
The problem here is @jrobeson pass PwmOut object by value. It works for me if I pass the argument by reference.
The question is: should it be possible to create multiple objects refering to one pwm instance? |
|
But should't |
|
@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. |
|
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. |
|
That's actually the driver @ARMmbed/mbed-os-core @pan- As for |
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? |
|
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:
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. |
|
/morph build |
1 similar comment
|
/morph build |
Build : FAILUREBuild number : 2866 |
Build : FAILUREBuild number : 2869 |
|
Restarting CI /morph build |
Build : SUCCESSBuild number : 2882 Triggering tests/morph test |
Exporter Build : FAILUREBuild number : 2503 |
|
Pausing Test CI until 5.9.6 PR is merged. |
|
/morph build |
1 similar comment
|
/morph build |
Build : SUCCESSBuild number : 2920 Triggering tests/morph test |
Exporter Build : FAILUREBuild number : 2539 |
Huh. Haven't seen this error in a while. /morph export |
Test : SUCCESSBuild number : 2669 |
|
/morph mbed2-build |
|
/morph export |
4 similar comments
|
/morph export |
|
/morph export |
|
/morph export |
|
/morph export |
|
Holding off on restarting export test due to 5.10 priority. |
|
/morph build |
Build : SUCCESSBuild number : 3015 Triggering tests/morph test |
Test : SUCCESSBuild number : 2781 |
Exporter Build : SUCCESSBuild number : 2627 |
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