search

rust-iot / rust-radio-sx127x

Rust driver for the Semtech SX127x series of Sub-GHz LoRa/ISM radio transceivers
Mozilla Public License 2.0
33 stars 17 forks source link

dependencies on version of radio confuse embedded-hal-compat #38

Closed pdgilbert closed 7 months ago

pdgilbert commented 3 years ago

(Mentioned in #34, but unclear about details.) rust-radio-sx127x dependencies on the version of radio confuse embedded-hal-compat

Version 0.10.1 of radio-sx127x on crates.io uses radio "0.4.0" . This does not work with embedded-hal-compat. The builds fail with wrong number of arguments, with or without

[patch.crates-io]
radio = { git = "https://github.com/ryankurte/rust-radio.git", branch = "master" }

Version 0.10.1 of radio-sx127x on https://github.com/rust-iot/rust-radio-sx127x uses radio "0.7.0" and my fork with the PR adding examples uses radio "0.8.1" . A crate using these needs to use the same version:

radio-sx127x  = {  git = "https://github.com/rust-iot/rust-radio-sx127x", default-features = false }
radio         = { version = "0.7.0" }

or

radio-sx127x  = {  git = "https://github.com/pdgilbert/rust-radio-sx127x", default-features = false }
radio         = { version = "0.8.1" }

The patch above has no affect on build for either (but I have not tested running). Is it possible the patch is no longer needed for anything?

Some consolidation would be good.

Cightline commented 3 years ago

Oh my god thank you. I've been using your examples and I had radio set to 0.8.1 instead of 0.7.0 so compilation kept failing because the radio::Transmit trait wasn't implemented.

pdgilbert commented 3 years ago

Yes, the version interactions are messy. Something to look forward to when everything stabilizes a bit. I've looked briefly at radio 0.9.0 but without success. Hoping @ryankurte will comment when there is a new set of pieces that should work together.

ryankurte commented 3 years ago

ahh, yeah i left a patch in there which made everything more tricky than it needed to be, sorry!

removed the patch and release 0.11.0 now uses radio@0.9.0, hopefully this is more straightforward. @pdgilbert you may like to rebase your PR against this (there shouldn't be any major changes i hope)

Cightline commented 3 years ago

ahh, yeah i left a patch in there which made everything more tricky than it needed to be, sorry!

It's all good. The library code seems to be written well, I had my 2 feather boards blinking at each other earlier which was neat.

pdgilbert commented 3 years ago

Ok, in a small test it builds with

embedded-hal-compat = "0.1.3" 
radio-sx127x  = { version = "0.11.0", default-features = false }
radio         = { version = "0.9.0" }

and no patch. I'll try updating my PR.

pdgilbert commented 3 years ago

@Cightline, what MCU do your feather boards have and which device HAL are you using? And does it have an RFM95 radio?

ryankurte commented 3 years ago

It's all good. The library code seems to be written well, I had my 2 feather boards blinking at each other earlier which was neat.

there's something about things blinking over an RF link right?! glad it's mostly working for y'all.

Cightline commented 3 years ago

@Cightline, what MCU do your feather boards have and which device HAL are you using? And does it have an RFM95 radio?

@pdgilbert: I have 2x of these. I'm using the feather_m0 crate. The ATSAMD Rust guys do some good work btw. Here's the boards they support.

there's something about things blinking over an RF link right?! glad it's mostly working for y'all.

@ryankurte: Oh yeah for sure. Hey I noticed you're subscribed to the rctestflight channel on YouTube. The whole reason why I bought these boards is so I can have reliable RC telemetry. ExpressLRS is doing some pretty impressive stuff with LoRa boards, figured I'd make a "safe" version.

ryankurte commented 3 years ago

Hey I noticed you're subscribed to the rctestflight channel on YouTube. The whole reason why I bought these boards is so I can have reliable RC telemetry

oh neat! way back i used to build / fly weird thrust-vectoring quadrotors, telemetry was always a bit of a trick huh.

Cightline commented 3 years ago

Yep. The following is getting off topic so let me know if I need to post this somewhere else.

I have a rough-draft on Gitlab if you wanna see what I have so far. The code runs off interrupts. After starting up both of the Feather M0 LoRa boards they will oscillate between sending a beacon and listening for a beacon. Once the beacon is received they immediately start "talking" as fast as possible (which I am visualizing with the on-board LEDs). These boards have amazing range for their size.

ryankurte commented 3 years ago

heads up i've published releases against embedded-hal@1.0.0-alpha.5 now, which also requires a bump toembedded-hal-compat@0.4.0 if you're using that

pdgilbert commented 3 years ago

After figuring out I now need use embedded_hal_compat::ForwardCompat rather than IntoCompat and append .forward rather than .compat I have my code working again now using 

embedded-hal = { version = "1.0.0-alpha.5" }  
old-e-h      = { version = "0.2.6", package = "embedded-hal" }
embedded-hal-compat = "0.4.0" 
radio         = { version = "0.9.1" }
driver-pal   = { git = "https://github.com/ryankurte/rust-driver-pal/", default-features = false } # 0.8.0-alpha5
...

I am a bit puzzled that I had to change .try_delay_ms back to .delay_ms so I tried briefly with ReverseCompat and .reverse() but that seemed to make things worse. I don't understand the contexts when one should consider ReverseCompat or ForwardCompat.

ryankurte commented 3 years ago

After figuring out I now need use embedded_hal_compat::ForwardCompat rather than IntoCompat and append .forward rather than .compat I have my code working again now using

ahh yes, initially e-h-c only supported one-way compatibility, but, i ran into a need for both. forward() wraps an 0.2.x implementation in an 1.0.0-alpha.N compatible wrapper, allowing one to for example, use a 0.2.x HAL with a 1.0.0-alpha.N driver, and reverse() does the opposite.

I am a bit puzzled that I had to change .try_delay_ms back to .delay_ms

try prefixes were dropped in e-h@1.0.0-alpha.5 as returning results is the standard for all embedded-hal traits now.

pdgilbert commented 2 years ago

I'm having trouble again with getting the proper mix of versions for radio, rust-radio-sx127x, and embedded-hal-compat. I think the problem is the version mix, but the errors I'm getting are like

`error[E0432]: unresolved import `embedded_hal::delay::blocking::DelayMs`
 --> /home/paul/.cargo/registry/src/github.com-1ecc6299db9ec823/driver-pal-0.8.0-alpha.5/src/wrapper.rs:5:37
  |
5 | use embedded_hal::delay::blocking::{DelayMs, DelayUs};
  |                                     ^^^^^^^
  |                                     |
  |                                     no `DelayMs` in `delay::blocking`
  |                                     help: a similar name exists in the module: `DelayUs`

error[E0405]: cannot find trait `DelayMs` in module `embedded_hal::delay::blocking`
  --> /home/paul/.cargo/registry/src/github.com-1ecc6299db9ec823/driver-pal-0.8.0-alpha.5/src/lib.rs:59:38
   |
59 |     + embedded_hal::delay::blocking::DelayMs<u32>
   |                                      ^^^^^^^ help: a trait with a similar name exists: `DelayUs`
   |
  ::: /home/paul/.cargo/registry/src/github.com-1ecc6299db9ec823/embedded-hal-1.0.0-alpha.6/src/delay.rs:14:5
   |
14 |     pub trait DelayUs {
   |     ----------------- similarly named trait `DelayUs` defined here

error[E0405]: cannot find trait `DelayMs` in module `embedded_hal::delay::blocking`
...
Cightline commented 2 years ago

I was messing around with this the other day. The following builds successfully on my system. radio-sx127x is the latest. Pretty sure I just copied a few of these from your examples. 

[dependencies]
cortex-m            = "0.7.3"
feather_m0          = { version = "0.10.1", features = ["default", "rtic", "rt", "unproven", "rfm", "dma"] }
panic-halt          = "0.2.0"
heapless            = { verison = "0.7.7", features = ["default"], version = "0.7.7" }
panic-semihosting   = "0.5.6"
cortex-m-rtic       = { version = "0.6.0-rc.2", features = [] }
embedded-hal        = { version = "1.0.0-alpha.5" }
old-e-h             = { version = "0.2.6", package = "embedded-hal" }
radio               = { version = "0.9.1" }
driver-pal          = { version = "0.8.0-alpha.5", default_features = false }
radio-sx127x        = { path = "../../rust-radio-sx127x", default-features = false }
shared-bus-rtic     = { version = "0.2.2", features = ["thumbv6"] }
embedded-hal-compat = "0.4.0"
pdgilbert commented 2 years ago

I finally found my problem. In my Cargo.toml I need to specify 

embedded-hal = { version = "1.0.0-alpha.5,<1.0.0-alpha.6" }

otherwise cargo update is updating embedded-hal to alpha.6 and that does not work with radio v0.9.1 and radio-sx127x v0.13.0 from github and embedded-hal-compat v0.4.0. For anyone looking, the CI for my code is at https://github.com/pdgilbert/LoRaGPS-rust/actions and jobs show the cargo tree output.

I'm not sure if there is yet a set of versions that work with embedded-hal v1.0.0-alpha.6. Any update @ryankurte ?

ryankurte commented 2 years ago

ahh, unfortunately this is likely to occur any time embedded-hal changes, possibly we should be using strict version matching in embedded-hal-compat to make this more obvious.

I'm not sure if there is yet a set of versions that work with embedded-hal v1.0.0-alpha.6. Any update @ryankurte ?

both this library and embedded-hal-compat will need an update to embedded-hal v1.0.0-alpha.6, very happy to accept PRs if you have the time otherwise it's on my list (but i have uhhh a few crates to update yet) ^_^

pdgilbert commented 2 years ago

More obvious is always helpful for me. I have my hands full with other things at the moment, so I'll leave it on your list. There is no special rush for me but I would appreciate a heads up here when you do get around to it.

pdgilbert commented 2 years ago

I hope you might soon get to the update for embedded-hal v1.0.0-alpha.6? I just took a look but more recent commits seem to be for linux-embedded-hal and log I think may break no-std. So if I touch it I am afraid I would break more than I fix. The stm32f4xx-hal is making some steps toward eh-v1 (see https://github.com/stm32-rs/stm32f4xx-hal/pull/388) but I seem to need to be using alpha6 to play with it. That means I've had to disable testing of rust-radio-sx127x to check other example. A bit unfortunate because I thought rust-radio-sx127x would be the easy part.

ryankurte commented 2 years ago

yep, currently blocked on:

pdgilbert commented 2 years ago

Maybe skip alpha.6 and go to alpha.7? I'm not so worried about compat but it would be nice to have a driver crate using eh-1.0.0-alpha.7 to test in stm32f4xx_hal which is now supporting it.

pdgilbert commented 2 years ago

Possibly it's not ready for use but I tried branch e-h@1.0.0-alph.7 and I'm stuck on

error[E0277]: the trait bound `Spi<stm32f4xx_hal::pac::SPI1, (stm32f4xx_hal::gpio::Pin<Alternate<PushPull, 5_u8>, 'A', 5_u8>, stm32f4xx_hal::gpio::Pin<Alternate<PushPull, 5_u8>, 'A', 6_u8>, stm32f4xx_hal::gpio::Pin<Alternate<PushPull, 5_u8>, 'A', 7_u8>), TransferModeNormal>: embedded_hal::spi::blocking::Transfer` is not satisfied
   --> src/lora_spi_gps_usart.rs:513:16
    |
513 |     let lora = Sx127x::spi(
    |                ^^^^^^^^^^^ the trait `embedded_hal::spi::blocking::Transfer` is not implemented for `Spi<stm32f4xx_hal::pac::SPI1, (stm32f4xx_hal::gpio::Pin<Alternate<PushPull, 5_u8>, 'A', 5_u8>, stm32f4xx_hal::gpio::Pin<Alternate<PushPull, 5_u8>, 'A', 6_u8>, stm32f4xx_hal::gpio::Pin<Alternate<PushPull, 5_u8>, 'A', 7_u8>), TransferModeNormal>`
    |
note: required by `radio_sx127x::Sx127x::<driver_pal::wrapper::Wrapper<Spi, CsPin, BusyPin, ReadyPin, ResetPin, Delay>, SpiError, PinError, DelayError>::spi`
   --> /home/paul/.cargo/git/checkouts/rust-radio-sx127x-d16bc9c4082b08d0/eb4197a/src/lib.rs:133:5
    |
133 | /     pub fn spi(
134 | |         spi: Spi,
135 | |         cs: CsPin,
136 | |         busy: BusyPin,
...   |
140 | |         config: &Config,
141 | |     ) -> Result<Self, Error<SpiError, PinError, DelayError>> {
    | |____________________________________________________________^

For more information about this error, try `rustc --explain E0277`.
warning: `rust-integration-testing-of-examples` (lib) generated 1 warning
error: could not compile `rust-integration-testing-of-examples` due to previous error; 1 warning emitted

I'm not sure if this is a problem in my code or in radio-sx12x or in stm32f4xx_hal. Any ideas?

ryankurte commented 2 years ago

i appreciate your ongoing effort! next e-h release we've got a bunch of SPI changes which should reduce / remove the need for some of this so, once we've done that i'll go through and simplify all the dependencies as best i can.

if it'd help / you're willing to share the repo i can have a look at that to resolve your dependency woes for now,.

pdgilbert commented 2 years ago

Just to be clear, this is not about the compat layer anymore. I'd like a version of radio-sx127x that works with e-h-1.0.0-alpha as being used in stm32f4xx_hal. At the moment it is eh-1.0.0-alpha.7 but if it makes sense to wait for a new version then I'm ok with that. The Transfer trait problem mentioned just above is in branch eh-1-alpha of https://github.com/pdgilbert/rust-integration-testing. The problem happens in src/lora_spi_gps_usart.rs which compiles before examples are attempted, so it has been commented out in order to run other examples. I just re-enabled it so you can see the CI at https://github.com/pdgilbert/rust-integration-testing/actions/runs/1896521664. The lora jobs are near the bottom but everything is failing on the setup in the src/ file. A couple of the steps in the jobs show the tree, in case that is helpful. Let me know if there is anything else you need.

pdgilbert commented 7 months ago

This is all now resolved by switch to embedded-hal-1.0.0 and no longer using embedded-hal-compat (at least for stm32f4xx_hal , stm32g4xx_hal and stm32h7xx_hal so far).