Skip to content

Add an option to use LowPowerTimer for poll #6418

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

Merged
merged 2 commits into from
Apr 12, 2018

Conversation

amq
Copy link
Contributor

@amq amq commented Mar 21, 2018

Description

Similar to use-lowpower-timer-ticker in events/

This allows to use LEUART / LPUART with an ability to enter deep sleep.

For example, the cellular modem can stay constantly connected, listening for events, without preventing deep sleep.

Pull request type

  • Fix
  • Refactor
  • New target
  • Feature
  • Breaking change

@kjbracey
Copy link
Contributor

kjbracey commented Mar 22, 2018

Why is it an option, rather than being #if DEVICE_LPTIMER, and (probably related), why might it cause you to miss events?

This is kind of a general question, as I'm seeing this pattern popping up in lots of places - do we believe the low power timer is broken on certain targets? Why don't we have a "MillisecondTimer" class that automatically directs to low power if available, else microsecond? Why does every piece of code need this ifdef/option?

This also relates to #5419 - since that we no longer need a local Timer anyway for the RTOS case, as we can use Kernel::get_ms_count. The catch is we don't have the non-RTOS equivalent discussed for there yet, and this is a potentially non-RTOS-available call.

@amq
Copy link
Contributor Author

amq commented Mar 22, 2018

An option makes this change decisively non-breaking, I thought it would get merged easier this way. Happy to change to #if DEVICE_LOWPOWERTIMER (or directly to DEVICE_LPTICKER as per #6351)

My thought about losing events was that while it is highly unlikely that LowPowerTimer would affect poll directly, the fact that deep sleep won't be blocked anymore could have some side-effects.

@kjbracey
Copy link
Contributor

I guess I'd like some input from those more familiar with the LP timer and power area - it just seems to me that something's not right if code keeps having to do this. I'm pushing back on the pattern - #5419 was a response to #5378 and #5328, which was the equivalent change to this for the event queues.

It would feel most natural to me that if the application needs a timer/ticker at all, it should just be specifying "high-res (us)" or "normal-res (ms)" timer. And if the platform doesn't have a separate low-power timer, then both give you the same thing.

Perhaps we could arrange for LowPowerTimer to be typedeffed or otherwise act as Timer if !DEVICE_LOWPOWERTIMER.

I've no real idea what deep sleep disables on platforms, and thus what might be implicitly relying on the Timer keeping it awake. I guess the concern is that maybe for example the serial port stops working in deep sleep, so the use of Timer is accidentally making it work? Is that possible?

@kjbracey
Copy link
Contributor

I note that SerialBase::attach locks deep sleep, so if you have an interrupt handler for serial, deep sleep won't get entered. That means that if you're using UARTSerial , deep sleep won't get entered anyway.

That's what I'd expect, if it was the case that deep sleep disabled the serial port. So UARTSerial won't be relying on the Timer to keep it working.

Have you got some other FileHandle device that does work during deep sleep then?

@kjbracey
Copy link
Contributor

@geky - thoughts?

@amq
Copy link
Contributor Author

amq commented Mar 22, 2018

In my case, I haven't observed any issues with using LowPowerTimer in poll, I was just being cautious.

Regarding typedeffing, I would certainly appreciate it. Currently, I have #if DEVICE_LOWPOWERTIMER everywhere in my application where a timer is needed.

@@ -34,6 +34,11 @@
"force-non-copyable-error": {
"help": "Force compile time error when a NonCopyable object is copied",
"value": false
},

"poll-use-lowpower-timer": {
Copy link
Contributor

@0xc0170 0xc0170 Mar 23, 2018

Choose a reason for hiding this comment

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

events have something similar:

        "use-lowpower-timer-ticker": {
            "help": "Enable use of low power timer and ticker classes. May reduce the accuracy of the event queue.",
            "value": 0
        }

these 2 are similar in functionality, should be aligned

however, lets first answer more important questions here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to align it as much as possible

Why not use-lowpower-timer? This would imply that the whole platform uses LowPowerTimer

Why not poll-use-lowpower-ticker-timer? There is no ticker here, unlike in events

@0xc0170 0xc0170 requested review from bulislaw and c1728p9 March 23, 2018 17:56
@0xc0170
Copy link
Contributor

0xc0170 commented Mar 23, 2018

@c1728p9 @bulislaw Can you review #6418 (comment) please

@amq
Copy link
Contributor Author

amq commented Apr 3, 2018

Related: #5943

@geky
Copy link
Contributor

geky commented Apr 5, 2018

I am for this in the short term.

I agree with @kjbracey-arm that this pattern is popping up too often. We should introduce a generic class that the application can switch between timer implementations.

Honestly, I think users assume the Timer class already does this. What if changed Timer to something like HighResTimer and made Timer a configurable typedef? That way users could opt-in to low-power mode without having to fuss around with user libraries.

@kjbracey
Copy link
Contributor

kjbracey commented Apr 9, 2018

@amq - I'm still curious what actual device this helps with. As I commented above, I believe UARTSerial will be blocking deep sleep while active, and there aren't many other FileHandles supporting poll() yet.

@amq
Copy link
Contributor Author

amq commented Apr 9, 2018

@kjbracey-arm my final goal is to be able to run the cellular modem constantly, while keeping the MCU in deep sleep most of the time. From the point of view of my target (EFM32PG12) and modem (ublox Sara G3 / R4), it is possible. There are multiple changes needed to mbed-os to make this happen, and this is one of them, together with e.g. #5943

@kjbracey
Copy link
Contributor

kjbracey commented Apr 9, 2018

Okay, that makes sense.

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 10, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Apr 10, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented Apr 10, 2018

@mbed-ci
Copy link

mbed-ci commented Apr 10, 2018

@cmonr
Copy link
Contributor

cmonr commented Apr 10, 2018

Odd. Going to see if those failures are repeatable.

/morph test

@mbed-ci
Copy link

mbed-ci commented Apr 11, 2018

Test : SUCCESS

Build number : 1508
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6418/1508

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