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

Remove custom Silicon Labs sleep management #5581

Merged
merged 2 commits into from Dec 12, 2017

Conversation

Projects
None yet
5 participants
@stevew817
Contributor

stevew817 commented Nov 24, 2017

Description

Standardize on the mbed sleep manager by removing the sleepmodes API, and statically redirecting hal_sleep to EM1 and hal_deepsleep to EM2. This should resolve #5423, #5005 and #4082.

@0xc0170 @bulislaw

Status

READY

Migrations

If users were calling blockSleepmode and unblockSleepmode:

  • (un)blockSleepmode(EM0) and (un)blockSleepmode(EM1): don't call anymore
  • (un)blockSleepmode(EM2) and below: replace with sleep_manager_(un)lock_deep_sleep() from mbed_sleep.h
@bulislaw

Looks good, I'm not 100% sure the locking is necessary as we lock the deep sleep in our driver layer. It also seem to conform with our new sleep HAL API https://github.com/ARMmbed/mbed-os/blob/feature-hal-spec-sleep/hal/sleep_api.h

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 27, 2017

Standardize on the mbed sleep manager by removing the sleepmodes API, and statically redirecting hal_sleep to EM1 and hal_deepsleep to EM2. This should resolve #5423, #5005 and #4082.

+1 for addressing those issues.

In the most cases in this patch, the changes in HAL could be removals. It should be functional either way. Is that correct?

For instance i2c hal implementation does not need to touch deepsleep here, as I2C object does. Same could be applied to spi, pwmout, serial.

@stevew817

This comment has been minimized.

Contributor

stevew817 commented Nov 27, 2017

@0xc0170 Yes, should be functional either way. I'm not sure what the preferred means is in mbed-OS, actually. Do you just always disable deepsleep when doing 'something', or do you leave it up to the driver to figure out whether the current operation is supported in deepsleep or not?

Comes to mind:

  • EFM32 has a low-energy UART which is available during deepsleep. If you disable deepsleep for all Serial operations in the CPP driver layer, then the point of having LEUART is basically moot.
  • EFM32 has an I2C peripheral which under some circumstances can operate in deepsleep

Yes, I'm making a case for leaving it up to the driver, since it is the silicon vendor who should know best what is possible during deepsleep and what is not.

@theotherjimmy theotherjimmy requested a review from 0xc0170 Nov 27, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 29, 2017

@0xc0170 Yes, should be functional either way. I'm not sure what the preferred means is in mbed-OS, actually. Do you just always disable deepsleep when doing 'something', or do you leave it up to the driver to figure out whether the current operation is supported in deepsleep or not?

Fair points. Only disabling deepsleep for drivers that require high speed freq clocks (very limited cases where it could potentially run, then question is what speed can be achieved, and other factors). The model was simplified. Therefore drivers like SPI, I2C, Serial lock deep sleep

Yes, I'm making a case for leaving it up to the driver, since it is the silicon vendor who should know best what is possible during deepsleep and what is not.

I haven't seen this in the code, might have overlooked. Ane example, If serial object locks deepsleep while transferring data and waiting for interrupt, and in HAL there is a check if its LPUART, then it would in the idle loop enter deepsleep anyway?

@0xc0170 0xc0170 requested a review from c1728p9 Nov 29, 2017

@stevew817

This comment has been minimized.

Contributor

stevew817 commented Nov 30, 2017

I haven't seen this in the code, might have overlooked.

Here, for example: https://github.com/ARMmbed/mbed-os/pull/5581/files#diff-a61dbd2ee4b7dbd1c40906a404f3629bL2119

If the LEUART is ran off of the LF clock, and its baud rate is supported by the LF clock, then deepsleep is not blocked by the driver.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 30, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Nov 30, 2017

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@bulislaw

This comment has been minimized.

Member

bulislaw commented Nov 30, 2017

@0xc0170 do you think it makes sense to push the locking to HAL implementation rather than have it in driver? So we can leverage different HW capabilities?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Dec 1, 2017

@stevew817 Can you please rebase, there is conflict now due to your earlier patches that got in yesterday

@0xc0170 do you think it makes sense to push the locking to HAL implementation rather than have it in driver? So we can leverage different HW capabilities?

As it is , driver provides generic locking, HAL can extend it if it provides more granular selection, as we can see within this patch.

stevew817 added some commits Nov 24, 2017

Remove custom Silicon Labs sleep management
Standardize on the mbed sleep manager by removing the sleepmodes API, and statically redirecting hal_sleep to EM1 and hal_deepsleep to EM2.

@stevew817 stevew817 force-pushed the SiliconLabs:feature/remove-custom-sleepmodes branch to 5dd4613 Dec 1, 2017

@stevew817

This comment has been minimized.

Contributor

stevew817 commented Dec 1, 2017

@0xc0170 Rebased.

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Dec 1, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Dec 1, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Dec 1, 2017

@stevew817

This comment has been minimized.

Contributor

stevew817 commented Dec 4, 2017

@0xc0170 Can you restart CI?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Dec 4, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Dec 4, 2017

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 removed the needs: CI label Dec 4, 2017

@0xc0170 0xc0170 merged commit 6c59e13 into ARMmbed:master Dec 12, 2017

10 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2 Local mbed2 testing has passed
Details
travis-ci/tools Local tools testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment