Enhancement requests

[dalbert2]

I'd like to make a few enhancements to your LoRa library.
Would you be open to me making the following enhancements?

1. Add support for FSK operation e.g.:
   typedef enum { sx_mode_lora, sx_mode_fsk } sx_radio_mode;
   void lora_set_mode(sx_radio_mode new_mode); // default init mode is LoRa
   void lora_set_fsk_bitrate(unsigned bps); // auto-sets other FSK parameters
   void lora_get_fsk_fei(void); // read frequency error

2. Add support for making the reset pin optional if set to -1 and/or adding an installer for an optional external reset function
   (Because the ESP32 is so short on pins, I use an external GPIO expander wherever possible)

3. Add optional support for interrupt-driven operation using at least DIO0 rather than polling using vTaskDelay()

Thanks for providing the library!

[nopnop2002]

My repository is based on this.
https://github.com/Inteform/esp32-lora-library

The base repository doesn't support FSK, so I think I'll have to rewrite all the code in order to support FSK.

---

This is my personal opinion, but I don't think interrupts are needed with ESP-IDF.

In ESP-IDF, multiple tasks run in parallel.

in case of interrupt:

bool interrupt = false;
while(1) {

  if (interrupt) {
     {do receive process}
     interrupt = false;
  }

}

in case of polling:

xQueue = xQueueCreate( QUEUE_LENGTH, QUEUE_SIZE );
xTaskCreate(task, "ReceiveFromSX", 4096, NULL, 1, NULL);
while(1) {

  if (xQueueReceive(xQueue, buffer, 0) {
     {do receive process}
  }
}

These look the same

[dalbert2]

With respect to interrupts: unless I've missed something, the library provides no way to tell when a packet is received (or when transmission of a packet has finished) other than by polling the radio via the IRQ flags register. This introduces lots of latency and consumes power and is unnecessary because the radio has interrupt output pins (e.g. DIO0) that can be used to asynchronously indicate that a packet has been received or that transmission of a packet is complete. This allows the task to block (and the whole ESP32) to sleep until the radio needs to be serviced. An ISR triggered by DIO0 could (and probably should) place a message on a queue when triggered to allow processing of the received message outside the interrupt context. However, as far as I can tell, there is no way to do that with the library as written.

As for FSK, I think I can add a few functions fairly surgically to extend the library to support both LoRa and FSK/OOK without interfering with backward compatibility.

BTW, I have a lot of experience with the SX12xx family of radios.

[dalbert2]

Example: to allow use of FSK and OOK modes, I'd just add this to lora.h:

/// FSK/OOK support
typedef enum {sx_mod_fsk  = 0x00,
              sx_mod_ook  = 0x20,
              sx_mod_lora = 0x80} sx_modulation_t;
void lora_set_modulation(sx_modulation_t mode);

and this to lora.c:

void lora_set_modulation(sx_modulation_t new_mod) {

   // Fetch current modulation and operating mode
   int op_mode = lora_read_reg(REG_OP_MODE);
   int cur_mod = op_mode & 0xE0; // current modulation

   // set new modulation if changed
   if (cur_mod != new_mod) {
      // If entering or leaving LoRa mode, must transition to sleep first
      if ((cur_mod == sx_mod_lora) || (new_mod == sx_mod_lora)) {
         lora_write_reg(REG_OP_MODE, op_mode & ~0x17); // enter sleep mode
         while (lora_read_reg(REG_OP_MODE) & 0x07);    // wait for sleep
      }
      // Set new modulation mode
      lora_write_reg(REG_OP_MODE, new_mod);
      // restore original operating mode
      lora_write_reg(REG_OP_MODE, new_mod | (op_mode & 0x17));
   }
}

[dalbert2]

In case I wasn't clear about what I was asking: I am not asking you to make any changes. I have already extended the lora component to support FSK operation without making any changes that would affect backward compatibility.

I am in the process of extending it to allow use of the DIO pins to avoid having to poll the IRQ register(s).

My question is whether you are open to these changes being added to your fork or whether I should start a new fork. If on your fork, I would submit the changes to you for review as a PR.

[nopnop2002]

Your PR is welcome.

I need time to understand your PR and incorporate it into my repository.

[nopnop2002]

I have a suggestion.

Instead of adding lora_set_modulation(),
Wouldn't it be cleaner to add a mode to lora_init ()?

lora_init (sx_mod_fsk);

lora_init (sx_mod_ook);

lora_init (sx_mod_lora);

I don't think we will switch modes during communication.

---

I think the following functions also need to be changed to behave according to the fsk/ook mode.

lora_set_sync_word(int sw);
lora_enable_crc();
lora_disable_crc();
lora_set_coding_rate(cr);
lora_get_coding_rate();
lora_set_bandwidth(bw);
lora_get_bandwidth();
lora_set_spreading_factor(sf);
lora_get_spreading_factor(sf);
lora_set_preamble_length(long length);
lora_get_preamble_length(void);
lora_explicit_header_mode(void):
lora_implicit_header_mode(int size);

That is, all functions that deal with registers above 0x0D.
Except 0x40 0x41 0x42 0x4B 0x4D 0x5B 0x61 0x62 0x63 0x64 0x70.

My question is whether you are open to these changes being added to your fork or whether I should start a new fork. If on your fork, I would submit the changes to you for review as a PR.

We may need esp-idf-sx127x_fsk.

[dalbert2]

I agree that specifying the modulation type at lora_init() makes sense. I didn't do it that way because my goal was to ensure full backward compatibility so that my changes wouldn't break existing code that uses the component. It doesn't impose much of a hardship on the user to call lora_init() and then lora_modulation(sx_mod_fsk). Because the module is C rather than C++, we can't take advantage of overloading and parameter defaults which might have made things easier.

Another parameter that might be appropriate for lora_init() but to maintain backward compatibility I put in a separate function is lora_set_transceiver_type(sx_transceiver_t xcvr) which takes sx_transceiver_1276 or sx_transceiver_1272 which is needed if you want to support both transceiver types because a few registers are different.

There are a few things we should consider enhancing on the existing code such as enforcing atomicity in some places (e.g. for get/set frequency, the reads/writes of the FRF registers must be atomic).

I already have the FSK version working and sending and receiving packets. When I finish, I'll submit a PR and you can decide if you want to incorporate the changes...and then hopefully we can improve on them over time!

[nopnop2002]

backward compatibility is important, but clean code is even more important.