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

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

Closed
pdgilbert opened this issue Mar 26, 2021 · 25 comments
Closed

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

pdgilbert opened this issue Mar 26, 2021 · 25 comments

Comments

@pdgilbert
Copy link

(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
Copy link

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
Copy link
Author

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
Copy link
Member

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 [email protected], 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
Copy link

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
Copy link
Author

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
Copy link
Author

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

@ryankurte
Copy link
Member

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
Copy link

Cightline commented May 27, 2021

@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
Copy link
Member

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
Copy link

Cightline commented Jun 10, 2021

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
Copy link
Member

heads up i've published releases against [email protected] now, which also requires a bump to[email protected] if you're using that

@pdgilbert
Copy link
Author

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
Copy link
Member

ryankurte commented Oct 10, 2021

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 [email protected] as returning results is the standard for all embedded-hal traits now.

@pdgilbert
Copy link
Author

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
Copy link

Cightline commented Nov 30, 2021

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Author

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 stm32-rs/stm32f4xx-hal#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.

@pdgilbert
Copy link
Author

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
Copy link
Author

Possibly it's not ready for use but I tried branch [email protected] 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
Copy link
Member

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
Copy link
Author

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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants