rust-iot / rust-radio-sx127x

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

impl Transmit #12

Closed pdgilbert closed 5 months ago

pdgilbert commented 3 years ago

While learning Rust I have been working through some examples that I am trying to do in a way that they will work with several stm32*xx_hal's. (The examples using rust-radio-sx127x are the lora_spi_* ones in the second table in my scoreboard at https://pdgilbert.github.io/eg_stm_hal/ .)

Following a suggestion of https://github.com/TheZoq2 in issue https://github.com/stm32-rs/stm32f1xx-hal/issues/234#issuecomment-647327798 I am using setup() functions for hal specific code. These setup() functions return objects that can be used in application code that is common for all setups. One of the most difficult parts of this is determining the return value for the setup functions, which requires details the compiler looks after if the code is not separated into a function. For example, the signature for the lora_spi_send example needs

    fn setup() ->  Sx127x<Wrapper<Spi<SPI1, (PA5<Alternate<AF5>>,    PA6<Alternate<AF5>>,   PA7<Alternate<AF5>>)>,  Error, 
                   PA1<Output<PushPull>>,  PB8<Input<Floating>>,  PB9<Input<Floating>>,  PA0<Output<PushPull>>, 
                   core::convert::Infallible,  Delay>,  Error, core::convert::Infallible> {

(The complete example code is at https://github.com/pdgilbert/eg_stm_hal/blob/master/examples/lora_spi_send.rs )

When traits are available I think I should be able to replace the function signature with

 fn setup() ->  impl Transmit {

but when I do that I get errors about trait bounds were not satisfied:


error[E0599]: no method named `unwrap` found for enum `core::result::Result<(), <impl radio::Transmit as radio::Transmit>::Error>` in the current scope
   --> examples/lora_spi_send.rs:674:37
    |
674 |        lora.start_transmit(message).unwrap();    // should handle error
    |                                     ^^^^^^ method not found in `core::result::Result<(), <impl radio::Transmit as radio::Transmit>::Error>`
    |
    = note: the method `unwrap` exists but the following trait bounds were not satisfied:
            `<impl radio::Transmit as radio::Transmit>::Error: core::fmt::Debug`

error[E0599]: no method named `delay_ms` found for opaque type `impl radio::Transmit` in the current scope
   --> examples/lora_spi_send.rs:683:13
    |
683 |        lora.delay_ms(5000u32);
    |             ^^^^^^^^ method not found in `impl radio::Transmit`

error: aborting due to 2 previous errors; 2 warnings emitted

Am I doing something wrong?

ryankurte commented 3 years ago

When traits are available I think I should be able to replace the function signature with...

i think in this case you're probably just missing the Error bounds (something like fn setup() -> impl Transmit<Error=Error<Error, core::convert::Infallible>>) . i generally find it more useful to just define a type alias for the appropriate setup and use that elsewhere, and it is worth noting that as impl erases the returned type this can lead to issues when trying to compose things.

pdgilbert commented 3 years ago

(Please don't forget that I am still new to Rust.) I thought impl trait was supposed to make it easier, the compiler could figure out what the return type had to be. But I think the error bounds will be different for each hal and so figuring this out seems as hard as just filling in the type that the compiler tells me it is expecting. I don't understand how my defining a type or type alias would help since I am only using it in one place, as the return type for the setup function. (Very possibly I am missing something.)

But maybe I'm asking the wrong question. Is there a type or a type alias that I can use for the object returned by Sx127x::spi(...)?

pdgilbert commented 3 years ago

After some head banging over the "something like" part I see this is much better than I I first thought. It almost works with

  fn setup() ->  impl Transmit<Error=radio_sx127x::Error<stm32f4xx_hal::spi::Error, core::convert::Infallible>> {

but I get an error```

error[E0599]: no method named delay_ms found for opaque type impl radio::Transmit in the current scope --> examples/lora_spi_send.rs:689:13 | 689 | lora.delay_ms(5000u32); | ^^^^^^^^ method not found in impl radio::Transmit


If I comment out that line then it builds. So I'm back to needing an alternate delay.
pdgilbert commented 3 years ago

In fact, really good! Other than the delay issue I think (still untested) I can use the same fairly simple signature in 7 hal crates and only have to do something slightly different in the two that do not use core::convert::Infallible, for stm32h7xx_hal and stm32l0xx_hal I need:

 fn setup() ->  impl Transmit<Error=radio_sx127x::Error<Error, stm32h7xx_hal::Never>> {
 fn setup() ->  impl Transmit<Error=radio_sx127x::Error<Error, void::Void>> {

and this lets me get rid of a lot of unneeded hal specific use inports like:


warning: unused import: `embedded_spi::wrapper::Wrapper`
  --> examples/lora_spi_send.rs:49:5
   |
49 | use embedded_spi::wrapper::Wrapper;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

warning: unused imports: `Enabled`, `Floating`, `Input`, `Output`, `PA0`, `PA1`, `PB8`, `PB9`, `PushPull`, `Spi`, `pac::SPI1`
   --> examples/lora_spi_send.rs:411:27
    |
411 |                     spi::{Spi, Enabled, Error},
    |                           ^^^  ^^^^^^^
...
414 |                            gpioa::{PA0, PA1}, Output, PushPull,
    |                                    ^^^  ^^^   ^^^^^^  ^^^^^^^^
415 |                            gpiob::{PB8, PB9}, Input, Floating},
    |                                    ^^^  ^^^   ^^^^^  ^^^^^^^^
416 |                     pac::SPI1,
    |                     ^^^^^^^^^

Figuring out all the signatures and these imports has been one of the hardest part of making things work with multiple systems!

pdgilbert commented 3 years ago

Other than delay, things look good on the send side, but on the receive side I get an additional error:

error[E0308]: mismatched types
   --> examples/lora_spi_receive.rs:659:53
    |
297 |     fn setup() ->  impl Receive<Error=radio_sx127x::Error<Error, core::convert::Infallible>> {
    |                    ------------------------------------------------------------------------- the expected opaque type
...
659 |             Ok(v)  if v  =>  {n = lora.get_received(&mut info, &mut buff).unwrap();
    |                                                     ^^^^^^^^^ expected associated type, found struct `radio_sx127x::device::PacketInfo`
    |
    = note: expected mutable reference `&mut <impl radio::Receive as radio::Receive>::Info`
               found mutable reference `&mut radio_sx127x::device::PacketInfo`
    = note: consider constraining the associated type `<impl radio::Receive as radio::Receive>::Info` to `radio_sx127x::device::PacketInfo` or calling a method that returns `<impl radio::Receive as radio::Receive>::Info`
    = note: for more information, visit https://doc.rust-lang.org/book/ch19-03-advanced-traits.html
ryankurte commented 3 years ago

Figuring out all the signatures and these imports has been one of the hardest part of making things work with multiple systems!

this is particularly difficult with an extremely statically typed language like rust... good effort!

error[E0599]: no method named delay_ms found for opaque type impl radio::Transmit in the current scope

well, you're returning an impl Transmit, so the compiler only knows about the methods covered by that trait. you could compose a higher trait like here that covers all the traits you need.

I don't understand how my defining a type or type alias would help since I am only using it in one place, as the return type for the setup function. (Very possibly I am missing something.)

you can use impl like you are now, but you'll run into trouble when you try and store that anywhere you need to know the type. this is probably fine, but the alternative is to define the exact type required for each platform (pub type PlatformRadio = Sx127x<...>) then import that from your common-main / wherever. whatever works is good.

Other than delay, things look good on the send side, but on the receive side I get an additional error:

you're missing another associated type binding, see the docs (or in the definition of the Receive trait)

pdgilbert commented 3 years ago

I can resolve the access to delay by changing the signature to

use embedded_hal::blocking::delay::DelayMs;
fn setup() ->  impl DelayMs<u32> + Transmit<Error=sx127xError<Error, core::convert::Infallible>> {

so everything looks really good on the send side now (using 'master' not 'everything-update' yet). However, on the receive side I am having a lot of trouble finding the proper incantation forimpl with the associated type Info. With

fn setup() ->  impl DelayMs<u32> + Receive<Error=radio_sx127x::Error<Error, core::convert::Infallible>>  { 

I get the error

error[E0308]: mismatched types
   --> examples/lora_spi_receive.rs:664:53
    |
298 |     fn setup() ->  impl DelayMs<u32> + Receive<Error=radio_sx127x::Error<Error, core::convert::Infallible>>  { 
    |                    ---------------------------------------------------------------------------------------- the expected opaque type
...
664 |             Ok(v)  if v  =>  {n = lora.get_received(&mut info, &mut buff).unwrap();
    |                                                     ^^^^^^^^^ expected associated type, found struct `radio_sx127x::device::PacketInfo`
    |
    = note: expected mutable reference `&mut <impl embedded_hal::blocking::delay::DelayMs<u32>+radio::Receive as radio::Receive>::Info`
               found mutable reference `&mut radio_sx127x::device::PacketInfo`
    = note: consider constraining the associated type `<impl embedded_hal::blocking::delay::DelayMs<u32>+radio::Receive as radio::Receive>::Info` to `radio_sx127x::device::PacketInfo` or calling a method that returns `<impl embedded_hal::blocking::delay::DelayMs<u32>+radio::Receive as radio::Receive>::Info`
    = note: for more information, visit https://doc.rust-lang.org/book/ch19-03-advanced-traits.html

From the compiler message I think l probably need something like

fn setup() ->  impl DelayMs<u32> + Receive<Error=radio_sx127x::Error<Error, core::convert::Infallible>>  
                                     + Receive<Info=radio::Receive>::Info  { 

to pull in the associate type Info but I have tried many variations on this with no luck. Any suggestions would be much appreciated.

ryankurte commented 3 years ago

unchecked but it should be more like...

fn setup() ->  impl DelayMs<u32> + Receive<Info=radio_sx127x::ReceiveInfo, Error=radio_sx127x::Error<Error, core::convert::Infallible>> { 
pdgilbert commented 3 years ago

It looks like that syntax is about right, but it seems that ReceiveInfo is found in the git version of radio. I've been using release 0.7 for radio and when I change that other thinks break. Just to torture myself I tried 'update-everything' but could not discover where I should find the new trait DelayError and it looked like lots of other things would break too.

Unless you suggest otherwise then, rather than distract you with old stuff, I think I should put this aside until 'update-everything' is merged and the new arrangement is a bit more stable. The current status is that I have my examples lora_sp_send and lora_spi_gps working using impl traits for the return values of the setup() functions, and lora_spi_receive working using exact types for the return values of the setup()function. All of this is with current git master forradio_sx127x, 0.7 for radio, and embedded-hal "0.2.4".

ryankurte commented 3 years ago

oh right it should be radio_sx127x::device::PacketInfo, it's often useful to look at the docs (or cargo doc --open) for these things.

we're hopefully not far off the update-everything, just an alpha release of linux-embedded-hal. this version-coherence issue is one you'll come across a lot in rust, waiting is fair enough, but when required the best technique to mitigate this is via patch. you can see where i'm using this in the sx128x driver here for the same reason, though it is worth noting that only the top level is respected so you have to add this to your project file.

pdgilbert commented 3 years ago

Yes, I tried that, but this

   fn setup() ->  impl DelayMs<u32> + Receive<Info=radio_sx127x::device::PacketInfo::Info, 
                          Error=radio_sx127x::Error<Error, core::convert::Infallible>> { 

gives

use fully-qualified syntax: `<radio_sx127x::device::PacketInfo as Trait>::Info`

and this

   fn setup() ->  impl DelayMs<u32> + Receive<Info=<radio_sx127x::device::PacketInfo as radio::Receive>::Info, 
                          Error=radio_sx127x::Error<Error, core::convert::Infallible>> { 

gives

the trait `radio::Receive` is not implemented for `radio_sx127x::device::PacketInfo`

Thanks for the hints and I will work on it more when you update-everything. I may wait until then since it actually is building at the moment. Many of my other example seem to work with the alpha release of linux-embedded-hal so I will try to switch to that when you do.

pdgilbert commented 3 years ago

My setup() signature was

use radio_sx127x::Error as sx127xError;     
use stm32f4xx_hal::spi::{Spi, Error};

fn setup() ->  impl  DelayMs<u32> + Transmit<Error=sx127xError<Error, core::convert::Infallible>> {

After you merged update-everything my setup() signature needs a third type (for DelayError I think). The message is

error[E0107]: wrong number of type arguments: expected 3, found 2
   --> examples/lora_spi_send.rs:287:56
     fn setup() ->  impl  DelayMs<u32> + Transmit<Error=sx127xError<Error, core::convert::Infallible>> {
                                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected 3 type arguments

error[E0277]: the trait bound `stm32f4xx_hal::spi::Spi<stm32f4::stm32f411::SPI1, (stm32f4xx_hal::gpio::gpioa::PA5<stm32f4xx_hal::gpio::Alternate<stm32f4xx_hal::gpio::AF5>>, stm32f4xx_hal::gpio::gpioa::PA6<stm32f4xx_hal::gpio::Alternate<stm32f4xx_hal::gpio::AF5>>, stm32f4xx_hal::gpio::gpioa::PA7<stm32f4xx_hal::gpio::Alternate<stm32f4xx_hal::gpio::AF5>>)>: embedded_hal::blocking::spi::transfer::Default<u8>` is not satisfied
    --> examples/lora_spi_send.rs:317:19
...

I've tried several things with no luck specifying the type, and not a good idea where I should be looking. Hints would be appreciated.

Also, I have the impression that I should be able to do

use {CommsError, PinError, DelayError} from somewhere;
fn setup() ->  impl  DelayMs<u32> + Transmit<Error=sx127xError<CommsError, PinError, DelayError>> {

but I suppose that may be overly optimistic.

niondir commented 3 years ago

Is it correct, that I need

[dependencies.radio]
version = "0.7.0"

and

extern crate radio;
[...]
use radio::Transmit;

To call lora.start_transmit(message).unwrap();

Can't I get the trait without adding the radio dependency myself?

ryankurte commented 3 years ago

sortof yeah, you need at least thre use radio::Whatever, extern is a pre 2018 edition artifact that is only required in special cases now.

Can't I get the trait without adding the radio dependency myself?

not at the moment, because the radio traits not currently re-exported by the radio-sx127x crate, though there's no reason they couldn't be either added as a public export to the root or via a prelude.

pdgilbert commented 5 months ago

This issue is out-of-date and effectively resolved.