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

PmwOut: Add method to enable/disable PWM #11641

Merged
merged 2 commits into from Nov 4, 2019

Conversation

hugueskamba
Copy link
Collaborator

@hugueskamba hugueskamba commented Oct 7, 2019

Description

It is now possible to temporarily suspend PWM and safely preserve the duty
cycle set. This functionality is needed to allow a device to enter deep
sleep as a PWM instance prevents deep sleep in order for the timer it
relies on to run so its output can be modified. The duty cycle configuration
can be restored upon resuming from deep sleep.

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@evedon @bulislaw @kjbracey-arm

Release Notes

@@ -98,6 +98,19 @@ void PwmOut::pulsewidth_us(int us)
core_util_critical_section_exit();
}

void PwmOut::enable_pwm(bool enabled)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually need to pwmout_free to free the HW (and then to restore the state)? I'm not sure what will happen with HW state if the high frequency clock would go down and also HW may limit the low power state based on active HW.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ciarmcom ciarmcom requested review from bulislaw, evedon, kjbracey and a team October 7, 2019 15:00
@ciarmcom
Copy link
Member

ciarmcom commented Oct 7, 2019

@hugueskamba, thank you for your changes.
@bulislaw @kjbracey-arm @evedon @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

@hugueskamba
Copy link
Collaborator Author

This force-push splits the previously introduced method (PwmOut::enable_pwm) into PwmOut::suspend and PwmOut::resume as they are more user friendly.
The new PwmOut::suspend method powers down the PWM hardware and saves the duty cycle set so the PwmOut::resume is able to restore it.
The deep sleep lock is also now acquired on object instantiation instead of just when PwmOut::write method is called. This has the benefit of ensuring the PWM output is modified if PwmOut::pulsewidth (or its variants) is used instead of PwmOut::write.

Copy link
Member

@bulislaw bulislaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Not related to this PR but this class forces users to use floats 😞

@hugueskamba
Copy link
Collaborator Author

This force-push explicitly mentions that calls prior to resume after suspend are undefined behaviour?

@kjbracey
Copy link
Contributor

Looks good. Not related to this PR but this class forces users to use floats

Not in this case - there are integer versions of both float calls, and they don't go through the float.

evedon
evedon previously requested changes Oct 15, 2019
Copy link
Contributor

@evedon evedon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to add greentea tests (is there an existing test suite that can be expanded)?

drivers/source/PwmOut.cpp Outdated Show resolved Hide resolved
@hugueskamba
Copy link
Collaborator Author

hugueskamba commented Oct 15, 2019

We also need to add greentea tests (is there an existing test suite that can be expanded)?

@evedon

There is no PWM test that I could find test except for https://github.com/ARMmbed/mbed-os/tree/master/TESTS/mbed_hal_fpga_ci_test_shield/pwm

It is now possible to temporarily suspend PWM and safely preserve the duty
cycle set. This functionality is needed to allow a device to enter deep
sleep as a PWM instance prevents deep sleep in order for the timer it
relies on to run so its output can be modified. The duty cycle configuration
can be restored upon resuming from deep sleep.
@hugueskamba
Copy link
Collaborator Author

This force-push fixes the Doxygen spelling error.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 17, 2019

There now PWM test that I could find test except for https://github.com/ARMmbed/mbed-os/tree/master/TESTS/mbed_hal_fpga_ci_test_shield/pwm

Shall we add there new test cases to test resume/suspend?

@0xc0170 0xc0170 added the release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 label Oct 17, 2019
@hugueskamba
Copy link
Collaborator Author

hugueskamba commented Oct 17, 2019

There now PWM test that I could find test except for https://github.com/ARMmbed/mbed-os/tree/master/TESTS/mbed_hal_fpga_ci_test_shield/pwm

Shall we add there new test cases to test resume/suspend?

That test file is for testing the hal layer. The change is in the driver level.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 18, 2019

There is no test for the driver layer?

@0xc0170 0xc0170 requested a review from evedon October 18, 2019 13:04
@0xc0170
Copy link
Contributor

0xc0170 commented Oct 18, 2019

I run CI meanwhile

@hugueskamba hugueskamba force-pushed the hk-fix-deepsleep-pwmout branch 2 times, most recently from 1579347 to f6fb265 Compare October 30, 2019 10:19
UNITTESTS/drivers/PwmOut/test_pwmout.cpp Outdated Show resolved Hide resolved
UNITTESTS/drivers/PwmOut/test_pwmout.cpp Show resolved Hide resolved
UNITTESTS/drivers/PwmOut/test_pwmout.cpp Show resolved Hide resolved
UNITTESTS/drivers/PwmOut/test_pwmout.cpp Outdated Show resolved Hide resolved
UNITTESTS/drivers/PwmOut/test_pwmout.cpp Outdated Show resolved Hide resolved
UNITTESTS/drivers/PwmOut/test_pwmout.cpp Outdated Show resolved Hide resolved
UNITTESTS/drivers/PwmOut/test_pwmout.cpp Outdated Show resolved Hide resolved
UNITTESTS/drivers/PwmOut/test_pwmout.cpp Show resolved Hide resolved
@hugueskamba hugueskamba force-pushed the hk-fix-deepsleep-pwmout branch 2 times, most recently from ac77d3a to db1c269 Compare October 31, 2019 11:23
@hugueskamba
Copy link
Collaborator Author

This force-push adds unit test cases for the constructor and the destructor.

@hugueskamba hugueskamba force-pushed the hk-fix-deepsleep-pwmout branch 3 times, most recently from 8bcddc4 to 6352a01 Compare October 31, 2019 15:56
@hugueskamba hugueskamba reopened this Oct 31, 2019
Copy link
Contributor

@evedon evedon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes.
It might have been easier to use the framework to clear the expectations before the test starts but this works as well.

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 4, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Nov 4, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 3
Build artifacts

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.

None yet

7 participants