-
Notifications
You must be signed in to change notification settings - Fork 852
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
Consolidate all LORA physical layer implementations behind one API #1023
Comments
Commenting here instead of #1041
I agree on the approach. However, in terms of where this happens, I think we should leave board-specifics out of embassy-lora itself, and perhaps surface the orchestrator concept, at least initially, in a separate create (some kind of BSP?). Most likely outside the embassy main repo? I think what you're describing is not only a solution for lora specifically, but a pattern that applies to other types of networking. With all these traits its also a potential downside of making the APIs a bit harder to use, so keeping embassy-lora somewhat focused on the drivers + integrating with rust-lorawan I think is the appropriate scope for that, because some might want to use it at that level rather than binding to some orchestrator for their board. |
Adding additional comment moved from #1041: Perhaps a better way to envision the "orchestrator" is as a builder which returns a LoRa structure supporting the API for an MCU/LoRa chip combination. Since the only current interfaces to Semtech chips of which I am aware are SPI and pin setting/checking, the hope is to hide any dependencies on STM32WL or nRF52 for this behind the LoRa API. |
1064: Fix LoRaWAN PHY settings for SX126x driver r=lulf a=jbeaurivage While working on #1023 / #1041, I noticed that the `lorawan_device::PhyTxRx` implementation does not conform to the LoRaWAN standard, and therefore devices using this driver could never communicate with a gateway. This PR backports the changes I've made to fix the offending parameters, and I can confirm that the driver now works with LoRaWAN networks. * Set preamble length to 8 symbols * Set polarity to inverted for received messages Co-authored-by: Justin Beaurivage <justin@wearableavionics.com>
1060: feat: embassy-usb-logger and example for rpi pico r=Dirbaio a=lulf * Add embassy-usb-logger which allows logging over USB for any device implementing embassy-usb * Add example using logger for rpi pico 1064: Fix LoRaWAN PHY settings for SX126x driver r=Dirbaio a=jbeaurivage While working on #1023 / #1041, I noticed that the `lorawan_device::PhyTxRx` implementation does not conform to the LoRaWAN standard, and therefore devices using this driver could never communicate with a gateway. This PR backports the changes I've made to fix the offending parameters, and I can confirm that the driver now works with LoRaWAN networks. * Set preamble length to 8 symbols * Set polarity to inverted for received messages Co-authored-by: Ulf Lilleengen <lulf@redhat.com> Co-authored-by: Justin Beaurivage <justin@wearableavionics.com>
1064: Fix LoRaWAN PHY settings for SX126x driver r=Dirbaio a=jbeaurivage While working on #1023 / #1041, I noticed that the `lorawan_device::PhyTxRx` implementation does not conform to the LoRaWAN standard, and therefore devices using this driver could never communicate with a gateway. This PR backports the changes I've made to fix the offending parameters, and I can confirm that the driver now works with LoRaWAN networks. * Set preamble length to 8 symbols * Set polarity to inverted for received messages Co-authored-by: Justin Beaurivage <justin@wearableavionics.com>
@jbeaurivage, @lulf: I am back after a 3 week hiatus. I am making a note here regarding the fix in #1064. We will need to consider the setting of the polarity bit for both the LoRa P2P case and the LoRaWAN case. I assume that for the P2P case inverted polarity should not be set. Perhaps the P2P case should not use PhyTxRx. |
Agreed; considering that |
It appears that to this point the Embassy emphasis within lora has been on the LoRaWAN use case, so I'm not sure whether PhyTxRx should be solely a LoRaWAN construct when P2P is considered. In any case, I will need to adjust the Embassy repository's P2P examples sooner rather than later to support our decision. LoRa P2P is quite useful. I believe it supports Meshtastic's off grid communication project, and I have used it in order to hand my neighbor a device that receives and reports - as an example - my home's temperature while I am away. Our homes are separated by a significant distance, so it helps that they can be alerted to a malfunctioning furnace remotely. |
Oh no doubt, all I'm saying is that |
So, the original intent for embassy-lora was to be an integration point between lorawan-device and embassy HALs. As I think you correctly point out, there is a useful use case for LoRa P2P as well. An alternative to adding a However, is also some work ongoing in rust-iot/radio-hal#24 - and I'm wondering if time would be better spent making that work with async/embassy, as it would probably cover most of the functionality for LoRa p2p. The additional work would then to make the lorawan-device work with the 'radio-hal' drivers. At the time when embassy-lora was created, there was no embedded-hal-async, so keeping it inside embassy was the easiest. Now I think maybe more of the drivers could live in radio-hal (which would need to use embedded-hal-async), and only the bits that depend on embassy HALs need to live in the embassy repo. Thoughts? |
It should be easy enough for me to open a separate issue to fix the LoRa P2P examples to use Sx1262xRadio's lora implementation directly. Hopefully the automatic GitHub reference placed in the pertinent radio-hal pull request will gauge interest within that team. The decision comes down to what functionality Embassy itself should encompass. There was a decision to place the BLE implementation represented by nrf-softdevice outside Embassy. LoRa also provides device-device communication, so maybe it should also move out of Embassy. And then what does that mean for embassy-net? However, given the trouble I've had getting both LoRa and BLE functionality to cargo up together to use the full functionality of the RAK4631 module, I'd currently vote to keep LoRa in Embassy and move BLE in there :). To be fair, I haven't put much effort into hammering LoRa/BLE together and realize prospects will improve as these features head toward release. |
First of all, let me say I'm very grateful for your contributions to embassy-lora and embassy, and I hope you will continue to feel motivated for that. So please take my disagreement with your stance only as a disagreement. And in any case, it's just my personal opinion, I'm happy for things to go in a different direction if that's what other maintainers want.
My take is that Rust/Cargo already has a mechanism for writing independent libraries that compose with traits + crates.io. IMO the reason for keeping a module in the embassy repo itself is that it a. is not releasable (yet) and depended on by the HALs or the executor The downside of having a module in embassy repo is that it increases the review, CI and release bottleneck of the module itself. The reason why embassy-net and embassy-lora is in the tree is a bit historical: there was no public embedded-nal-async, embedded-io, embassy-time or embassy-sync crates. However, embassy-stm32 depends on embassy-net, which is something you don't have for embassy-lora. The reason why nrf-softdevice is not in embassy I don't know, but I think it makes sense to keep it separate, especially now that it doesn't directly depend on embassy other than utlities like embassy-sync, and softdevice also being deprecated by Nordic, so it doesn't really make sense to add that now. Now that embedded-hal-async exists, drivers like sx127x or sx126x only really needs to rely on those gpio and SPI traits. The added benefit is that those drivers can be used in frameworks like RTIC or on an ESP32 easily without depending on embassy, which is a win-win for everyone. Keeping them in embassy will make it perceived to be embassy-specific, and it makes changes and releasing those drivers a slower process. Keeping them in a separate repo will be easier because there will be a smaller set of maintainers that are focused on those drivers instead. stm32wl is a bit of a special case where there is a dependency on embassy-stm32, but I don't think it should drive the direction. I'm sure we can figure something out for that.
To summarize my stance: I think that would be moving in the wrong direction, and you loose the benefits you get when there is a dependency manager like cargo, an ecosystem like crates.io, and the standardized traits where you can mix and match. I'm not against having examples in embassy that tie everything together. In any case I think @Dirbaio should have the final say in where we're heading. |
Excellent response! I do not have the big picture here, and this response helps. While I'll start implementing the LoRa consolidation feature (the root subject of this issue) in Embassy itself, it should be easy to move it to a better crate as this shakes out. I will also review the embedded-hal-async crate in detail to ensure the LoRa consolidation feature relies on it for SPI and GPIO. |
Milestone on the path to creating one LoRa driver base: a STM32WL device successfully sends P2P packets to a RAK4631 (nrf52480) device using basically the same sx126x implementation recently added to Embassy and now being developed as a separate package. Since the ST folks decided to go away from using regular pins for LoRa chip control, the existing Embassy subghz code informed the specific implementation of the following trait for STM32WL: pub trait InterfaceVariant { The implementation of these functions will be dependent on the RTOS and the chip. The actual SPI implementation will be dependent on the RTOS and chip also, but the LoRa driver requires embedded_hal_async::spi. Hopefully this will allow the LoRa driver base to move outside Embassy, with Embassy providing sample usage through STM32WL, STM32L0, and NRF examples. As indicated in a previous question on the Embassy chat, the STM32WL SUBGHZSPI ST implementation is “different”. Given that I didn’t receive a response to my question there about getting it hooked up through an async interface (DMAMUX1, but no DMA TX/RX), I wrapped code from the existing Embassy subghz SPI implementation with BlockingAsync for the time being. That satisfies embedded_hal_async::spi. There is still much to do to modify the “C-like” sx126x implementation into a “Rust-like” generic implementation. Several of the Semtech LoRa chips follow the same basic opcode/register processing as the sx126x. However, the sx127x opcode/register processing is significantly different. |
Adding the following to pub trait InterfaceVariant:
since there is no guarantee the antenna switching logic (the old RadioSwitch) will always be driven by a control pin. |
I think that some of the SPI operations benefit from async, and some not. I wonder if it is possible to make the API use blocking fn for things like enable_rf_switch, but maybe async for "tx this buffer"? It would require templating each fn using the appropriate SPI interface, and pass a &mut of the Spi Bus, maybe that makes the code very complex. |
The initial version of the implementation is now on https://github.com/ceekdee Separate LoRa package (lora):
Embassy updates (embassy):
|
@lulf: See the previous comment for some background information. Updates follow. Enough of the LoRa physical layer processing refactor is completed to allow a discussion if this is on track to:
In short, is this on the way to meeting the goals we have discussed? The evolving LoRa physical layer API (only flushed out so far for the SX1261/2 Tx path; see the commented out code to get an idea of what remains) is here: https://github.com/ceekdee/lora/blob/main/lora/src/lib.rs A stm32wl example using it (Tx only) is here: https://github.com/ceekdee/embassy/blob/master/examples/stm32wl/src/bin/lora_p2p_send.rs An nrf example (setup only) is here: https://github.com/ceekdee/embassy/blob/master/examples/nrf/src/bin/lora_p2p_receive.rs The trait for the interface between the MCU and LoRa board is here: https://github.com/ceekdee/lora/blob/main/lora/src/mod_traits.rs (the InterfaceVariant trait) A couple of implementations are here: https://github.com/ceekdee/embassy/blob/master/embassy-stm32/src/lora.rs Note that these additions to Embassy allow the existing Embassy LoRa implementations and examples to co-exist with the new lora crate. The transition between embassy-lora and a separate lora crate for the guts of LoRa physical layer processing should be reasonably straight-forward. Each kind of LoRa chip will have a separate implementation of the RadioKind trait: https://github.com/ceekdee/lora/blob/main/lora/src/mod_traits.rs (only fleshed out for SX1261/2 for the Tx functionality currently). I am reasonably confident that the three functions here: https://github.com/ceekdee/lora/blob/main/lora/src/interface.rs will handle all of the known ways (OpCode vs. Register methods) that Semtech currently uses for their SX126x and SX127x chips. In order for this new lora crate to be adopted:
While there is much left to be implemented, I hope enough is there to start discussing architecture and course corrections. I have been working with nrf52840 and stm32wl55 with sx1262, and have an STM32L0 with sx1276 on order. I also have an rp2040 on order, and once I understand how to get that to work with Embassy, I plan to order some Semtech chip for that. |
I think this is spot on, thanks for putting in the work so far 🚀
I think all of the above makes sense in general and I won't nitpick on stuff right now. This very much aligns with my interpretation of what we agreed!
I think that's a valid goal. On using/requiring reexporting types from lora, I think requiring something like
I think hosting the repo in the embassy org could be fine if you're OK with that. Otherwise as long as it gets maintained I think it can live anywhere.
There should be plenty of examples for that, and it implements embedded-hal-async already so hopefully you should be good to go :) Thanks for putting this amazing amount of work, it's great to see it all coming together! |
We are at the next discussion point for this implementation. There is much context above, but the README file and associated documentation is now the best reference: https://github.com/ceekdee/lora Questions:
https://github.com/rust-embedded but I am new to the embedded community and do not know the challenges involved in placing the crate there.
I believe the stm32l0/sx1276 and nrf/sx1262 support in the lora crate is on track to be more useful than that in embassy-lora (and I can speak authoritatively on nrf/sx1262 ... :) ). I took the approach to stm32wl that it has an sx1262, so it should be possible to use the newer sx126x implementation as base, referring to the Embassy subghz implementation for inspiration where the STM folks tried to subsume the sx1262 in their own machinations. I imagine there will be a few gear grindings in their machine to overcome, but the Embassy subghz implementation helps immensely. |
I think it would, as it would be a useful source for writing new drivers and reusing code between them.
I think projects that end up in rust-embedded tend to be crates that span the entire ecosystem, like the cortex-m crates for instance. Moving this repo there is going to be a hard sell I think, because of the required process for all WG repos. Another alternative could be the rust-embedded-community org, which is a bit like 'org for embedded crates in the community that don't belong to a particular project'. However, I would argue that this repo could live in the embassy-rs org, because it's focused on async, and embassy is a community of various embedded async projects.
Assuming we can get all the existing lora examples working, I think it makes sense to do that. Looking at https://github.com/ceekdee/embassy/blob/master/embassy-stm32/src/lora.rs - I think we should make sure dependency on lora crate is optional and only enabled via some feature flag or based on the 'subghz' feature.
I'd be happy to test out this driver on stm32wl. Is your embassy fork the best place to try it out? |
Feature flag external-lora has been added to make the lora crate optional. The lora crate is still needed for some of the examples for stm32wl, stm32l0, and nrf52840, but the lora crate is optional when building one's own application. The flag has not been extended to make the LoRa implementations in embassy-lora optional. Perhaps we will do that later, but it seems premature currently. |
Couple of comment reviewing this:
Wrt. the existing implementations, I think they can be removed when embassy-lora is migrated to use the lora crate instead of the builtin drivers. I wonder if the interface variants should be placed there, because it seems to become the integration point between lorawan-device, embassy and the lora crate. My only reservation is the current Subghz implementation for stm32wl, and if there are features in the stm32wl variant of sx126x that is missed by removing the current Subghz impl in embassy. Thoughts? |
The update: consolidates InterfaceVariant processing in embassy-lora, updating embedded-hal there to: embedded-hal-02 = { package = "embedded-hal", version = "0.2.6", features = ["unproven"] } for consistency with other Cargo.toml instances. This still keeps the original LoRa implementations independent of the implementation for the external lora crate - both features sets are still available. We can deprecate and then remove the original LoRa implementations once there is significant usage and testing of the new lora crate features. |
Following is a proposal to expand LoRa functionality using Embassy. I am sure there are considerations I am missing, but I believe there is enough substance here to warrant moving forward.
|
I'm not sure if you meant to perform the steps in the order you wrote them, but overall I agree with them. Some comments below.
This sounds like a good idea. I think @Dirbaio who owns the organization would need to agree and accept this. I'm also wondering if lora-radio is a more descriptive name of the crate?
I think it's fine to remove the embassy-lora implementations assuming the new ones work, we don't have any backwards compatibility guarantee for unreleased crates, and overall I think it would be less work to just remove it.
I think these last two should be done before the lora crate integration, so that we don't keep things in an intermediate step. I can help with review and releasing a new version of the rust-lorawan crate, so that it can be used in embassy. |
Issue resolved by PR #1396. |
This issue arises out of the discussion regarding the recent addition of an implementation to support the Semtech sx126x LORA chips:
#985 (comment)
I hope to start working on this project in December, or to contribute to other initiatives that start before then. In the meantime, this issue serves as a launchpad for a discussion on expectations developers have for a LORA physical layer implementation and API that supports Semtech LORA chips and MCU variants. It would be most helpful if envisioned use cases can be added here to shape the discussion.
My personal focus at this time is on a point-to-point (P2P) implementation that requires only the LORA physical layer. However, I see great value in interfacing the physical layer implementation with - for example - the Drogue IoT project.
The text was updated successfully, but these errors were encountered: