mcci-catena/arduino-lmic

[Proposal] Remove undesirable dependency '#define LMIC_ENABLE_onEvent 0' for LMIC_registerEventCb()

Opened this issue · 1 comments

lnlp commented

Remove undesirable dependency '#define LMIC_ENABLE_onEvent 0' for LMIC_registerEventCb()

Documentation for

2.3.4 void **onEvent(ev_t ev)

Advises ('in any case is better') not to use the legacy onEvent() event callback and dynamically register an event callback with LMIC_registerEventCb() instead.

But this always requires the following definition:

    #define LMIC_ENABLE_onEvent 0

Having to '#define LMIC_ENABLE_onEvent 0' as requirement before LMIC_registerEventCb() can be used is not logical and not evident (therefore error prone). In fact current documentation for LMIC_registerEventCb() does not even mention it.
This undesirable dependency can be removed by the following:

  • Make '#define LMIC_ENABLE_onEvent 0' the default behavior WITHOUT requiring any macro/symbol to be defined for that.

  • Only automatically register 'legacy' onEvent() as event callback when the following is explicitly defined: '#define LMIC_ENABLE_onEvent' (without a value).

  • Add to file project_config/lmic_project_config.h the following:

    // #define LMIC_ENABLE_onEvent  /* Uncomment to use legacy onEvent() */

And add to the documentation that above line shall be uncommented for existing ('legacy') applications (migrating from LMIC-Arduino).

Most people already need to adjust their region when switching from LMIC-Arduino so uncommenting this setting for existing applications will not be a big issue.

This solution removes an undesirable dependency and will free any future applications using the MCCI LoRaWAN LMIC library from being required to add
'#define LMIC_ENABLE_onEvent 0' before LMIC_registerEventCb() can be used.

terrillmoore commented

@lnlp I think the docs are confusing things, but the implementation doesn't do what you're worried about. In fact, the compliance sketch uses LMIC_registerEventCb() without defining LMIC_ENABLE_onEvent to 0, yet doesn't have an onEvent function. The intent of the code is that it uses a weak reference; if you don't provide an onEvent function, the "right thing" happens, but a few bytes of code are wasted on a runtime check.

  • 👍1