Skip to content
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.

Sign up for GitHub

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

Jump to bottom

sys/net: add some definitions for loramac #7592

Merged
merged 1 commit into from
Dec 18, 2017
Merged

sys/net: add some definitions for loramac #7592

merged 1 commit into from
Dec 18, 2017

Conversation

aabadie
Copy link
Contributor

@aabadie aabadie commented Sep 11, 2017

This PR adds required definitions and constants used for using LoRaMAC. It's mainly based on what is required for #7505 but I tried to keep them as generic as possible by checking the specs of the MAC layer of LoRaWAN.

There are probably missing definitions but those can be added later if needed.

@aabadie aabadie added the Area: network Area: Networking label Sep 11, 2017
@aabadie aabadie added this to the Release 2017.10 milestone Sep 11, 2017
miri64
miri64 requested changes Sep 11, 2017
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor thing. Will give it a proper review tomorrow.

sys/include/net/loramac.h
#endif

#endif /* NET_LORAMAC_H */
/** @} */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing newline or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed (missing newline)

@aabadie aabadie mentioned this pull request Sep 11, 2017
Merged
@aabadie aabadie force-pushed the loramac_definitions branch 2 times, most recently from 6fa3716 to a397db3 Compare September 12, 2017 10:26
@jia200x
Copy link
Member

jia200x commented Sep 13, 2017

Looks good to me (at least for class A devices ;)

@aabadie aabadie force-pushed the loramac_definitions branch 2 times, most recently from 423b0c2 to 08cc456 Compare September 19, 2017 10:43
@aabadie
Copy link
Contributor Author

aabadie commented Sep 19, 2017

@miri64, any chance for you to look into this one ?

@aabadie aabadie force-pushed the loramac_definitions branch 2 times, most recently from 41eb188 to ebdba7c Compare October 8, 2017 10:50
@miri64
Copy link
Member

miri64 commented Oct 10, 2017

With gnrc_netdev close to being deprecated, maybe this can be ported to gnrc_netif2 directly?

@miri64
Copy link
Member

miri64 commented Oct 10, 2017

Sorry, I thought this PR introduces gnrc_netdev_loramac_init(). Where is that?

@aabadie
Copy link
Contributor Author

aabadie commented Oct 11, 2017

I thought this PR introduces gnrc_netdev_loramac_init(). Where is that?

Well, they were supposed to be added in #7505, but I just realized that the function definitions are missing or use a wrong name (gnrc_loramac_recv, gnrc_loramac_send, gnrc_netdev_loramac_init).
Do you have any advice ?

@aabadie aabadie force-pushed the loramac_definitions branch 2 times, most recently from 8b6072e to de19479 Compare October 11, 2017 16:10
@aabadie
Copy link
Contributor Author

aabadie commented Oct 11, 2017

@miri64, I moved the gnrc_netdev_loramac_init function from #7505 to here. The implementation is here as well. It's totally not tested.

@aabadie aabadie added the Area: LoRa Area: LoRa radio support label Oct 13, 2017
@aabadie aabadie force-pushed the loramac_definitions branch 2 times, most recently from 390c31c to 1dc9bf3 Compare October 13, 2017 09:18
@dylad
Copy link
Member

dylad commented Oct 13, 2017

Just a question here completely unrelated :
Why is there an implementation of loramac within GNRC and a LMIC package ? What's the point ?

@aabadie
Copy link
Contributor Author

aabadie commented Oct 16, 2017 •
edited
Loading

Why is there an implementation of loramac within GNRC and a LMIC package ? What's the point ?

The idea will be at some point to call LMIC API (or another one) via gnrc_netdev_loramac_init. A single API for working with different underlying implementations. But I have not yet a clear vision right now myself on what it should look like in the end.

@kYc0o kYc0o mentioned this pull request Nov 6, 2017
Closed
23 tasks
@aabadie aabadie added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Nov 7, 2017
@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 7, 2017
@miri64 miri64 removed the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Nov 7, 2017
@miri64
Copy link
Member

miri64 commented Nov 7, 2017

Removing Hack'n'ACK label since this contains gnrc(_netdev) related stuff.

@aabadie
Copy link
Contributor Author

aabadie commented Dec 14, 2017 •
edited
Loading

@miri64, I removed the netdev related commit from this one. So now this PR only contains LoRaMAC useful definitions.
Used in #7505 (needs adaption though) and the coming Semtech LoRaMAC PR.

@aabadie aabadie mentioned this pull request Dec 14, 2017
Merged
miri64
miri64 reviewed Dec 18, 2017
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from this (purely opinion-based) value and naming comments, I'm fine with this PR. After a short discussion, I think we can merged.

sys/include/net/loramac.h
*/
#ifndef LORAMAC_DEV_EUI_DEFAULT
#define LORAMAC_DEV_EUI_DEFAULT { 0x00, 0x00, 0x00, 0x00, \
0x00, 0x00, 0x00, 0x00 }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is also true for LoRA, but from experience with IEEE 802.15.4 a random EUI might be better than an all 0 one. Of course, this should be in accordance with the standard, so if certain bits have semantics, they should be set accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving 0 values will prevent the node from the joining a LoRaWAN network in any case. That's why by default they use those arrays. For example, the RN2483 module (with the stack already in it) has those values by default.

The arrays are provided by the LoRaWAN provider when one register a new node and they depend on the type of join procedure (either ABP or OTAA).

I can verify what the LoRaWAN specs mention about that to be sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds convincing enough to me :-)

sys/include/net/loramac.h
#define LORAMAC_APP_KEY_DEFAULT { 0x00, 0x00, 0x00, 0x00, \
0x00, 0x00, 0x00, 0x00, \
0x00, 0x00, 0x00, 0x00, \
0x00, 0x00, 0x00, 0x00 }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those keys should definitely not be 0....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is something that is set by the user retrieved from the LoRaWAN backend, I think these list can be kept like this by default.
The other problem is that some keys are not needed depending on the activation mode (ABP: requires all keys, OTAA only requires dev eui, app eui and app key).

Are you convinced ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dito

sys/include/net/loramac.h
* @brief Default device address
*/
#ifndef LORAMAC_DEV_ADDR_DEFAULT
#define LORAMAC_DEV_ADDR_DEFAULT { 0x00, 0x00, 0x00, 0x00 }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same applies here as for the EUI-64.

sys/include/net/loramac.h Outdated
* @brief Default NetID (only valid with ABP join procedure)
*/
#ifndef LORAMAC_DEFAULT_NETWORK_ID
#define LORAMAC_DEFAULT_NETWORK_ID (1U)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the doc you name it NetID. Why not in the name (would have the benefit of being much shorter)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

miri64
miri64 approved these changes Dec 18, 2017
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@miri64 miri64 merged commit 1601341 into RIOT-OS:master Dec 18, 2017
@aabadie aabadie deleted the loramac_definitions branch June 8, 2018 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 approved these changes

Assignees

@miri64 miri64

@jia200x jia200x

Labels
Area: LoRa Area: LoRa radio support Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Milestone
Release 2018.01
Development

Successfully merging this pull request may close these issues.
5 participants
@aabadie @jia200x @miri64 @dylad @smlng