The PwmOut::suspend() and PwmOut::resume() functions should use *_pulsewitdth_us() functions

[billwatersiii]

Description of defect

The problem identified here...
#13480

Was fixed here...
#13492

But, I believe that the intent was to have the PwmOut::suspend() and PwmOut::resume() functions use *_pulsewitdth_us() functions. The #13492 update added PwmOut::read_pulsewitdth_us() and the associated pwmout_read_pulsewidth_us functions, but it didn't use them.

Right now, the code uses a percentage for the saving/restoring the _duty_cycle. And when restoring this, it provides the "percentage of the period" before providing the period. This will likely be a problem for many devices.

From mbed-os/drivers/source/PwmOut.cpp:

void PwmOut::suspend()
{
    core_util_critical_section_enter();
    if (_initialized) {
        _duty_cycle = PwmOut::read();
        _period_us = PwmOut::read_period_us();
        PwmOut::deinit();
    }
    core_util_critical_section_exit();
}

void PwmOut::resume()
{
    core_util_critical_section_enter();
    if (!_initialized) {
        PwmOut::init();
        PwmOut::write(_duty_cycle);
        PwmOut::period_us(_period_us);
    }
    core_util_critical_section_exit();
}

I believe that the intent was to update the code to the following...

void PwmOut::suspend()
{
    core_util_critical_section_enter();
    if (_initialized) {
        _duty_cycle = PwmOut::read_pulsewitdth_us();
        _period_us = PwmOut::read_period_us();
        PwmOut::deinit();
    }
    core_util_critical_section_exit();
}

void PwmOut::resume()
{
    core_util_critical_section_enter();
    if (!_initialized) {
        PwmOut::init();
        PwmOut::pulsewitdth_us(_duty_cycle);
        PwmOut::period_us(_period_us);
    }
    core_util_critical_section_exit();
}

There still could be hardware-specific issues here. I think that a better approach is to push this into target-specific code and just call save/restore functions here.

void PwmOut::suspend()
{
    core_util_critical_section_enter();
    if (_initialized) {
        PwmOut::save();
        PwmOut::deinit();
    }
    core_util_critical_section_exit();
}

void PwmOut::resume()
{
    core_util_critical_section_enter();
    if (!_initialized) {
        PwmOut::init();
        PwmOut::restore();
    }
    core_util_critical_section_exit();
}

Target(s) affected by this defect ?

Probably several devices from multiple vendors. At least these:

CY8CKIT064B0S2_4343W
CY8CKIT_062S2_43012
CY8CKIT_062_BLE
CY8CKIT_062_WIFI_BT
CY8CPROTO_062S3_4343W
CY8CPROTO_062_4343W

Toolchain(s) (name and version) displaying this defect ?

All

What version of Mbed-os are you using (tag or sha) ?

Latest

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

N/A

How is this defect reproduced ?

As outlined here...
#13480

[0xc0170]

There is a test in the pull request provided you can run on your hardware to verify it works as expected. Do tests pass?

I did not dig into the code but I would expect the current implementation or using _pulsewitdth_us methods to have the same result. Both should work.

cc @ARMmbed/team-cypress

[billwatersiii]

@0xc0170, sorry, I used the wrong link for the pull request in the description. I updated that link to #13492.

The test from #11641 will not catch the problem. If there is a problem, it will be seen on the pin. In the case with the devices above, you will not see the expected duty-cycle on the output on that pin. This is because you passed in a percentage before passing in the value to take the percentage of. I do have a workaround for the devices above. But, I suspect that other vendor/devices/targets will have this issue.

Why did #13492 add PwmOut::read_pulsewitdth_us() and the associated pwmout_read_pulsewidth_us functions, if the intent wasn't to use them to save/restore the duty cycle? The pwmout_read_pulsewidth_us function had to be added to 20+ files.

[0xc0170]

Why did #13492 add PwmOut::read_pulsewitdth_us() and the associated pwmout_read_pulsewidth_us functions, if the intent wasn't to use them to save/restore the duty cycle? The pwmout_read_pulsewidth_us function had to be added to 20+ files.

@talorion would you be able to comment?

[talorion]

Why did #13492 add PwmOut::read_pulsewitdth_us() and the associated pwmout_read_pulsewidth_us functions, if the intent wasn't to use them to save/restore the duty cycle? The pwmout_read_pulsewidth_us function had to be added to 20+ files.

@talorion would you be able to comment?

i am not sure why i added PwmOut::read_pulsewitdth_us
i assume that since i had to add PwmOut::read_period_us i added PwmOut::read_pulsewitdth_us as well.