Add initial LoRa support

[Mani-Sadhasivam]

Hello,

This is my second attempt on adding LoRa (Long Range low power) technology support to Zephyr. This initial PR includes only P2P (Peer to Peer) support without using the LoRaMac stack. As per the discussion on the previous PR #17685, I've made use of Semtech's LoRaMac-node repository for better code maintenance. I've imported both LoRaMac and LoRa drivers (SX1276) as a module here but only P2P related drivers are enabled for now.

Since this is an initial PR, I've left many TODO items in the Zephyr SX1276 Shim driver for tackling in future.

What is working?

Only P2P support using SX1276 LoRa Radio driver. This has been tested using 96Boards Wistrio board. There is no frequency limitation as of now but only below bandwidths are supported:

125KHz, 250KHz, 500KHz

...and this limitation comes from Semtech's LoRa drivers.

Future plans

LoRaWAN support

Discussions

#5436 #5764 #17685

Thanks,
Mani

[Mani-Sadhasivam]

Tagging: @tbursztyka @alexanderwachter @jukkar @chris-makaio @KwonTae-young

[KwonTae-young]

I have a custom board using the STM32L151CBT and SX1276.
I tested sample/drivers/lora and it works fine.

***** Booting Zephyr OS build v1.8.99-18626-gabe9fca35d9b *****
[00:00:00.222,000] <inf> sx1276: SX1276 Version:12 found
[00:00:05.749,000] <inf> lora_receive: Received data: helloworld

[erwango]

Few comments after first quick review.
Can you split the PR and put SPI/RTC support on L1 in dedicated PRs so we can merge these first?

[KwonTae-young]

I think send and recive should be repeated.

[Mani-Sadhasivam]

Okay. Will use continuous loop.

[KwonTae-young]

I think sx1276_lora_recv() needs timeout.

[Mani-Sadhasivam]

@KwonTae-young Makes sense! I think the API should allow this timeout value to be specified by the application. If timeout passed is 0, then it should block until the data arrives. What do you think?

Like below,

static int sx1276_lora_recv(struct device *dev, u8_t *data, u32_t timeout)
{
        int ret;

        Radio.SetMaxPayloadLength(MODEM_LORA, 255);
        Radio.Rx(0);

        if (!timeout) {
                /* Wait until the data arrives */
                k_sem_take(&dev_data.data_sem, K_FOREVER);
        } else {
                ret = k_sem_take(&dev_data.data_sem, K_MSEC(timeout));
                if (ret < 0) {
                        LOG_ERR("Receive timeout!");
                        return ret;
                }
        }

        memcpy(data, dev_data.rx_buf, dev_data.rx_len);

        return dev_data.rx_len;
}

[alexanderwachter]

I think the API should allow this timeout value to be specified by the application. If timeout passed is 0, then it should block until the data arrives. What do you think?

I would suggest using the already defined macros K_FOREVER and K_NO_WAIT for that purpose.

[Mani-Sadhasivam]

I would suggest using the already defined macros K_FOREVER and K_NO_WAIT for that purpose.

@alexanderwachter Okay. I think you're talking about passing K_NO_WAIT from the application instead of passing 0.

[KwonTae-young]

timeout works fine.
However, if the timeout occurs once in samples/drivers/lora/receive, it is returned and the program terminates.

***** Booting Zephyr OS version 2.0.99 *****              
[00:00:00.221,000] <inf> sx1276: SX1276 Version:12 found  
[00:00:01.228,000] <err> sx1276: Receive timeout!
[00:00:01.228,000] <err> lora_receive: LoRa receive failed

I am testing with the following modifications. (timeout = 1000ms)

diff --git a/samples/drivers/lora/receive/src/main.c b/samples/drivers/lora/receive/src/main.c
index 46824dd69e..99b6985165 100644
--- a/samples/drivers/lora/receive/src/main.c
+++ b/samples/drivers/lora/receive/src/main.c
@@ -48,11 +48,10 @@ void main(void)
                len = lora_recv(lora_dev, data, 0);
                if (len < 0) {
                        LOG_ERR("LoRa receive failed");
-                       return;
+               } else {
+                       LOG_INF("Received data: %s", log_strdup(data));
                }
 
-               LOG_INF("Received data: %s", log_strdup(data));
-
                /* Receive data at 1s interval */
                k_sleep(1000);
        }

***** Booting Zephyr OS version 2.0.99 *****
[00:00:00.221,000] <inf> sx1276: SX1276 Version:12 found
[00:00:01.228,000] <err> sx1276: Receive timeout!
[00:00:01.228,000] <err> lora_receive: LoRa receive failed
[00:00:03.229,000] <err> sx1276: Receive timeout!
[00:00:03.229,000] <err> lora_receive: LoRa receive failed
[00:00:05.230,000] <err> sx1276: Receive timeout!
[00:00:05.230,000] <err> lora_receive: LoRa receive failed
...
...

[Mani-Sadhasivam]

@KwonTae-young This is the expected behavior. And the termination depends on the user application. The sample application I have provided just terminates once the timeout is received.

[KwonTae-young]

@Mani-Sadhasivam Okay. timeout works fine.

[jukkar]

BTW, did anyone had plans to add LoRa support to the BSD socket layer after this initial support is merged?

[Mani-Sadhasivam]

BTW, did anyone had plans to add LoRa support to the BSD socket layer after this initial support is merged?

@jukkar Of course! I will work on it. That would be the exact functionality required for FOTA.

[Mani-Sadhasivam]

@carlescufi Updated the PR!

[rlubos]

@carlescufi Not exactly similar to OT, as we don't reuse the existing loramac-node module CMake build system, but I guess it was not as straightforward to integrate as OT was (at least it looks complex).

Left a few comments to consider, not necesirally CMake related.

[rlubos]

This API is missing a parameter that would indicate the size of the data buffer. How would the implementation know how many bytes can it copy into the data buffer? If we blindly copy data w/o taking buffer size into consideration we will sooner or later (but rather sooner) overwrite some other data.

[Mani-Sadhasivam]

@rlubos Okay, I got the reason now. The buffer copy happens with the size of the recevied length reported by the lora stack. So even if we pass the size parameter, we can't just copy those bytes since it may or may not be received. But there are couple of usecases with size parameter:

• We can check if the buffer size exceeded the max recv length i.e., 255
• Return error if we don't receive the size bytes

[rlubos]

@Mani-Sadhasivam The main reason for providing an extra size parameter is to detect when we can't copy the data so that we don't exceed the buffer provided. I'm not a Lora expert, so I'm not sure what would be the best action to take in case this happens, but the solutions I see:

• Return an error and drop the packet (results in data loss),
• Copy only the bytes that fit, return a number of bytes copied and drop the rest (that's how DATAGRAM sockets work),
• Copy only the bytes that fit, return a number of bytes copied and keep the rest of the packet for further reads (that's how STREAM sockets work)

I understand though, that any of these will require some rework and would postpone this PR. Alternatively, we could specify in the API what's the size of the buffer that needs to be provided (as according to @alexanderwachter I understand that Lora packet has a maximum size, if it's 255 bytes used in the sample I guess it could also be a reasonable approach).

[carlescufi]

@Mani-Sadhasivam @alexanderwachter can we make 100% sure that the new LoRa sample is being built by CI before we merge this PR.

[Mani-Sadhasivam]

@carlescufi Sure, we can wait. I waited for almost 4 months ;) Btw, there are few comments from @rlubos and @alexanderwachter, do you want me to incorporate them now or I can do it after merging this initial PR?

[carlescufi]

@m

@carlescufi Sure, we can wait. I waited for almost 4 months ;) Btw, there are few comments from @rlubos and @alexanderwachter, do you want me to incorporate them now or I can do it after merging this initial PR?

Please incorporate them now before merging this PR.

[alexanderwachter]

@Mani-Sadhasivam I think you should incorporate the comments from @rlubos.
@rlubos The buffer has a fixed size. But a parameter would be nice.

[jukkar]

It might be a good idea to mark the LoRa support as [EXPERIMENTAL] here so that it is easier to tweak the API if needed.

[rlubos]

@rlubos The buffer has a fixed size. But a parameter would be nice.

I understand, but there's no information whatsoever what is the expected buffer size in the API header file. So at a minimum, we should provide this information. Still, I think that extra size_t argument passed for the function would be a neater solution.

[alexanderwachter]

@rlubos i totally agree with the buffer size argument.

[Mani-Sadhasivam]

@rlubos @alexanderwachter @carlescufi Incorporated the review comments. Please take a look

[WilliamGFish]

@Mani-Sadhasivam This looks great but could your SX1276 not cover the whole SX12xx range? Obviously there may be work to make work generic.

The LoRaMac-Node project also provides support for SX1272/73, SX1276/77/78/79 and SX1261/2 radios.

[Mani-Sadhasivam]

@Mani-Sadhasivam This looks great but could your SX1276 not cover the whole SX12xx range? Obviously there may be work to make work generic.

The LoRaMac-Node project also provides support for SX1272/73, SX1276/77/78/79 and SX1261/2 radios.

@WilliamGFish Of course! But I don't have any modules other than SX1276, so can't test. If others in the community has the modules and interest, it can always be extended. What I have provided is the base platform.