mcci-catena / arduino-lmic

LoraWAN-MAC-in-C library, adapted to run under the Arduino environment
https://forum.mcci.io/c/device-software/arduino-lmic/
MIT License
636 stars 207 forks source link

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

Open lnlp opened 5 years ago

lnlp commented 5 years ago

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:

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 5 years ago

@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.