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.

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

[RFC][DNM] Porting LoRaMAC-node project to zephyr #5764

Closed
wants to merge 8 commits into from

Conversation

miyatsu
Copy link
Contributor

@miyatsu miyatsu commented Jan 20, 2018

Recently, I'm try to use Semtech SX1278 radio chip to do something. But zephyr didn't support LoRaWAN so far.

So, I'm trying to porting the LoRaMac-node project (https://github.com/Lora-net/LoRaMac-node) to zephyr just for my self use ONLY, but no plan to contribute this work to zephyr community.

Here are some previous conversations #4569 . I took
suggestions from @tbursztyka , try to make this patch easy to somebody else to finish this work.

And LoRaWAN may will be zephyr's major components in the future, see #5436

I decide to open this PR so every body can see it, may helpful for zephyr feature development on LoRaWAN support.

All files are copy from LoRaWAN project.

Signed-off-by: Ding Tao <[email protected]>
All files are copy from LoRaWAN-node project.

Signed-off-by: Ding Tao <[email protected]>
All files are copy from LoRaMAC-node project.

Signed-off-by: Ding Tao <[email protected]>
We implement them at one single file to port to zephyr.

Signed-off-by: Ding Tao <[email protected]>
We implement them at one single file to port to zephyr.

Signed-off-by: Ding Tao <[email protected]>
Unfinished, this file is the driver of sx1276.

Signed-off-by: Ding Tao <[email protected]>
Unfinished, this file is the driver of sx1272.

Signed-off-by: Ding Tao <[email protected]>
This commit is only for test purpose.

Signed-off-by: Ding Tao <[email protected]>
@miyatsu
Copy link
Contributor Author

miyatsu commented Jan 20, 2018

@codecov-io
Copy link

Codecov Report

Merging #5764 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5764      +/-   ##
=========================================
+ Coverage   51.29%   51.3%   +<.01%     
=========================================
  Files         441     441              
  Lines       42268   42268              
  Branches     8063    8063              
=========================================
+ Hits        21683   21685       +2     
+ Misses      20066   20064       -2     
  Partials      519     519
Impacted Files Coverage Δ
arch/posix/core/posix_core.c 87.06% <0%> (+1.72%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f88c28...f335ec4. Read the comment docs.

@rodrigopex
Copy link
Contributor

So, I'm trying to porting the LoRaMac-node project (https://github.com/Lora-net/LoRaMac-node) to zephyr just for my self use ONLY, but no plan to contribute this work to zephyr community.

@miyatsu since you don´t want to contribute this work to Zephyr community, I will try to start with a different approach, an easier one. My start point will be to make a driver for the RFM95 and port the LoRaWAN (AKA LoraMac) as a library only. After that, I will try to make LoRaWAN part of the kernel itself. I think this is a more smooth/natural way to get deep into Zephyr. Let´s begin with a more realistic goal for me. Thank you for your help.

@miyatsu
Copy link
Contributor Author

miyatsu commented Jan 23, 2018

@rodrigopex Do you got any idea of how to make the LoRaMAC as a library ?

I'm try to make the LoRaMAC run like fs in zephyr, that can linked with the kernel, rather than compile the hole LoRaMAC with kernel to zephyr output file.

I find the cmake command add_library() via subsys/bluetooth/CMakeLists.txt .. But don't know how to use it.

Copy link
Collaborator

@tbursztyka tbursztyka left a comment

Choose a reason for hiding this comment

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

For the source code you wrote yourself, check the style:

  • 80 chars limit
  • '{' is on the same line as if (), for (), while ()...

Rest looks fine to me. Looks like you just need to finish the Kconfig/CMakeLists.txt stuff.


menu "LoRaWAN"

config LORAWAN
Copy link
Collaborator

Choose a reason for hiding this comment

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

Merge this line with line 8:

menuconfig LORAWAN
....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Write this as you suggested, this menu will not be shown in a single page.

subsys/lorawan/Kconfig Show resolved Hide resolved
@@ -0,0 +1,2 @@
zephyr_sources_ifdef(CONFIG_SX1272 sx1272.c)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you need a CMakeLists.txt here, as all the source code is in ext/...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggested, I should move all imp func to ext/ dir ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@miyatsu Sorry I got confused with the patches. Actually the driver function implementation (sx1272.c and sx1276.c), should go in drivers/lora with this CMakeLists.txt and the radio Kconfig part. Looks like it only needs to be moved, content seems fine to me. Maybe change the Kconfig first config option to:
menuconfig LORA
bool "LoRa Radio chip support"
default n
select

if LORA
... the config option for the 2 radio chip (the separation in dedicated Kconfig.sx--- as you did is nice. Maybe add the prefix LORA_ to all these options though. This is usually how we do, so it tells to which part the config option belongs to.

endfi #LORA


int bus_spi_init(void)
{
spi_cs.gpio_dev = device_get_binding("GPIOA");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, so here you will have to use your Kconfig options you declare in the last patch

@carlescufi
Copy link
Member

@miyatsu If you are posting a PR here I assume that you actually do want to make this available to Zephyr in general, so we need to follow the rules.

Let's start by cleaning up the import to ext/.

First, the commit messages when importing into ext/ have a special format, that you can find here:
http://docs.zephyrproject.org/contribute/contribute_non-apache.html#code-component-readme-template
An example can be seen here:

ext: nordic: Import relevant files from nrfx 0.8.0

nrfx is an extract from the nRF5 SDK that contains solely the drivers
for peripherals present in Nordic SoCs, for convenience complemented
with the MDK package containing required structures and bitfields
definitions, startup files etc.

Origin: nrfx
License: BSD 3-Clause
URL: https://github.com/NordicSemiconductor/nrfx/tree/v0.8.0
commit: b7cfe970b45ad7cc9c36b62ee620508e9e2c7fb5
Purpose: Provide peripheral drivers for Nordic SoCs
Maintained-by: External

Then you need to import a specific version of the LoraWan-node repository, untouched from the upstream version.
If you really need changes on top of that, you can add a commit on top of the clean import and we'll see if it works there.

@nashif nashif added the DNM This PR should not be merged (Do Not Merge) label Mar 28, 2018
@miyatsu miyatsu closed this Apr 19, 2018
@chris-schra
Copy link
Contributor

sorry, I'm confused - will this ever be merged? I'm starting to work with it and add RN2483... and it would be great if I knew how to prepare to contribute it later

@tbursztyka
Copy link
Collaborator

@derchrismakaio : Unfortunately the author of this PR did not update it and closed it. Nothing prevents you to take over the work already done, apply the requested changes above, and create a new PR from it. Would be nice to get that into zephyr.

@carlescufi
Copy link
Member

@derchrismakaio as @tbursztyka mentions, you can pull this PR and create a new one from it that you can submit later.
More info here: http://docs.zephyrproject.org/contribute/contribute_guidelines.html#pull-requests-and-issues

@miyatsu
Copy link
Contributor Author

miyatsu commented Sep 3, 2018

Hi @derchrismakaio , I'm no longer contribute LoRaWan to zephyr any more, due to some reasons. I'm sorry to confirm that this PR will never be merged. But you can create a new PR from this or write it from scratch to contribute to zephyr as @carlescufi mentioned, that would be very nice.

@chris-schra
Copy link
Contributor

chris-schra commented Sep 3, 2018 via email

@carlescufi
Copy link
Member

carlescufi commented Sep 3, 2018

@derchrismakaio

Alright, I‘ll give it a try.

Brilliant!

What should happen to the LoRaWan-node repo?

I believe that the repo is 3-clause BSD, and so it can only live in Zephyr's ext/ folder. Typically we maintain unchanged external repos there, so if you need a "fork" with changes you either submit them upstream to the loramac-node repo or we create one under Zephyr's GitHub account.

EDIT: We are currently working towards supporting external repositories in Zephyr without having to copy their contents in the ext/ folder.

@chris-schra
Copy link
Contributor

I tried to put it in ext/ but problem is that I have to override the CMakeLists.txt.
I thought I could do it from within subsys/lorawan/CMakeLists.txt via

add_subdirectory_ifdef(CONFIG_LORAWAN ${PROJECT_SOURCE_DIR}/ext/lorawan)

but that would require a binary_dir

Alternative - if good practice: I could restructure the directories like this...

ext/lorawan/

  • (our) CMakeLists.txt

ext/lorawan/LoRaMac-node

  • the untouched LoRaMac-node repo

@carlescufi
Copy link
Member

@SebastianBoe would you mind replying to this comment

@carlescufi
Copy link
Member

@derchrismakaio I would probably go with the alternative you mention, seems cleaner to me.

@vanwinkeljan
Copy link
Member

@derchrismakaio Don't know if it is of any help but cmake has support to import external projects and it is already used in zephyr (e.g. zephyr/subsys/net/lib/openthread/CMakeLists.txt)

@SebastianBoe
Copy link
Collaborator

I tried to put it in ext/ but problem is that I have to override the CMakeLists.txt.

See ext/lib/ipc/open-amp for how this has been done before.

Seems that it the second alternative that has worked well before.

Don't know if it is of any help but cmake has support to import external projects and it is already used in zephyr (e.g. zephyr/subsys/net/lib/openthread/CMakeLists.txt)

ExternalProject (recursively invoking build systems) should only be done when it is infeasible to integrate/port the build system. I'm not familiar with LoRaMac so I can't speak to what should be done with it.

@chris-schra
Copy link
Contributor

@SebastianBoe this is actually what I did when I started to work yesterday ;)
I'm not sure what should go to subsys though.

What's the best way to post code excerpts here?
I've constructed (well, copied it from the lib) some sort of an interface which defines the Radio functions (RadioInit, RadioGetStatus, etc)
I guess the LoRa radio drivers will be separate from ext and subsys but included in drivers/lora eg
And these drivers could "implement" the Radio interface. And AFAIK that's what the subsys is for (telling from having a short glance at the bluetooth stuff, but I'm not sure).
Please also note that I'm still new to C/CPP (1yr experience only) and natively I'm rather the C# guy, so I will need a lot of review and guidance - apologies for that!

@chris-schra
Copy link
Contributor

chris-schra commented Sep 4, 2018

urgh, okay, I've built the skeleton but it lacks implementation, of course.
We (as company) won't need the actual LoRaMac-node repo because we're using Microchip's RN2483 which already has the MAC implementation. But I think it would be great to keep both in mind: LoRa devices and LoRaWAN devices (I don't like the MAC as it doesn't really distinguish between both device classes, what do you think?).

So here's my concept:

Interfaces
subsys/lora/lora_radio.h (which can then bridge to LoRaMac-node repo which is prepared)
subsys/lora/lorawan_radio.h (which implements the LoRaWAN stuff)

I recommend moving from lorawan dir to lora (because that was what was really confusing me).

As most LoRaWAN devices (like the RN2483) bring both layers, I'd try to implement both but with focus on LoRaWAN.

Drivers
drivers/lora/rn2483.h for the actual modem implementation of RN2483

Shell
As there are things to setup before one can start working with the modem (like device keys, etc), I'd create a lora_shell.c in drivers/lora/ and take @mike-scott modem_shell.c as a starting point

Setup
I'm still wondering how the device setup should happen. For example, for me the relation between board and (peripheral) device is flexible. @mike-scott did put the modem device fixed on a bus for the pca10056 which I understand and which works. But it gave me a hard time when just starting.
The RN2483 has some GPIO pins which are not specific to LoRa, so I would add them in drivers -> lora -> RN2483 to be filled out. But what I really liked was the possibility to select a bus. The RN2483 is an UART device and our board (based on nRF52840) exposes two UART drivers. Is there a way to make menuconfig let me choose between the activated UART drivers? Maybe even independent from the board? So if a board with only one UART is selected, I can only choose UART0? With independent I mean here, that I don't want to put a lot ifs in the Kconfig a la "select UART_MCUX_2 if BOARD_FRDM_K64F"
Shouldn't all boards just simply expose UART0, UARTE0, UART1, UARTE1 if available? I can see that the DTS of PCA10056 does it with
#define CONFIG_UART_0_NAME NORDIC_NRF_UARTE_40002000_LABEL

And another question:
the existing Semtech's lora_radio.h seems fine and I would use it in the subsys. But you say it would have to stay untouched then, even if moved out of the ext/ folder, right?

@SebastianBoe
Copy link
Collaborator

SebastianBoe commented Sep 4, 2018

Is there a way to make menuconfig let me choose between the activated UART drivers?

If the activation is done through Kconfig, and not DT, then yes. It sounds like you would need a choice statement. If the activation is not available in Kconfig you would need to duplicate the information in Kconfig, this is a workaround to #7302

See #9499 (comment) for how to have a selection of options with the default selection being affected by the board choice.

EDIT: I can't speak of how to organize the system into drivers and subsystems as I am not an expert in that area.

@carlescufi
Copy link
Member

What's the best way to post code excerpts here?

This is an old PR. What I would suggest is that you create a new PR with a basic proof-of-concept and then people will be able to comment inline.

@miyatsu
Copy link
Contributor Author

miyatsu commented Sep 4, 2018

@derchrismakaio Would you mind loop me in when you create an new PR of this?

@chris-schra
Copy link
Contributor

@miyatsu of course I'll do, but keep in mind that I won't focus on the MAC layer (which I think you need)

@Mani-Sadhasivam
Copy link
Member

@chris-makaio Did you get time to create a new PR out of this? I'm also working on LoRa node for one of our 96Boards and might get along with you. Please let me know!

@chris-schra
Copy link
Contributor

@Mani-Sadhasivam I didn't create a new PR on this yet because I'm working on different drivers currently, but will need LoRa modem drivers in future. I'm happy to discuss things

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: LoRa DNM This PR should not be merged (Do Not Merge)
Projects
None yet
Development

Successfully merging this pull request may close these issues.