RFC: Add initial LoRa support #17685
Conversation
Mani-Sadhasivam
requested review from
carlescufi,
erwango,
galak,
MaureenHelm,
nashif and
superna9999
as code owners
zephyrbot
added
area: Devicetree
area: SPI
|
Thanks for this @Mani-Sadhasivam! I just have a couple of initial questions.
Thanks! |
|
CC @chris-makaio |
|
Hi @carlescufi
Good question! The existing SX127x drivers depend on their own SPI drivers which conflict with the SPI API available in Zephyr. I know that we can always add a commit on top to modify the external repositories but I thought of using the external library only for adding the LoRaWAN stack. IMO it makes sense to have native radio drivers in zephyr and depend on the external repo for LoRaWAN stack.
As I mentioned in the PR description, I wanted to start small and move on to the full implementation later. Since implementing the P2P mode is fairly easy as there is no dependency on LoRaWAN stack, I intentionally left it as a TODO for me or community.
Yes, that's the idea. Having only P2P functionality won't be useful and definitely, I'd like to add LoRaWAN in future.
|
|
We‘re using RN2483 and I‘d love to add a generic LoRa api for it. |
include/lora.h
Outdated
| @@ -0,0 +1,141 @@ | |||
| /* | |||
There was a problem hiding this comment.
We are trying to migrate away from single include files in include/ directory. The lora.h could be moved to include/net as there is also other radio technology headers there.
There was a problem hiding this comment.
@jukkar IMO this belongs to include/drivers.
There was a problem hiding this comment.
@jukkar IMO this belongs to include/drivers.
Could be, we already have IEEE 802.15.4 generic (non-driver) related headers in include/net, so in this respect the lora.h could be placed there. If the lora.h contains very driver specific stuff, then sure, include/driver is better place.
There was a problem hiding this comment.
@jukkar for me it looks like a driver ;-)
There was a problem hiding this comment.
Thinking about how this API will be used in future, I'd like it to be in include/net. Currently, it just deals with only plain driver stuff but for sure it will change soon.
There was a problem hiding this comment.
Ok. Thought this API is finished and the networking related stuff goes into a separate API.
There was a problem hiding this comment.
Thought this API is finished and the networking related stuff goes into a separate API.
TBH there is no clear separation of drivers/lora now. That's why I thought of placing it under net. Till we have the LoRaWAN implementation it would be difficult to guess how this API will evolve.
drivers/Kconfig
Outdated
| @@ -89,4 +89,6 @@ source "drivers/neural_net/Kconfig" | |||
|
|
|||
| source "drivers/hwinfo/Kconfig" | |||
|
|
|||
| source "drivers/lora/Kconfig" | |||
There was a problem hiding this comment.
Can you move the lora Kconfig setting after the drivers/ieee802154/Kconfig line so that we have various radio technologies near each other.
boards/arm/96b_wistrio/pinmux.c
Outdated
| @@ -33,8 +43,38 @@ static int pinmux_stm32_init(struct device *port) | |||
|
|
|||
| stm32_setup_pins(pinconf, ARRAY_SIZE(pinconf)); | |||
|
|
|||
| struct device *gpioa = | |||
There was a problem hiding this comment.
Please introduce all the variables at the beginning of the function.
| @@ -0,0 +1,14 @@ | |||
| cmake_minimum_required(VERSION 3.13.1) | |||
| macro(set_conf_file) | |||
There was a problem hiding this comment.
I think this is no longer needed, the build system will pickup board specific .conf files automatically.
| tests: | ||
| test: | ||
| platform_whitelist: 96b_wistrio | ||
| tags: LORA |
There was a problem hiding this comment.
move the tags to under the sample: block, you can also add "lora" tag together with "LORA" just in case the tag is case-sensitive.
There was a problem hiding this comment.
move the tags to under the sample: block
You mean as below?
sample:
description: Demonstration of LoRa Send functionality
name: LoRa Send Sample
tests:
sample.driver.lora_send:
platform_whitelist: 96b_wistrio
tags: LORA
you can also add "lora" tag together with "LORA" just in case the tag is case-sensitive.
Hmm. I think it makes sense to use just lora
There was a problem hiding this comment.
I meant something like this
sample:
description: Demonstration of LoRa Send functionality
name: LoRa Send Sample
tags: lora
tests:
sample.driver.lora_send:
platform_whitelist: 96b_wistrio
So that if we add new tests, then the tags are automatically set for them.
There was a problem hiding this comment.
Cool, thanks for the info. The reason I asked for the clarification is, I couldn't find tags under sample section in any existing samples.
There was a problem hiding this comment.
@jukkar Looks like CI is failing because of this change.
There was a problem hiding this comment.
Ah, indeed. The tags: needs to be under common: like this
common:
tags: lora
sample:
description: Demonstration of LoRa Send functionality
name: LoRa Send Sample
tests:
sample.driver.lora_send:
platform_whitelist: 96b_wistrio
Sorry for confusing advice, just came back from vacation :)
| @@ -0,0 +1,14 @@ | |||
| cmake_minimum_required(VERSION 3.13.1) | |||
There was a problem hiding this comment.
same comments to receiver as what I wrote for sender
Right, the current API assumes that the radio driver works in P2P mode by default. In future, we can have a separate function like |
|
Maybe you can already add it and return ENOTSUP for your driver? That gave me a starting point for my adoption. |
Sure. So we have two options for switching between modes:
|
Mani-Sadhasivam
force-pushed
the
lora-initial
branch
from
5f4e6b7 to
36b6db6
Compare
| @@ -0,0 +1,7 @@ | |||
| cmake_minimum_required(VERSION 3.13.1) | |||
There was a problem hiding this comment.
I missed this one in my first review. What about putting all the lora samples into samples/drivers/lora directory and then have send and receive subdirectories under it?
|
All checks are passing now. Review history of this comment for details about previous failed status. |
Add SPI1 support for STM32L1 controller series. Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
Add sample application for sending data packets over LoRa. Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
Add sample application for receiving data packets over LoRa. Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
Add CODEOWNERS entry for LoRa API, drivers and samples. Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
Mani-Sadhasivam
force-pushed
the
lora-initial
branch
from
56e894a to
a85fb7e
Compare
|
I vote for the lora_mode func |
|
@jukkar Incorporated your review comments! |
|
@Mani-Sadhasivam As far as I understand, LoRaWPAN uses LoRa as radio. Doesn't it make sense to implement a "RAW" LoRa API and use a shim to make it compatible with LoRaMAC-node? |
| # | ||
| --- | ||
| title: Semtech SX1276 LoRa Modem | ||
| version: 0.1 |
There was a problem hiding this comment.
Can remove version: (see #17681).
| type: string | ||
| category: required | ||
| constraint: "semtech,sx1276" | ||
| generation: define |
There was a problem hiding this comment.
Remove generation: .... No longer needed.
| type: int | ||
| category: required | ||
| description: Input clock frequency of the LoRa device | ||
| generation: define |
| reset-gpios: | ||
| type: compound | ||
| category: optional | ||
| generation: define, use-prop-name |
| dio-gpios: | ||
| type: compound | ||
| category: optional | ||
| generation: define, use-prop-name |
| { | ||
| int ret, i; | ||
|
|
||
| for (i = 0; i < data_len; i++) { |
There was a problem hiding this comment.
I think you don't need this loop. In the datasheet, there is the following statement:
FIFO access: if the address byte corresponds to the address of the FIFO, then succeeding data byte will address the FIFO. The address is not automatically incremented but is memorized and does not need to be sent between each data byte. The NSS pin goes low at the beginning of the frame and stay low between each byte. It goes high only after the last byte transfer.
So a burst transfer would bee sufficient here right?
| int ret, i; | ||
| u8_t regval; | ||
|
|
||
| for (i = 0; i < data_len; i++) { |
| return -EIO; | ||
| } | ||
|
|
||
| k_sem_take(&dev_data->data_sem, K_FOREVER); |
There was a problem hiding this comment.
Wouldn't it be a nice to have if you pass the timeout parameter as a function argument?
fro example:
static int sx1276_lora_recv(struct device *dev, u8_t *data, s32_t timeout)| } | ||
|
|
||
| regval &= ~REG_DIO_MAPPING1_DIO0_MASK; | ||
| regval |= 0x1 << 6; |
There was a problem hiding this comment.
Can you change this from magic numbers into defines like the other register values?
|
|
||
| /* Set frequency */ | ||
| freq_rf = config->frequency; | ||
| freq_rf *= (1 << 19); |
There was a problem hiding this comment.
You can safely ignore this comment, but I would prefer:
freq_rf = ((u64_t)config->frequency << 19) / CLOCK_FREQ;I think it's more clear, but maybe other people think differently :-)
| if (config->tx_power > sx1276_power[SX1276_PA_BOOST].max) | ||
| config->tx_power = sx1276_power[SX1276_PA_BOOST].max; | ||
|
|
||
| config->tx_power -= 3; |
There was a problem hiding this comment.
I'm not a LoRa expert, but is it safe to silently change a configuration that is passed by the user?
| regval &= ~SX1276_MODEM_CONFIG1_BW_MASK; | ||
| regval &= ~SX1276_MODEM_CONFIG1_CR_MASK; | ||
| regval &= ~SX1276_MODEM_CONFIG1_IMP_HDR; | ||
| regval |= ((config->bandwidth << 4) | (config->coding_rate << 1)); |
There was a problem hiding this comment.
Can you use something like SX1276_MODEM_CONFIG1_BANDWITH_POS instead of 4 here?
Same for the rate.
| } | ||
|
|
||
| regval &= ~SX1276_MODEM_CONFIG2_SF_MASK; | ||
| regval |= ((config->spreading_factor << 4) | 4); |
| config->tx_power = sx1276_power[SX1276_PA_BOOST].max; | ||
|
|
||
| config->tx_power -= 3; | ||
| ret = sx1276_write(dev, SX1276_REG_PA_DAC, 0x87); |
There was a problem hiding this comment.
Can you change the magic numbers into defines in general?
| LOG_ERR("Could not set gpio callback."); | ||
| return -EIO; | ||
| } | ||
| gpio_pin_enable_callback(data->dio0, GPIO_DIO0_PIN); |
There was a problem hiding this comment.
Add an empty line after {
There was a problem hiding this comment.
I am going to nack this for various reasons.
- LoRa drivers and stack already exist, maintained by SemTech (owner of this technology, which a huge +1 for porting there official stack) and are all BSD licensed. It's a lot of code and going for a full rewrite for zephyr only really would need way better reasons than given ones.
- SPI API (and/or GPIO one) is not a blocker for porting the original SemTech stack to Zephyr. It seems to me that has been too lightly studied before going to a full rewrite. I just looked at it 10min and saw it is fully possible, certainly a bit annoying on some corner case but nothing impossible, and far less complicated than rewriting everything.
- It looks like all prior discussion on LoRa has not been read. There were past attempts to port LoRa stack to Zephyr, there were even good starting points. Unfortunately PR owner never followed and it bit-rotted. But these were a good start for porting the official LoRa stack.
- Proposed API is not going to work when it will be time to get lorawan ported (unless the idea was to do some rewrite here again)
- That's a detail but LoRa is a standalone subsystem and is not part of subsys/net.
|
@tbursztyka Thanks for the review!
Where did I say that I'm going on for a full rewrite of the LoRa stack? I just mentioned that the radio drivers need to be present as native zephyr drivers and we can reuse the existing LoRaWAN stack from Semtech. If you are confident that the existing radio drivers can be reused (as @alexanderwachter already asked), I can take that approach. But I never said it is impossible to adapt the Semtech's radio drivers (there are only a few actually, see here). And you might wanna consider that this PR has been submitted as an RFC series, hoping for feedback from the community.
I did and I mentioned it in the PR description itself.
Again, I posted this RFC series to get a response from the community as the other PR was not moving forward. My approach might be wrong but that's the reason to have RFC series, right? When I started to look into the existing PR, I found only a couple of radio drivers (sx127x) present in the Semtech's repo (it makes sense because that repo is just an implementation of the LoRaWAN specification), so I thought of writing the radio drivers as a native Zephyr driver and depending on the Semtech's repo for the stack. Again, my intuition might be wrong here!
Yes, I agree that this API needs to be changed once we port LoRaWAN stack and I never assumed that this is complete.
|
|
Plus, I prefer having a non-Semtech starting point for other vendor drivers. |
Well, yes. The time you have spend on writing a driver from scratch would have been far sufficient to port semtech's repo :/ all in all it would pile up a lot of work, for no real benefit. Not to say about maintaining the whole.
It's just a matter for other vendor devices driver to implement the radio API. There is nothing semtech specific other than that. |
|
@tbursztyka |
|
@chris-makaio Let it be... I'll give it a try to port Semtech LoRaWAN stack as like the previous PR. And in the meantime, I'll keep this PR opened. Thanks everyone for the review! |
|
@carlescufi What would be the apt place to maintain Semtech's LoRaWAN stack as you suggested? And how can I do it using west? |
Depending on the size and complexity of the Semtech stack, you have 2 choices:
In principle we prefer modules, but if the code is small and self-contained enough having it in If you choose to go the module way, then you will need a change to zephyr's EDIT: See this commit as an example |
IMO it needs to be integrated as a module. I'll try the instructions. Thanks
|
| gpio_pin_write(reset_dev, GPIO_RESET_PIN, 1); | ||
| k_sleep(100); | ||
|
|
||
| ret = sx1276_read(dev, SX1276_REG_VERSION, ®val, 1); |
There was a problem hiding this comment.
I have a custom board using the STM32L151CBT.
Here, the regval value is not initialized, so my case is 0xE0.
VERSION is read correctly, but MOSI is outputting 0xE0.
So the waveform appears as below.
[u8_t regval;]
[u8_t regval = 0x00;]
|
How this relates to #9822 ? |
| @@ -328,7 +328,7 @@ static int spi_stm32_configure(struct device *dev, | |||
| LL_SPI_SetRxFIFOThreshold(spi, LL_SPI_RX_FIFO_TH_QUARTER); | |||
| #endif | |||
|
|
|||
| #ifndef CONFIG_SOC_SERIES_STM32F1X | |||
| #if !defined(CONFIG_SOC_SERIES_STM32F1X) && !defined(CONFIG_SOC_SERIES_STM32L1X) | |||
There was a problem hiding this comment.
Please split this commit into a dedicated PR.
|
Closing this PR since it is confusing to have 2 open PRs for same functionality. |
Ok, got it now :-). Txs! |
Hello,
This RFC series adds initial LoRa (Long Range low power) technology support to Zephyr (#5436). The initial support includes only P2P (Peer to Peer) support in LoRa mode and hence there is no LoRaWAN or FSK modulation support exist. And there is a good reason for starting with P2P mode since previous attempts to add LoRa support with LoRaWAN (#5764) proved to be tough. Hence this PR is submitted to discuss the basic structure of the LoRa API with something simple as a proof of concept.
The driver support in this PR includes Semtech SX1276 LoRa modem available in the 96Boards Wistrio board connected to internal STM32L1 controller via SPI. Also, there are 2 samples provided which demonstrate simple data send and receive transfers over LoRa in P2P mode.
Reviews are welcome :-)
Signed-off-by: Manivannan Sadhasivam mani@kernel.org