Use weak symbols for time keeping so that they can get easily overridden for implementation of sleep

[msperl]

Is your feature request related to a problem? Please describe.
Powerconsumption is quite high and to get dow some sort of sleep mode is needed (say using RTC 32 bit counters on SAMD21)
Unfortunately the only solution seems to be modifying the library itself to make this work.

So a cleaner solution would be adding weak symbols, so that a sketch can override some of the functionality if necessary

Describe the solution you'd like
Essentially it could look something as "simple" as this:

--- src/hal/hal.cpp.orig	2020-04-05 10:30:48.000000000 +0200
+++ src/hal/hal.cpp	2020-04-05 10:34:31.000000000 +0200
@@ -197,6 +197,7 @@
     // Nothing to do
 }

+u4_t hal_ticks () __attribute__((weak));
 u4_t hal_ticks () {
     // Because micros() is scaled down in this function, micros() will
     // overflow before the tick timer should, causing the tick timer to
@@ -257,6 +258,7 @@
 # define HAL_WAITUNTIL_DOWNCOUNT_THRESH ms2osticks(9) // but try to leave a little slack for final timing.
 #endif

+u4_t hal_waitUntil (u4_t time) __attribute__((weak));
 u4_t hal_waitUntil (u4_t time) {
     s4_t delta = delta_time(time);
     // check for already too late.
@@ -335,6 +337,7 @@
     return irqlevel;
 }

+void hal_sleep () __attribute__((weak));
 void hal_sleep () {
     // Not implemented
 }

For some other functions it may also be handy to get the "weak treatment".
I guess others may come up with other requests...

Describe alternatives you've considered
As said: modifying the library itself, but that is not really maintainable...

Other Ideas based on this
Providing a weak "default" implementation of onEvent with weak per event methods (e.g. onEvent_txcomplete, onEvent_joined, onEvent_rxcomplete) inside the library would then also be an option.
This would reduce the amount of code that needs to get written in each sketch.
But that is probably something for a separate feature request...

[altishchenko]

There is more of architectural decision here, basically implementation of HAL must be and is paltform dependent, its API must be universal. It should be provided so, that choosing a platform in the library chooses the appropriate HAL implementation. What you are suggesting is, basically, one of the ways to implement separation between library generic HAL API, default implementation and platform dependency. weak functions look good for the platform/project dependent callbacks and in your particular example only hal_ticks() should be __weak with other timing functions left intact.

As for the low power, several things must be considered and provisioned in the library.

1. io_init/io_deinit hooks (may and should be weaks) - to disable sleep distracting peripherals, leaving only HW that is supposed to wake processor up - in the case of LMIC library itself only timer is needed to wake up and process the nearest scheduled job.
2. lock/unlock function hooks that should enable/disable allowance to go low power between invocations of os_runloop_once(). Low power MUST NEVER be entered INSIDE os_runloop_once(), so call to hal_sleep() in it is a No-No, there must be _unlock() there only.
3. hal_sleep() - must be not a hook. It must be a solid implementation of the HALs prepare to sleep/wake up logic, calling platform provided specific hooks. Something like this:

void hal_sleep(bool fStop)
{
    if (__hal_locked()) {
        /* Low power disallowed */
        return;
    }

    /* Prepare for sleep */
    hal_io_deinit(); // HAL cleanup before sleep

    /* Perform platform preparation */
    hal_msp_io_deinit(); // HOOK: This IS a weak hook! With do nothing by default

    /* Enter LP Stop/LP Sleep mode */
   hal_msp_enter_sleep(fStop); // HOOK: fStop distinguishes between LP Sleep and LP Stop modes

    /* Wake up peripherals */
    hal_msp_io_init();  // Re-enable platform specific
    hal_io_init();     // Re-enable HAL IOs
}

For the purpose of the examples and providing self-contained solution, this should be done to os_runloop():

// execute jobs from timer and from run queue
void os_runloop () {
    while(1) {
        os_runloop_once();
        hal_sleep();
    }
}

[altishchenko]

We can discuss the logic and implementation by e-mail, if you want. I will be implementing something like this for our company project, where LMIC is just a part of a much bigger picture, anyway. You can get in touch with me at [email protected],
Cheers,
Alex

[terrillmoore]

The basic architectural problem is that the design of the IBM LMIC is just wrong in this area; on an Arduino (or most other systems) you can't sleep from inside a device driver. You have to post a sleep request that is handled (as it were) in the loop() routine, including doing all the pre-sleep shutdown. Unfortunately there are lots of other things to look at for pure functionality, and MCCI has been able to deal with this adequately by querying the run loop to find out if it's safe to sleep now. I'm disinclined further to hack on this, because the LMIC/oslmic/hal stuff is really not very well separated out; there are hal-ish things in the os, and os-ish things in the hal. I really want to spend my time looking at the SX1262, class B&C, FUOTA, etc. We can live with the current structure a while longer.

[lnlp]

You have to post a sleep request that is handled (as it were) in the loop() routine,
...
MCCI has been able to deal with this adequately by querying the run loop to find out if it's safe to sleep now.

I assume this means 'querying from within loop()'?
How to determine if it is safe to go sleep? Are there any included code examples / best practices for this?

[altishchenko]

I assume this means 'querying from within loop()'?

Not really. This means indicating from within the loop() that the library is fine if outer logic will decide to go to sleep. For this to happen you shouldn't use os_runloop() which becomes the main loop of the system. Your main loop should call os_runloop_once() periodically instead.

How to determine if it is safe to go sleep? Are there any included code examples / best practices for this?

This is where hal_sleep() is called in os_runloop_once(). You can change the implementation of hal_sleep() to posting such indication.

As for the best practices, some libraries I know of, keep a global bitmask, where every driver module can set its own bit to 1 to indicate that it is busy and reset it to 0 to indicate that it is ok to go to sleep. The outer main loop considers this bitmap at the end of each run and, if it is all zeros, executes sleep+wait-for-event/wait-for-interrupt processor instruction.

Like this:

void main_loop()
{
    while (1) {
        do_some_stuff1();
        do_some_stuff2();
        ...
        os_runloop_once();
        ...

        if (_sleep_ok_mask == 0) {
            Put_CPU_to_sleep();
        }
    }
}

[x893]

Suggest add WEAK to hal.cpp
hal_io_init
hal_pin_rxtx