[WIP] GNRC LoRaWAN#6645
[WIP] GNRC LoRaWAN#6645jia200x wants to merge 3 commits intoRIOT-OS:masterfrom Inria-Chile:gnrc_lorawan
Conversation
| bandwidth = 1; | ||
| symbTimeout = 14; | ||
| } | ||
| _rx_window_setup(loradev, channels[Channel].Frequency, datarate, bandwidth, symbTimeout, false ); |
There was a problem hiding this comment.
Compile error on that line in case USE_BAND_868 is defined.
error: 'Channel' undeclared (first use in this function)
It's better to test your implementation for compilation errors for other than 915MHz bands as well.
There was a problem hiding this comment.
Channel should be loradev->Channel.
Check if that fixes the issue.
Anyway, I will fix this on monday.
Cheers!
| return -1; | ||
| } | ||
|
|
||
| kernel_pid_t pid = atoi(argv[1]); |
There was a problem hiding this comment.
Why we should pass netdev's thread's PID into command explicitly? The description of command doesn't implies that.
I think the PID returned from gnrc_lorawan_init function called during auto-init procedure should be saved somewhere to be accessible from application code.
There was a problem hiding this comment.
I forgot to fix the description. This change was added 3 days ago.
Cheers!
|
Yes, sorry. Didn't test in that band. I will check what went wrong.
Concerning the PID issue, gnrc_lorawan_init is called from auto-init. So,
it's not exposed to main. In the future it should be a netif (so that way
is possible to retrieve the PID).
I cloned the behavior of send functions in RIOT examples.
Cheers!
El sábado, 25 de febrero de 2017, Cr0s <[email protected]> escribió:
… ***@***.**** commented on this pull request.
------------------------------
In sys/net/gnrc/link_layer/gnrc_lorawan/gnrc_lorawan.c
<#6645 (comment)>:
> +
+ // For higher datarates, we increase the number of symbols generating a Rx Timeout
+ if( ( datarate == DR_3 ) || ( datarate == DR_4 ) )
+ { // DR_4, DR_3
+ symbTimeout = 8;
+ }
+ else if( datarate == DR_5 )
+ {
+ symbTimeout = 10;
+ }
+ else if( datarate == DR_6 )
+ {// LoRa 250 kHz
+ bandwidth = 1;
+ symbTimeout = 14;
+ }
+ _rx_window_setup(loradev, channels[Channel].Frequency, datarate, bandwidth, symbTimeout, false );
Compile error on that line in case we using USE_BAND_868.
error: 'Channel' undeclared (first use in this function)
It's better to test your implementation for compilation errors for other
than 915MHz bands.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#6645 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABM8SJNlK0WtbhbSH7Niu0P2XC2OlqEzks5rgHyfgaJpZM4MKcgg>
.
|
| @@ -0,0 +1,45 @@ | |||
| APPLICATION = loramac | |||
|
|
|||
| FEATURES_REQUIRED = periph_spi periph_gpio periph_adc | |||
There was a problem hiding this comment.
In fact adc is not needed. I forgot to remove that line ;)
kaspar030
left a comment
kaspar030
left a comment
There was a problem hiding this comment.
It looks like lorawan as is ties in gnrc at the netdev level. That's a no-go. Either remove the gnrc dependency, split it up or move it into sys/net/gnrc.
| NETDEV2_EVENT_TX_TIMEOUT, | ||
| NETDEV2_EVENT_RX_TIMEOUT, | ||
| NETDEV2_EVENT_CRC_ERROR, | ||
| NETDEV2_EVENT_FHSS_CHANGE_CHANNEL, |
There was a problem hiding this comment.
At least this and the next need doc...
| struct netdev2_radio_rx_info { | ||
| uint8_t rssi; /**< RSSI of a received packet */ | ||
| uint8_t lqi; /**< LQI of a received packet */ | ||
| uint8_t snr; |
There was a problem hiding this comment.
/**< SNR of a received packet. */. For friendly OS, consistency is king.
| * @author José Ignacio Alamos <[email protected]> | ||
| */ | ||
|
|
||
| #ifndef NETDEV2_ETH_H |
|
|
||
| #include "net/netdev2.h" | ||
| #include "net/netopt.h" | ||
| #include "net/gnrc/lorawan/timer.h" |
There was a problem hiding this comment.
If this includes gnrc headers at netdev2 level, you've messed up the layering.
| extern "C" { | ||
| #endif | ||
|
|
||
| #define KEY_SIZE 16 |
There was a problem hiding this comment.
indent is all off (here and below). Please use type specifiers ((16U) where they might make a difference.
| int8_t Min : 4; | ||
| int8_t Max : 4; | ||
| }Fields; | ||
| }DrRange_t; |
There was a problem hiding this comment.
- space after
}
Are these definitions imported from somewhere?
If not, please don't use CamelCase, don't use type specifiers (sFields), and please use lower case variable/field names.
There was a problem hiding this comment.
ditto, imported from original code. I will remove them later
|
|
||
| typedef struct sLoRaMacParams | ||
| { | ||
| int8_t ChannelsTxPower; |
There was a problem hiding this comment.
if possible, reorder fields so they don't waste space through alignment constraints.
| bool b_rx; | ||
| bool link_check; | ||
| bool join_req; | ||
| gnrc_pktsnip_t *pkt; |
There was a problem hiding this comment.
No gnrc specific stuff allowed in netdev2...
There was a problem hiding this comment.
The GNRC LoRaWAN code lives in the sys/net/gnrc folder, but all variables are handled in this netdev2_lorawan_t.
Indeed it's GNRC dependent.
In this case, where should these variables live? (E.g a gnrc_netdev2_lorawan_t object?)
| { | ||
| assert(value_len == sizeof(uint32_t)); | ||
| uint32_t dev_addr = *((uint32_t *)value); | ||
| assert(dev_addr <= UINT32_MAX); |
There was a problem hiding this comment.
yep, you're right. This shouldn't be here.
| { | ||
| assert(value_len == sizeof(uint32_t)); | ||
| uint32_t net_id = *((uint32_t *)value); | ||
| assert(net_id <= UINT32_MAX); |
There was a problem hiding this comment.
yep, you're right. This shouldn't be here.
|
@emmanuelsearch I will test this PR in a few days on my hardware and I'll let you know the result. |
|
@kaspar030 There are some common variables that might still live in the netdev2_lorawan_t object (e.g dev_eui, dev_addr, etc). Exactly the same as the netdev2_ieee802154_t object. Do you think is ok to add a gnrc_netdev2_lorawan_t with all internal variables? Cheers |
|
@Cr0s now the 868mhz band is compiling. |
dylad
left a comment
dylad
left a comment
There was a problem hiding this comment.
tests/driver_sx1276 won't compile so I cannot test it.
tests/loramac sx1276 seems to be initialized but I still cannot send any data (I used a spectrum analyzer but I cannot see anything around 868MHz)
I will investigate further.
| # include the selected driver | ||
| USEMODULE += $(DRIVER) | ||
|
|
||
| CFLAGS += -DDEVELHELP |
There was a problem hiding this comment.
Missing CFLAGS USE_BAND, compiler complains.
tests/driver_sx1276/main.c
Outdated
| #include "shell_commands.h" | ||
| #include "thread.h" | ||
| #include "xtimer.h" | ||
| #include "lpm.h" |
There was a problem hiding this comment.
Did you mean periph/pm.h ? Is it useful ?
tests/driver_sx1276/main.c
Outdated
| int rx_test(int argc, char **argv) | ||
| { | ||
| nd->driver->set(nd, NETOPT_LORA_SINGLE_RECEIVE, false, sizeof(uint8_t)); | ||
| sx1276_set_rx(&sx1276, 0); |
There was a problem hiding this comment.
This function only has one argument, please remove 0
|
|
||
| static const sx1276_params_t sx1276_params[] = | ||
| { | ||
| #ifdef SX1276_PARAMS_BOARD |
There was a problem hiding this comment.
SX1276_PARAMS_DEFAULT seems to "prevail" on SX1276_PARAMS_BOARD
I made change on tests/loramac/common.h to adapt to my hardware for testing but it doesn't work. I had to change SX1276_PARAMS_DEFAULT to see change on my hardware.
|
Sorry, that test shouldn't be there. It's an outdated one. Please try the
loramac test.
There will be another PR for the netdev2 adoption of @Cr0s sx1276 driver. I
have another branch with the up to date test.
Cheers
|
|
I will commit the updated version of sx1276 test, so you can test the
868mhz band. As far as I know, it was working in the original driver.
Could you try #6002?
Cheers!
|
|
@dylad please try the just committed sx1276 driver. I don't have any sx1276 board atm. |
|
I've tested #6002 It seems to work so far(only tested with a spectrum analyzer atm) Hardware used :
Also try the MB1MAS shield with a SAML21-xpro but I'm facing issues which I think are not related to this PR. |
|
Okay tests/sx1276_driver is working I'm now working on loramac but there are some kind of "black magic" to me. |
|
Let's keep this one opened. #6797 is now merged and there are 2 possibilities:
|
|
Also, we will focus now in make this PR work with the upstream sx127x driver |
|
(or @aabadie maybe) |
|
I won't close this before we get @jia200x 's opinion. Maybe we can just close with the memo label ? |
|
Let's close it, since we adopted the pkg porting strategy.
El 18 dic. 2017 13:24, "Alexandre Abadie" <[email protected]>
escribió:
… I won't close this before we get @jia200x <https://github.com/jia200x> 's
opinion. Maybe we can just close with the memo label ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6645 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABM8SBTdTulQdFYy29vLLy66a5Y-diFQks5tBpGjgaJpZM4MKcgg>
.
|
|
Alright then. Closing in favor of #8264 |
1 similar comment
|
Alright then. Closing in favor of #8264 |
8 participants
Hi.
This is the WIP of GNRC LoRaWAN. Provides a full implementation of the LoRaWAN protocol (class A and C).
It's based on the original Semtech LoRaMAC implementation. Includes #6002 driver made by @Cr0s with netdev2 adoption. (I will split the PR in multiple parts later, but I think this way is easier for testing).
What has been tested so far:
What has been partially tested:
To be done:
Cheers!