Convenience macros? (Also to make examples more readable.)

[ropg]

Hi all,

As it is, the RadioLib examples directory is full of repetition that, IMHO, isn't particularly meaningful and doesn't serve code readability or to teach the principles at hand. There's hundreds of minor variations of:

  Serial.print(F("[Radio] Initializing ... "));
  int state = radio.begin();
  if(state == RADIOLIB_ERR_NONE) {
    Serial.println(F("success!"));
  } else {
    Serial.print(F("failed, code "));
    Serial.println(state);
    while(true);
  }

The code included at bottom of this post provides two macros, RADIOLIB() and RADIOLIB_OR_HALT(). This makes

RADIOLIB_OR_HALT(radio.begin());

execute the code and (if there's no hardware) print

[RadioLib] radio.begin() returned -2 (ERR_CHIP_NOT_FOUND)
[RadioLib] Halted

(When using RADIOLIB(), the user can access the last result code afterwards in _radiolib_status.)

Imagine the setup() function in an example not being a jumbled mess of if-blocks but looking something like:

void setup() {
  Serial.begin(115200);
  Serial.println("Initializing radio");

  // initialize SX1262 FSK modem at the initial frequency
  RADIOLIB_OR_HALT(radio.beginFSK(FREQ_BEGIN));

  // upload a patch to the SX1262 to enable spectral scan
  // NOTE: this patch is uploaded into volatile memory,
  //       and must be re-uploaded on every power up
  RADIOLIB_OR_HALT(radio.uploadPatch(sx126x_patch_scan, sizeof(sx126x_patch_scan)));

  // configure scan bandwidth and disable the data shaping
  RADIOLIB_OR_HALT(radio.setRxBandwidth(BANDWIDTH));
  RADIOLIB_OR_HALT(radio.setDataShaping(RADIOLIB_SHAPING_NONE));

  Serial.println("Starting scan");
}

I can see the tradeoffs, but compared to what's there now, I think the increase in readability and thus understandability is obvious. Not to mention it'll be considerably less tedious to write examples.

I guess one could make presence of another preprocessor macro create an even more minimal version that does not do the string representations to shave off a few bytes.

 

 

RadioLib_convenience.h

#ifndef RadioLib_convenience_h
#define RadioLib_convenience_h

/**
 * This allows for checking the off button, or whatever else the user wants to do during a halt.
*/
#ifndef RADIOLIB_DO_DURING_HALT
  #define RADIOLIB_DO_DURING_HALT delay(10)
#endif

extern int16_t _radiolib_status;

/**
 * @brief Convenience macro for issuing RadioLib commands.
 * This convenience macro can only be used for RadioLib functions that return a status code.
 * @param action The RadioLib command to issue
*/
#define RADIOLIB(action) \
  _radiolib_status = action; \
  Serial.printf("[RadioLib] %s returned %i (%s)\n", \ 
                #action, \
                _radiolib_status, \
                radiolib_result_string(_radiolib_status).c_str());

/**
 * @brief Convenience macro for issuing RadioLib commands and halting the program if the command fails.
 * This convenience macro can only be used for RadioLib functions that return a status code.
 * @param action The RadioLib command to issue
*/
#define RADIOLIB_OR_HALT(action) \
  RADIOLIB(action); \
  if (_radiolib_status != RADIOLIB_ERR_NONE) { \
    Serial.println("[RadioLib] Halted"); \
    while (true) { \
        RADIOLIB_DO_DURING_HALT; \
    } \
  }

/**
 * @brief Short textual representation for a few common RadioLib status codes
 *        (e.g. "ERR_CHIP_NOT_FOUND".) Returns lookup URL for all others.
 * @param result The returned status code from RadioLib
 */
String radiolib_result_string(const int16_t result);

#endif

 

RadioLib_convenience.cpp

#include <Arduino.h>
#include <RadioLib.h>

int16_t _radiolib_status;

/**
 * @brief Short textual representation for a few common RadioLib status codes
 *        (e.g. "ERR_CHIP_NOT_FOUND".) Returns lookup URL for all others.
 * @param result The returned status code from RadioLib
 */
String radiolib_result_string(const int16_t result) {
  // Just a few common ones that people get often
  switch (result) {
  case RADIOLIB_ERR_NONE:
    return "ERR_NONE";
  case RADIOLIB_ERR_CHIP_NOT_FOUND:
    return "ERR_CHIP_NOT_FOUND";
  case RADIOLIB_ERR_PACKET_TOO_LONG:
    return "ERR_PACKET_TOO_LONG";
  case RADIOLIB_ERR_RX_TIMEOUT:
    return "ERR_RX_TIMEOUT";
  case RADIOLIB_ERR_CRC_MISMATCH:
    return "ERR_CRC_MISMATCH";
  case RADIOLIB_ERR_INVALID_BANDWIDTH:
    return "ERR_INVALID_BANDWIDTH";
  case RADIOLIB_ERR_INVALID_SPREADING_FACTOR:
    return "ERR_INVALID_SPREADING_FACTOR";
  case RADIOLIB_ERR_INVALID_CODING_RATE:
    return "ERR_INVALID_CODING_RATE";
  case RADIOLIB_ERR_INVALID_FREQUENCY:
    return "ERR_INVALID_FREQUENCY";
  case RADIOLIB_ERR_INVALID_OUTPUT_POWER:
    return "ERR_INVALID_OUTPUT_POWER";
  }
  return "See https://jgromes.github.io/RadioLib/group__status__codes.html";
}

[jgromes]

I agree that a significant portion of each example is a copy-paste. However, I strongly disagree with the rest of this proposal for the following reasons:

1. I fail to see how a straightforward set of blocks, each testing for a single thing, is "a jumbled mess of if-blocks" - I challenge you to find more than two levels of nested if statements in the examples (I am aware of exactly two cases, neither of which would be solved by those macros. But it is possible I'm missing some among the 100+ examples).
2. I don't think the macro-based examples would be more readable - to me it would have the exact opposite effect. If I'm new to the lbirary and see a bunch of macros in the examples, I will assume those macros must be used to evaluate the functions, which is not true. Also, I don't know what those macros actually do, without going to the library source (or some documentation). On the other hand, if is perfectly self-explanatory. The power of 10 rule 8 comes to mind here ...
3. Quite a few of the lines in examples are actually caused by the fact that most Arduino platforms do not support generic printf - instead of a single printf call, I have to call Serial.print two or three times. If this was available, the examples would have been shorter. Your macros actually use printf (not my main point here, but would have to be reworked, of course).
4. But the main point is that it's examples - the information density is intentionally low. And yes, in some of the simpler examples, a 120-lines sketch contains maybe 10-12 actually meaningul lines of code. That's why every single one of the examples includes a link to the doxygen-generated documentation, which is the exact opposite - full reference with high information density.

Macros like you proposed are useful internally - see RADIOLIB_ASSERT. But I don't think it makes sense to e.g. add a macro that will (IMO) obscure the examples while simulatenously seeming to suggest to user that the "correct" way to handle errors is by using it. Halting on error is only one of many possible error handling approaches.

[ropg]

Hi there,

First off: Thank you for making RadioLib!

Like I said, I see tradeoffs. I agree with you that obscuring what happens underneath is not good in principle. But let's look at the setup() function in the spectrum scan file I took as an example:

void setup() {
  Serial.begin(115200);

   // initialize SX1262 FSK modem with default settings
  Serial.print(F("[SX1262] Initializing ... "));
  int state = radio.beginFSK();
  if(state == RADIOLIB_ERR_NONE) {
    Serial.println(F("success!"));
  } else {
    Serial.print(F("failed, code "));
    Serial.println(state);
    while(true);
  }

  // upload a patch to the SX1262 to enable spectral scan
  // NOTE: this patch is uploaded into volatile memory,
  //       and must be re-uploaded on every power up
  Serial.print(F("[SX1262] Uploading patch ... "));
  state = radio.uploadPatch(sx126x_patch_scan, sizeof(sx126x_patch_scan));
  if(state == RADIOLIB_ERR_NONE) {
    Serial.println(F("success!"));
  } else {
    Serial.print(F("failed, code "));
    Serial.println(state);
    while(true);
  }

  // configure scan bandwidth to 234.4 kHz
  // and disable the data shaping
  Serial.print(F("[SX1262] Setting scan parameters ... "));
  state = radio.setRxBandwidth(234.3);
  state |= radio.setDataShaping(RADIOLIB_SHAPING_NONE);
  if(state == RADIOLIB_ERR_NONE) {
    Serial.println(F("success!"));
  } else {
    Serial.print(F("failed, code "));
    Serial.println(state);
    while(true);
  }
}

As you can see, the author got tired of copying the same block yet once more, so the result codes were orred together. But your result codes are not bitmasks: what does the printed orred result code even mean? Without the orring, this would be a block of 50-something lines, just to issue four commands. At some point, IMHO, the value of "not obscuring what happens" loses to "seeing what happens in a few lines". I think my alternative is massively more readable, and thus better at teaching people how to work with RadioLib, which is the point of the examples.

As for the printf: good point: that should be reworked, naturally.

[jgromes]

As you can see, the author got tired of copying the same block yet once more

Actually, that has a reason - in that specific place, I don't care whether Rx bandwidth or data shaping failed. If either one failed, we cannot run the scan. So it's not as much a case of laziness, but I do agree that it does not explain itself and could people to use this in places where it is less appropriate.

[ropg]

(Also, I think the typical user of the library might actually much appreciate these macros for their own use.)

[jgromes]

That is possible/likely, so I would propose to add them in some public header. Though I remain unconvinced they should be used in the examples. Also I would like to hear more opinions on that, so if anyone can chime in that would be very much appreciated.

[StevenCellist]

I would not like to see those macros as well. I've had a journey to get here (at RadioLib), starting at the Heltec boards. The point with Heltec is that nobody's got a clue what is going on because it's so obscured, which is also a direction I've seen @ropg taking with his own Heltec library. No hard feelings (mostly misty), but at some point stuff loses all its meaning. While the examples are quite verbose indeed, as soon as you get the pinout and values you want OK, users can just delete most of that code anyway, as they clearly can read what they are there for. Radiolib_or_halt, I would have no clue what's supposed to happen, if I can delete, how I can optimize etc.

[ropg]

In case you were to become convinced that their use in the examples would be ok, one could add the following to the header of all examples:

/* Please do not be mystified by the RADIOLIB() and RADIOLIB_OR_HALT() macros. All they do is execute
 * the code in brackets, print the command and the resulting status code to the serial port and in the case
 * of RADIOLIB_OR_HALT halt the system if the result was not RADIOLIB_ERR_NONE. See <url> for details */

[HeadBoffin]

@ropg, perhaps take a look at PR 995 where the proposed Starter code does pretty much what you are suggesting.

[HeadBoffin]

  case RADIOLIB_ERR_RX_TIMEOUT:
    return "ERR_RX_TIMEOUT";

As you progress with LoRaWAN you'll find that this is a huge red herring - whilst entirely true, we'd expect this to happen 99% of the time because it's very asymmetrical - mostly uplinks, occasionally downlinks. Most of the library supports point to point activity - LW is a bit niche because it comes with a whole backend infrastructure that needs to be accounted for.

[ropg]

The LoRaWAN_TTN / deep sleep example in my unofficial heltec_esp32_lora_v3 library does the same thing as the new RadioLib LoRaWAN example from #995 and does not display this "error".

[HeadBoffin]

Well it wouldn't, would it, as yours started out from my Starter which doesn't display it.

I take it from your RADIOLIB_OR_HALT(action) that you like my debug function.