Skip to content
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

EFM32: Fixed pwmout_all_inactive being inversed #4142

Merged
merged 1 commit into from
Apr 19, 2017

Conversation

seppestas
Copy link
Contributor

@seppestas seppestas commented Apr 10, 2017

Description

Fixes a bug causing the pwmout_all_inactive function to return the inverse of the actual state. If one of the CC channel pins is enabled it means a channel is active so it should return true.

The bug seems to have been introduced in mbed 3 (a81fdc4). Before pwmout_all_inactive just checked if the route was enabled (which doesn't really work...)

This PR also contains some cleanup in pwm_init.

Status

READY

Steps to test or reproduce

Note: I only tested this on the EFM32 Happy Gecko MCU (EFM32HG_STK3400 target).

Since pwm_free is not called in ~PWMOut (see #3106), this bug currently does not cause any problems. However, when enabling this functionality, e.g by adding the folowing destructor to PWMOut:

~PwmOut() {
        core_util_critical_section_enter();
        pwmout_free(&_pwm);
        core_util_critical_section_exit();
}

this bug causes the PWM timer to be disabled when a PWMOut object is destroyed, even if other PWMOut objects are still being used. This causes the other PWMOut object to end up in an invalid state (I'm not sure why).

An excerpt of the code I used to test this:

#include "mbed.h"

InterruptIn activate_button(PC10);
AnalogIn in(PD6);
PwmOut *motor_forward;
PwmOut *motor_reverse;
volatile bool reverse = false;

static void activate_button_pressed() {
    reverse = ! reverse;
}

int main() {
    activate_button.rise(activate_button_pressed);

    while(true) {
        while (activate_button == 1) {
            if (motor_forward == NULL) {
                motor_forward = new PwmOut(PA0);
                motor_forward->period_us(25);
            }
            if (motor_reverse == NULL) {
                motor_reverse = new PwmOut(PA1);
                motor_reverse->period_us(25);
            }
            if (reverse) {
                motor_reverse->write(in.read());
                motor_forward->write(0);
            } else {
                motor_forward->write(in.read());
                motor_reverse->write(0);
            }
        }
        motor_reverse->write(0);
        motor_forward->write(0);
        if (motor_forward != NULL) {
            delete motor_forward;
            motor_forward = NULL;
        }
        if (motor_reverse != NULL) {
            delete motor_reverse;
            motor_reverse = NULL;
        }
        sleep();
    }
}

Without the fix of this PR, motor_reverse stops working after it gets destructed (even when it is initialized again).

If one of the CC channel pins is enabled, `pwmout_all_inactive` it
means a channel is active so it should return `true`.

This commit also contains some cleanup in `pwm_init`.
@0xc0170
Copy link
Contributor

0xc0170 commented Apr 10, 2017

cc @stevew817 @akselsm

@stevew817
Copy link
Contributor

Thanks @seppestas !

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 10, 2017

/morph test

@seppestas
Copy link
Contributor Author

How should I write tests for this? As far as I can tell there are no host-runnable / local unit test for the mbed SDK, only Greentea tests... Is there any intent to change this? If so, I see both Unity and CppUTest being used in other parts of the project. Is there any preference (maybe I should add googletest, cgreen or mutest XD).

@bridadan
Copy link
Contributor

bridadan commented Apr 10, 2017

/morph test

@seppestas in case there was confusion, this is us triggering a CI run with a command, not telling you to run/write tests (though @0xc0170 may still ask you to do so, he just won't call you "morph" 😄 )

@mbed-bot
Copy link

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1868

Build failed!

@seppestas
Copy link
Contributor Author

@bridadan I go that XD.

It's weird the CI is failing for the UBLOX C030 target, pretty sure that wasn't me 😁

@bridadan
Copy link
Contributor

@seppestas Nope, wasn't you 😄 We had a breakage on master yesterday but this has since been fixed.

/morph test

@mbed-bot
Copy link

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1884

All builds and test passed!

@sg- sg- merged commit 0176726 into ARMmbed:master Apr 19, 2017
aisair pushed a commit to aisair/mbed that referenced this pull request Apr 30, 2024
Ports for Upcoming Targets


Fixes and Changes

4008: [NUC472/M453] Support Bootloader and FlashIAP ARMmbed/mbed-os#4008
4102: Add SCL and SDA defs for I2C[0-2]; redefine I2C_[SCL,SDA] to I2C2 ARMmbed/mbed-os#4102
4118: STM32F4 Internal ADC channels rework ARMmbed/mbed-os#4118
4126: STM32F4 : remove SERIAL_TX and SERIAL_RX from available pins ARMmbed/mbed-os#4126
4148: Revert "STM32F4 Internal ADC channels rework" ARMmbed/mbed-os#4148
4152: STM32F4 Internal ADC channels rework ARMmbed/mbed-os#4152
4074: [Silicon Labs] Update pinout ARMmbed/mbed-os#4074
4133: U-BLOX_C030: Default XTAL is now 12MHz onboard. Option to use Debug 8MHz ARMmbed/mbed-os#4133
4142: EFM32: Fixed `pwmout_all_inactive` being inversed ARMmbed/mbed-os#4142
4016: [NRF5]: fix rtc overflow-while-set-timestamp issue ARMmbed/mbed-os#4016
4031: STM32 increase IAR heap size for big RAM targets ARMmbed/mbed-os#4031
4137: MCUXpresso: Update ARM linker files to eliminate reserving RAM for stack & heap ARMmbed/mbed-os#4137
4176: STM32L4 Internal ADC channels rework ARMmbed/mbed-os#4176
4154: STM32F7 Internal ADC channels rework ARMmbed/mbed-os#4154
4174: [NRF52840]:  fix rtc overflow-while-set-timestamp issue ARMmbed/mbed-os#4174
4180: [UBLOX_C030] create target hierarchy for the specific versions of the C030 board ARMmbed/mbed-os#4180
4153: STM32F2: Internal ADC channels rework ARMmbed/mbed-os#4153
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants