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

lora: Initial commit of LoRaWAN example #456

Merged
merged 7 commits into from
Nov 13, 2024

Conversation

alistair23
Copy link
Contributor

@alistair23 alistair23 commented Jul 14, 2024

This builds on top of #432 and adds a LoRaWAN example where a Tock system can send data to TTN.

@alistair23
Copy link
Contributor Author

Any ideas?

2024-07-14T10:37:45.7525104Z riscv64-unknown-elf-g++ -Wctor-dtor-privacy  -Wdelete-non-virtual-dtor  -Wold-style-cast  -Woverloaded-virtual  -Wsign-promo  -Wstrict-null-sentinel  -Wsuggest-final-methods  -Wsuggest-final-types  -Wsuggest-override  -Wuseless-cast  -Wzero-as-null-pointer-constant  -isystem ../../..//libradio/RadioLib/src -isystem ../../..//libradio/RadioLib/examples/NonArduino/Tock -I../../..// -DRADIOLIB_CLOCK_DRIFT_MS=9 -frecord-gcc-switches -gdwarf-2 -Os -fdata-sections -ffunction-sections -fstack-usage -D_FORTIFY_SOURCE=2 -Wall -Wextra -I../../../ -Wdate-time  -Wfloat-equal  -Wformat-nonliteral  -Wformat-security  -Wformat-y2k  -Winit-self  -Wmissing-declarations  -Wmissing-field-initializers  -Wmissing-format-attribute  -Wmissing-noreturn  -Wmultichar  -Wpointer-arith  -Wredundant-decls  -Wshadow  -Wunused-macros  -Wunused-parameter  -Wwrite-strings  -Wstack-usage=4096 -Wlogical-op  -Wtrampolines  -isystem ../../..//lib/libtock-newlib-4.2.0.20211231/riscv/riscv64-unknown-elf/include -isystem ../../..//lib/libtock-libc++-10.5.0/riscv/riscv64-unknown-elf/include/c++/10.5.0 -isystem ../../..//lib/libtock-libc++-10.5.0/riscv/riscv64-unknown-elf/include/c++/10.5.0/riscv64-unknown-elf -march=rv32imac -mabi=ilp32 -mcmodel=medlow -c -o build/rv32imac/CayenneLPP.o CayenneLPP.cc
2024-07-14T10:37:45.7594123Z In file included from CayenneLPP.h:20,
2024-07-14T10:37:45.7594817Z                  from CayenneLPP.cc:16:
2024-07-14T10:37:45.7596462Z ../../..//lib/libtock-libc++-10.5.0/riscv/riscv64-unknown-elf/include/c++/10.5.0/cstdlib:75:15: fatal error: stdlib.h: No such file or directory
2024-07-14T10:37:45.7597996Z    75 | #include_next <stdlib.h>
2024-07-14T10:37:45.7598516Z       |               ^~~~~~~~~~
2024-07-14T10:37:45.7599024Z compilation terminated.
2024-07-14T10:37:45.7608222Z make: *** [../../..//AppMakefile.mk:340: build/rv32imac/CayenneLPP.o] Error 1
2024-07-14T10:37:45.7635538Z ##[error]Process completed with exit code 2.
2024-07-14T10:37:45.7718454Z Post job cleanup.

Copy link
Member

@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

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

This is cool! TTN is great for having an 'out-of-box' working example. If I can find a minute, I'll try to do a similar for Helium at some point (which has much better penetration in North America than TTN).

examples/lora/sensor-lorawan/CayenneLPP.cc Outdated Show resolved Hide resolved
examples/lora/sensor-lorawan/main.cc Outdated Show resolved Hide resolved
@alistair23 alistair23 force-pushed the alistair/lora-wan branch 3 times, most recently from 5e43d89 to 1a6ee32 Compare July 19, 2024 10:53
@alistair23
Copy link
Contributor Author

CI fixed!

@ppannuto ppannuto added the blocked Blocked on promised changes or other PRs label Jul 23, 2024
@ppannuto
Copy link
Member

Marking as blocked until we can resolve the GPL question.

@tock/core-wg This is something we need to reason through, either on a core team call, or out of band.

tl;dr:

  • Is it reasonable to have GPL'd code in the main tock/libtock-c repo?
  • Does it change the discussion if GPL code is restricted to only be allowed in individual examples folders?
    • Or even perhaps a examples/license-restricted/... subfolder?

@alevy
Copy link
Member

alevy commented Jul 23, 2024

Is it reasonable to have GPL'd code in the main tock/libtock-c repo?

I don't think anyone is reasonably getting in any trouble here as long as they either don't depend on this library for a distributed program, or license any program that does depend on it under GPL, but the question is reasonable and understanding the answer is probably important.

My interpretation is that including it in this manner is fine. Anyone who doesn't use it in their program won't actually link against it, "components" in the libtock-c repo are ultimately licensed individually anyway (primarily under MIT/Apache), and the affects for an actual program compiled/distributed from this repo depend on what is actually distributed and what it depends on.

I'd be more concerned if we made any core parts of userland depend on GPL code.

If C had a good package management story, we would anyway just be saying that programs should depend on this other library, distributed elsewhere to use it, which would have the same implications and I don't think the question would come up. In this case, we'd just be hosting a copy of that library in the same repo. I don't see a significant difference.

Basically my vote is that this is fine. It's only actually a dependency if it's used for a particular application (in which case that application as a whole I believe would need to be distributed under GPL).

Does it change the discussion if GPL code is restricted to only be allowed in individual examples folders?

I think it's cleaner if the GPL code lives exclusively in a single example, less clean but perfectly good (and more reasonable) if it is only ever used/linked from individual example applications under the examples folder, and a red flag if we start linking against it from dependencies of applications (don't see that happening here).

Or even perhaps a examples/license-restricted/... subfolder?

I think this would be overkill.

TockLibrary.mk Outdated Show resolved Hide resolved
examples/lora/sensor-lorawan/Makefile Outdated Show resolved Hide resolved
examples/lora/sensor-lorawan/Makefile Outdated Show resolved Hide resolved
examples/lora/sensor-lorawan/Makefile Outdated Show resolved Hide resolved
examples/lora/sensor-lorawan/Makefile Outdated Show resolved Hide resolved
@alevy
Copy link
Member

alevy commented Aug 1, 2024

OK, looking into this a bit more deeply, two things:

  1. I think the GPL thing is a non-issue the way that this PR uses it. Perhaps worth sticking a notice in the relevant example apps.
  2. This PR makes apps/libraries that are highly hardware specific (the specific lora chip configured for a specific board that uses particular pins) in a way that is really unnecessary. Let me explain...

RadioLib is implemented in a way that allows for pluggable hardware implementations via the PhysicalLayer class, which the SX1262 class instantiated in the examples is a subclass of.

A trivially portable version of this would implement an instance of PhysicalLayer that interacts with a system call driver---this would be akin to the way we use OpenThread on top of an abstract 802154 syscall driver, and would allow a LoraWAN application to run on any board + module configuration.

Now, I realize that implementing a driver for the SX1262 is non-trivial. It's ~1000 LoC in RadioLib. lora-rs has an implementation in slightly less, it does not seem particularly timing sensitive or subtle, but nonetheless there are a lot of details to get right.

Now, it's clearly possible to use Tock + libtock-c to build these kinds of applications (as the PR clearly demonstrates). It's perfectly fine to have applications out in the world that don't adhere to the particular layering choice that we advocate upstream, and generally supporting exporting a SPI device (in this case) directly to userspace is good to support for that reason.

However, I think the question is: do we want to have these kinds of examples upstream? No doubt we already have a few, but I'd argue those are basically legacy or tests and should be removed (or retained merely as tests).

I suggest there are at least two options:

  1. No. These kinds of applications need to live elsewhere.
  2. No, but there can be in-progress work that lives in upstream. E.g., an SX1262 implementation and Lora device system call driver is in the process of being implemented in the kernel, until that's ready, we have an example that uses the C implementation in userspace, but progress is being made to change that.

@bradjc
Copy link
Contributor

bradjc commented Aug 1, 2024

However, I think the question is: do we want to have these kinds of examples upstream? No doubt we already have a few, but I'd argue those are basically legacy or tests and should be removed (or retained merely as tests).

I agree, but I'm also torn. It took a year implementing the lr1110 stack using the same SPI/GPIO HAL interface (#458). The examples in that would too only work for one radio chip. The radio lib driver for that is massive (and is also very new and didn't exist when we started). Like OpenThread, I kind of feel like these are necessary evils to make progress on other networking goals.

@alistair23
Copy link
Contributor Author

  • This PR makes apps/libraries that are highly hardware specific (the specific lora chip configured for a specific board that uses particular pins) in a way that is really unnecessary. Let me explain...

Have you read the entire PR? This is specifically not board specific (obviously you need a LoRa radio).

The GPIO pin mapping and SPI allocation is handled in the kernel. So this application can run on any board with a SX1261. main.rs in the kernel maps the GPIO pins and SPI bus, so that the application doesn't have to worry about it.

I agree that this example only supports the SX1261 module. It is easy to change the module in RadioLib to support any module that RadioLib supports, such as the LR1110 for example.

It obviously would be great to have the module support in the kernel. But that's yet another thing that needs to be re-written in a Tock way. So until someone goes ahead and does that I think this is a good start and works well. This implementation doesn't stop anyone from writing an implementation in the kernel and migrating to that in the future.

It's probably also worth noting that the #458 implementation only works on a single board. So maybe that needs more scrutiny?

@alevy
Copy link
Member

alevy commented Aug 2, 2024

@alistair23 from the code:

  // now we can create the radio module
  // pinout corresponds to the SparkFun LoRa Thing Plus - expLoRaBLE
  // NSS pin:   0
  // DIO1 pin:  2
  // NRST pin:  4
  // BUSY pin:  1

Have you read the entire PR? This is specifically not board specific (obviously you need a LoRa radio).

I have read the entire PR. Though I'm realizing now, that I had not read the RadioLib Tock adaptation library, which depends on libtock-c, which is a convoluted enough dependency that I admit I don't really understand now what happens with those pins that are passed in.

It's probably also worth noting that the #458 implementation only works on a single board. So maybe that needs more scrutiny?

I agree, and I have not yet really looked at that PR, though one important difference is that #458 adds tests only, while this is moving examples from wip to regular end-to-end examples.

@alistair23
Copy link
Contributor Author

I have read the entire PR. Though I'm realizing now, that I had not read the RadioLib Tock adaptation library, which depends on libtock-c, which is a convoluted enough dependency that I admit I don't really understand now what happens with those pins that are passed in.

Fair. The comment is trying to describe that this is what I tested with.

I did also test with a SX1261 externally attached via SPI, which also works. That does require changes to main.rs in the kernel though for the different SPI bus and GPIO pins.

The code in Tock where the GPIO lines are mapped looks like this

    let sx1262_gpio = components::gpio::GpioComponent::new(
        board_kernel,
        LORA_GPIO_DRIVER_NUM,
        components::gpio_component_helper!(
            apollo3::gpio::GpioPin,
            0 => &peripherals.gpio_port[36], // H6 - SX1262 Slave Select
            1 => &peripherals.gpio_port[39], // J8 - SX1262 Radio Busy Indicator
            2 => &peripherals.gpio_port[40], // J9 - SX1262 Multipurpose digital I/O (DIO1)
            3 => &peripherals.gpio_port[47], // H9 - SX1262 Multipurpose digital I/O (DIO3)
            4 => &peripherals.gpio_port[44], // J7 - SX1262 Reset
        ),
    )
    .finalize(components::gpio_component_static!(apollo3::gpio::GpioPin));

I agree, and I have not yet really looked at that PR, though one important difference is that #458 adds tests only, while this is moving examples from wip to regular end-to-end examples.

#432 is actually the PR that moves LoRa to examples. The PR backlog is so large that PRs are starting to build on each other :)

For clarity, this was originally in examples and moved out because it didn't support RISC-V (which it does now)

@ppannuto
Copy link
Member

ppannuto commented Oct 2, 2024

@alistair23 Once #432 is in, can you rebase this? I think my disposition is similar to Brad, i.e., networking stuff is hard and a working example has real value, even if it's a bit less hardware agnostic / purely layered than some idea thing might be, but I'll look at that more in depth early next week.

@alistair23
Copy link
Contributor Author

Rebased, this is just a single commit and can be merged once #432 is

@ppannuto ppannuto removed the blocked Blocked on promised changes or other PRs label Oct 25, 2024
@ppannuto
Copy link
Member

Okay! #432 is in, but this needs one more quick rebase now it looks like.

@alistair23
Copy link
Contributor Author

Okay! #432 is in, but this needs one more quick rebase now it looks like.

Done

@alistair23
Copy link
Contributor Author

PR opened: jgromes/RadioLib#1313

It breaks their CI though, as there is a backwards incompatible name change.

I have updated this PR to use the new names and not break the CI. So once this is merged I can get jgromes/RadioLib#1313 merged and fix everything up.

Let's try not to block this for too long

@alistair23
Copy link
Contributor Author

Ping! It'd be good to get this in so then I can get jgromes/RadioLib#1313 merged

@jgromes
Copy link

jgromes commented Nov 7, 2024

@ppannuto hey there! Just my two cents on one of the points raised:

It's a bit weird that the Tock HAL file is in the RadioLib repo and not in the libtock-c repo IMHO, but that seems to be the norm for the RadioLib project

It probably makes more sense to move the Tock HAL file to the tock project itself. The HAL files present in RadioLib started out as examples that turned out to be more easy to maintain as part of the project source. However, unlike the other HALs which are pretty generic (e.g. for Raspberry Pi), the Tock HAL is very specific. So I'm not opposed to moving it, in fact I would support it ;)

alevy
alevy previously approved these changes Nov 12, 2024
@alevy
Copy link
Member

alevy commented Nov 12, 2024

@ppannuto can we maybe move forward with this and kind of iterate our way past the circular dependency?

@ppannuto
Copy link
Member

I pushed an update that copies over my updated RadlioLib Tock HAL to the libtock-c repo, and has the application use our HAL to build.

@alistair23 This is compile-tested only; sorry, I don't have this hardware on-hand.

alevy
alevy previously approved these changes Nov 13, 2024
@alevy alevy dismissed ppannuto’s stale review November 13, 2024 01:31

Presumably you fixed your concern?

@alevy alevy added waiting-on-author and removed blocked-upstream Waiting on something from an upstream project labels Nov 13, 2024
@alevy
Copy link
Member

alevy commented Nov 13, 2024

OK, I think we just want @alistair23 to confirm this still works.

@alistair23
Copy link
Contributor Author

OK, I think we just want @alistair23 to confirm this still works.

Still works!

@ppannuto ppannuto added this pull request to the merge queue Nov 13, 2024
Merged via the queue into tock:master with commit c0202f9 Nov 13, 2024
2 checks passed
@alistair23 alistair23 deleted the alistair/lora-wan branch November 13, 2024 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants