Add option to specify custom failure handler

[frazar]

As of now, when a FAILURE occurs in LMIC, it halts the execution.

This PR introduces a way to specify a custom error handling function, by defining the LMIC_FAILURE_HANDLER optional variable and an arbitrary error handling function like

#define LMIC_FAILURE_HANDLER my_failure_handler // This goes in lmic_project_config.h

// This goes in the .ino sketch
void my_failure_handler(const char *file, uint16_t line) {
    Serial.println("MY FAILURE HANDLER WAS TRIGGERED");
    save_sensitive_data_to_eeprom();
    while(1) ;
}

[terrillmoore]

Could we make this dynamic, instead of a #define, which is so troublesome in classic Arduino setups? That's only one more pointer in struct LMIC. Then for re-entrancy, also supply a context pointer. (In my experience, this always grows to wanting something even more modular, but..) To save RAM, let's ask the client to provide a structure HalFailureHandlerBlock_t containing the pointer to the function to be called and its context pointer (and maybe a link pointer). Then the user would call HalFailureHandlerBlock_t *Arduino_LMIC::hal_set_failure_handler(HalFailureHandlerBlock_t *pHandler), which would set the failure handler block pointer, and return the previous value. Only RAM cost of this isn't used: the pointer to the handler block, which would be NULL by default.

[frazar]

Yes, I agree that using #define might not be handy when using the Arduino IDE.
I avoided the pointer solution as it incurs in a RAM cost.

However, I don't understand why a context is needed. Also, it is not clear to me what a link pointer is.

With the latest changes, the new usage is the following

// In the .ino sketch

void my_failure_handler(const char *file, uint16_t line) {
    Serial.println("MY FAILURE HANDLER WAS TRIGGERED");
    while(1) ;
}

void setup() {
    // ...
    LMIC_reset();

    LMIC_setHalFailureHandler(&my_failure_handler); // Must go after LMIC_reset()!!
    // ...
}

As of now, the field in the LMIC struct is cleared by LMIC_reset(), but that's not probably the right way to go. Maybe that should be done by os_init()?

[terrillmoore]

Gosh I'm sorry. I messed up a little here because I was in too much of a hurry.

The API for controlling hal_failed() is something that has to be defined by the hal, not by the LMIC. So the function pointer etc has to be a static stored in hal.cpp (like the other things like here:

arduino-lmic/src/hal/hal.cpp

Lines 25 to 27 in d112c86

	static const Arduino_LMIC::HalPinmap_t *plmic_pins;
	static Arduino_LMIC::HalConfiguration_t *pHalConfig;
	static Arduino_LMIC::HalConfiguration_t nullHalConig;

So instead of LMIC_setHalFailureHandler(), we need hal_set_failure_handler(). The function type should be Arduino_LMIC::HalFailureHandler_t. And so forth.

The reason I ask for a context pointer (a void* "user pointer") is that complex reusable code is almost always reentrant, and up-calls always therefore need an arbitrary pointer to complete the 'closure'. But the caller can have a static in this case, as it's a catastrophic situation.

[frazar]

The PR was updated again, following your feedback.

Now the usage is the following:

// In the .ino sketch
void my_failure_handler(const char* const file, uint16_t const line) {
    Serial.println("MY FAILURE HANDLER WAS TRIGGERED");

    Serial.print("File name: '");
    Serial.print(file);
    Serial.println("'");

    Serial.print("Line: ");
    Serial.println(line);

    while(1) ;
}

// Later, e.g. in setup()
hal_set_failure_handler(&my_failure_handler); // Optional, to configure a custom handler
hal_set_failure_handler(NULL); // Optional, to restore the default behavior

I think the documentation should go in the docx/pdf, should I leave that to you, @terrillmoore ?

Also, while implementing the PR, I noticed that the declarations of the functions in "hal/hal.cpp" are not in "hal/hal.h" but rather in "lmic/hal.h". Should we consider merging the two?

[terrillmoore]

For various reasons, I don't want to try merging hal/hal.h with lmic/hal.h -- mainly because of .cpp / .c issues, and i want to focus my efforts (meagre as they are) on port 224 support (and therefore identifying roadblocks to LoRaWAN certification) prior to doing a lot of restructuring. I have a lot of code that is not Arduino based that uses a different HAL; that will be open sourced once the customer signs off, so .. now's not a great time to look at that.

I would entertain looking at breaking out the upcall for event dispatching so that users don't have to duplicate the event handling switch over and over, and incorporating some of the usability features from mcci-catena/Arduino-LoRaWAN; and at integrating PlatformIO support in our CI testing. But I want not to make futher big changes in structure or implementation of core until we have a major upgrade in testability.

[terrillmoore]

I think we need to make the return from the custom hal failure handler follow same path as the default.

[frazar]

I am also interested in improving the testability of the library, and I would like to thank you for your efforts.

If you need help in reaching the many objectives you mentioned, I propose you open a GitHub issue for each of them, describing the overall requirements for that change.

[terrillmoore]

Port 224 support: #215. I have an architecture for this, but I need to do a test implementation.
PlatformIO support: #228.
Event dispatcher cleanup: #229.

If we're going to get multiple people working on this (which I welcome), I should set up a discourse site (or a slack group); I find github issue threads are not effective for organizing work. This will be another issue #230...