-
Notifications
You must be signed in to change notification settings - Fork 6.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add initial LoRaWAN support #23664
Add initial LoRaWAN support #23664
Conversation
All checks are passing now. checkpatch (informational only, not a failure)
Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages. |
fb75a90
to
872e94d
Compare
@SebastianBoe I'm not sure what would be the best way to handle the library issue here. The Also, I'm not very keen to enable |
drivers/lora/sx1276.c
Outdated
@@ -42,30 +48,25 @@ static struct sx1276_dio sx1276_dios[] = | |||
#define SX1276_MAX_DIO ARRAY_SIZE(sx1276_dios) | |||
|
|||
struct sx1276_data { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dev_data
should probably be static.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay
drivers/lora/sx1276.c
Outdated
#define BOARD_TCXO_WAKEUP_TIME 5 | ||
|
||
/* TODO: Use Non-volatile memory for backup */ | ||
static volatile u32_t backup_reg[2] = { 0, 0 }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we put these variables into dev_data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no benefit of doing so... but will move it
include/lorawan.h
Outdated
@@ -0,0 +1,113 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are trying to get rid of various includes from include/
so this is not good place for lorawan.h. You could place the include file into include/net/
directory as there are other radio API includes there already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. We had this discussion for previous LoRa support also. But we settled with include/drivers/lora.h
. In this case, since we don't have include/subsys
I think it can live under include/net
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mani-Sadhasivam If lorawan.h is moving to /net, should the sample application fall under samples/net?
samples/lorawan/class_a/prj.conf
Outdated
CONFIG_LOG=y | ||
CONFIG_SPI=y | ||
CONFIG_GPIO=y | ||
CONFIG_NEWLIB_LIBC=y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need newlib?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some functions used by the stack are not present in the minimal libc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jukkar The reason is provided by @alexanderwachter
samples/lorawan/class_a/src/main.c
Outdated
#include <logging/log.h> | ||
LOG_MODULE_REGISTER(lorawan_class_a); | ||
|
||
char data[MAX_DATA_LEN] = {'h', 'e', 'l', 'l', 'o', 'w', 'o', 'r', 'l', 'd'}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be done other way around, so just set the data
variable (without size) and if you need its length, the use sizeof()
, like this:
char data[] = {'h', 'e', 'l', 'l', 'o', 'w', 'o', 'r', 'l', 'd'};
#define MAX_DATA_LEN sizeof(data)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
samples/lorawan/class_a/src/main.c
Outdated
LOG_INF("Sending data..."); | ||
while (1) { | ||
ret = lorawan_send(2, LORAWAN_DEFAULT_DATARATE, data, | ||
MAX_DATA_LEN, true, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the data length is only used here, you could just write this sizeof(data)
and avoid the MAX_DATA_LEN define.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
subsys/lorawan/Kconfig
Outdated
# SPDX-License-Identifier: Apache-2.0 | ||
|
||
menuconfig LORAWAN | ||
bool "LoRaWAN" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommened that you add [EXPERIMENTAL]
until the API is a bit more stable. This way it is easier to change things in the API if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
subsys/lorawan/lorawan.c
Outdated
static volatile bool send_status = false; | ||
|
||
/* MAC status strings */ | ||
const char *to_status_str[] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converting the status value to string, would be less error prone if you do the conversion directly in a function, so something like this:
const char *status2str(int status)
{
switch (status) {
case LORAMAC_STATUS_OK:
return "OK";
....
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
subsys/lorawan/lorawan.c
Outdated
}; | ||
|
||
/* MAC event info status strings */ | ||
const char *to_event_info_status_str[] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry @Mani-Sadhasivam , I'm not quite able to understand what the problem is.
@SebastianBoe Till now, zephyr_library_named(loramac-node)
is declared by drivers/lora/CMakeLists.txt
but for LoRaWAN, we need to use library reference in subsys/lorawan
. If we use set(ZEPHYR_CURRENT_LIBRARY loramac-node)
in subsys/lorawan
it doesn't work because this gets processed before drivers/lora
and the library definition is missing.
To solve this issue, I moved the library definition to subsys/lorawan
and it works well. But for the plain LoRa usecase where we don't need subsys/lorawan
, this is creating issue because the loramac-node
library definition is missing as subsys/lorawan
is not enabled.
For me, the only way to fix this issue is by enabling CONFIG_LORAWAN
for plain LoRa also but I don't favor it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you create more libraries? One for each entity; driver, subsystem, lorawan, lora ?
Then you wouldn't need to
set(ZEPHYR_CURRENT_LIBRARY loramac-node)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SebastianBoe But the corresponding library will be used by the loramac-node module to include drivers: https://github.com/zephyrproject-rtos/loramac-node/blob/master/zephyr/CMakeLists.txt#L3
If we use different libraries, how will the loramac-node module add the drivers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't it also create it's own library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm confused. If all 3 are different libraries, then how dirvers/lora
and subsys/lorawan
will use the drivers present in loramac-node? Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am also confused. What is the problem here?
One system can use another by having it invoke functions in the other system.
Which functions are in which libraries is unimportant. Any library can invoke functions from any other library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the error after making different libraries: https://pastebin.ubuntu.com/p/NT9TBDkcNP/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, to solve this you would want to do
zephyr_include_directories(path/to/dir/of/LoRaMac.h)
this is assuming that the header files are fairly unique, if not you need to do something that is a bit more involved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used this as per Slack conversation: f3068ee#diff-4d3f1718e6c473abc54edf7fd4c5830d
subsys/lorawan/lorawan.c
Outdated
static void McpsIndication(McpsIndication_t *mcpsIndication) | ||
{ | ||
if (mcpsIndication->Status != LORAMAC_EVENT_INFO_STATUS_OK) { | ||
LOG_ERR("%s failed : %s", __func__, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not put __func__
to log prints. The logger functions can be configured to add them automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
drivers/lora/sx1276.c
Outdated
LOG_ERR("Failed to read counter value (err %d)", err); | ||
return 0; | ||
} | ||
return k_ms_to_ticks_ceil32(k_uptime_get_32()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you use k_cycle_get_32
directly here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it but the calculated delta between two points doesn't seem to be right. Seems like it takes accumulated count into account.
drivers/lora/sx1276.c
Outdated
return ticks; | ||
u32_t RtcGetTimerElapsedTime(void) | ||
{ | ||
return (k_ms_to_ticks_ceil32(k_uptime_get_32()) - saved_time); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use k_cycle_get_32
and store the saved_time in cycles.
drivers/lora/sx1276.c
Outdated
|
||
u32_t RtcSetTimerContext(void) | ||
{ | ||
saved_time = k_ms_to_ticks_ceil32(k_uptime_get_32()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend to use ticks for saved_time
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
saved_time
is already in ticks!
samples/lorawan/class_a/prj.conf
Outdated
CONFIG_LOG=y | ||
CONFIG_SPI=y | ||
CONFIG_GPIO=y | ||
CONFIG_NEWLIB_LIBC=y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some functions used by the stack are not present in the minimal libc.
subsys/lorawan/lorawan.c
Outdated
#include <LoRaMac.h> | ||
|
||
#ifdef CONFIG_LORAMAC_REGION_AS923 | ||
#define CONFIG_LORAWAN_REGION LORAMAC_REGION_AS923 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not OK to define macro's named CONFIG_*.
CONFIG_ is a reserved namespace for Kconfig.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
I'm sorry @Mani-Sadhasivam , I'm not quite able to understand what the problem is. |
872e94d
to
934f2f1
Compare
2a25fee
to
324e427
Compare
@@ -1,5 +1,9 @@ | |||
# SPDX-License-Identifier: Apache-2.0 | |||
|
|||
zephyr_library_named(loramac-node) | |||
if(TARGET loramac-node) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An explanation in a comment above is in order. E.g. something like:
sx1276.c is added to the loramac-node library when it exists because it needs the same include directories as the loramac-node has.
When it does not exist a library of the same name is created because ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
324e427
to
0f3f546
Compare
0f3f546
to
768a688
Compare
stale
@jukkar @andysan @tejlmand @alexanderwachter @KubaFYI @KwonTae-young @chrta could you please take another look at this PR? It's been open for a while and should be ready now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will approve once you address the comments from @tejlmand from May
Code still uses:
and the alignment comment has also not been addressed. |
ff3f110
to
39ac1a4
Compare
@jukkar @andysan @alexanderwachter @KubaFYI @KwonTae-young any additional comments before we merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Add LoRaWAN API for Zephyr. Only config, join and send methods are implemented. Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
Add initial support for LoRaWAN based on Semtech's loramac-node library. Current implementation only supports OTAA config and sending data to LoRaWAN server like ThingsNetwork. While at it, this commit also moves the "loramac-node" library definition from drivers/lora to subsys/lorawan. This is required because, subsys/lorawan gets processed before drivers/lora and that creates issue while building. Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
39ac1a4
to
2e22205
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
LGTM |
This sample application shows how to configure an end node in Class-A mode and to send data to network server via gateway. Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
SystemMaxRxError is used to negotiate overall timing error for Rx in the loramac-node library. Hence, add support for configuring this parameter from Kconfig. Signed-off-by: Kuba Sanak <contact@kuba.fyi> Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
For consistency with other parts of Zephyr, the public APIs available in lorawan subsystem now returns error codes from the set defined in errno.h. Signed-off-by: Kuba Sanak <contact@kuba.fyi> [mani: reworked the code and commit a bit for upstream] Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
Add CODEOWNERS entry for LoRaWAN Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
The error handling code currently has a couple of issues: * It relies on ordered lists and upstream not changing any constants. * Converted messages are not stored in constant memory which means that log_strdup is needed whenever they are printed. This change also factors out error handling to a separate file, lw_priv.{c,h}, to facilitate reuse in a future secure element and state storage implementation. Signed-off-by: Andreas Sandberg <andreas@sandberg.pp.se>
2e22205
to
264f9d9
Compare
Hello,
This PR adds initial LoRaWAN support to Zephyr by using the loramac-node library from Semtech. This LoRaWAN support is added as a subsystem and sits on top of existing LoRa so that the underlying radio drivers get reused.
With the current support, a node can configure and send data to the network server via a gateway and also receive Acks in predefined window slots. A sample application is also included in this PR which demonstrates the CLASS-A functionality.
This PR has been verified using 96Boards Wistrio board and a SAMD21 board with SX1276 LoRa modem. Also, this has been tested in IN865 and EU868 region wherein EU868 the duty cycle restrictions are more strict so the sample application may not work correctly as it sends data in 5s interval.
TODO:
This LoRaWAN project is a collaborative work and following developers deserve explicit acknowledgments for their commendable contributions:
Kwon Tae Young - https://github.com/KwonTae-young
Kuba Sanak - https://github.com/KubaFYI
Andreas Sandberg - https://github.com/andysan
For more discussions other than this PR, please join #zephyr-lorawan slack channel.
Thanks,
Mani