Skip to content

Conversation

@chris-schra
Copy link
Contributor

This is the first rough proof of concept of adding a LoRa layer.
Sorry for unnecessary files like .gitignore, I'll clean this up as soon as I prepare an actual production PR.

Concept:

  • lora_context is the entry point
  • keep LoRa and LoRaWAN seperate
  • There are a lot of devices based on UART commands, thus, lora_context_uart is meant as their base
  • I don't see a reason why there would have to be multiple context instances, well, maybe for a nano gateway but that's out of scope for me
  • there are basic events which should be implemented by every application and would go in the lora_context_cb, on the other hand there are callbacks (like I demo'ed for get version) which are rather callbacks then events
  • @miyatsu suggested to port LoRaMac-node which I'd love to see - although I can't do it due to a lack of supported devices and need. But basically they could go to a new lora_context_loramacext or so - if required, I've already tried to create a directory tree for this which is not included in this PR to keep things clean

To discuss:

  • I suggest abstracting the UART peripherals a bit more, that's why I introduced eg HW_UART0_ENABLED which should be selected from any board-specific UART driver's submenu, if possible
  • I feel it is problematic to kind of "hide" the UART peripheral's baud rate. I was testing and testing, no response from my RN2483 until I finally remembered to change the baud rate. I think it would be better to be able to configure the baud rate in the Serial driver menu, depending on count of UART peripherals selected

Requests (apart from comments):

  • I like @mike-scott 's UART device logic implemented for the modem drivers. Is it possible to move that to a utility class?

@codecov-io
Copy link

codecov-io commented Sep 6, 2018

Codecov Report

Merging #9822 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9822   +/-   ##
=======================================
  Coverage   52.24%   52.24%           
=======================================
  Files         212      212           
  Lines       25949    25949           
  Branches     5593     5593           
=======================================
  Hits        13558    13558           
  Misses      10155    10155           
  Partials     2236     2236

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 19845a7...3982537. Read the comment docs.

@mike-scott
Copy link
Contributor

@derchrismakaio FYI, @agross-linaro is also working on a Modem driver and I think together we could identity functions which belong in a utility class.

@@ -0,0 +1,43 @@
.. _hello_world:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a copy of the existing hello_world example (found in samples/hello_world/README.rst). If you're using this as a starting template, great. But this PR can't be merged as it is, and will get doc build errors because a doc with the name _hello_world already exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the reply, but the PR isn’t meant for merging yet but rather for discussion of the concept

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Ping me when it's ready for a doc review.

…into add_lora

# Conflicts:
#	drivers/serial/Kconfig
#	subsys/net/buf.c
#	subsys/shell/shell.c
@nashif nashif added the DNM This PR should not be merged (Do Not Merge) label Sep 23, 2018
@chrta
Copy link
Contributor

chrta commented Oct 11, 2018

It would be nice if the structure of the lora driver would allow to easily add support for SPI-based lora transceivers (like the SX1272) later on.

Copy link
Contributor

@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.

It's quite messy. You will have to step back a little bit, and rework your patch-set so every patch comes with logical modifications (and proper title/message). If you have an idea on how you want to design it, please document that. Currently it's hard to understand what you are trying to achieve.

Also, please, apply asap proper code style, as well as use proper logging system (no printk etc...). New logging system also provides hexdump functionality.

@pfalcon
Copy link
Contributor

pfalcon commented Oct 21, 2019

For reference, nowadays there's also another PR for LoRa functionality: #18998 (which is itself an update of #17685 linked above).

@wentongwu wentongwu self-requested a review October 22, 2019 02:05
@SebastianBoe SebastianBoe removed their request for review October 22, 2019 08:59
@galak
Copy link
Contributor

galak commented Apr 17, 2020

closing this as we have lora and modem functionality in tree. Feel free to re-open and update if desired.

@galak galak closed this Apr 17, 2020
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.

10 participants