stm32-rs / stm32f1xx-hal

A Rust embedded-hal HAL impl for the STM32F1 family based on japarics stm32f103xx-hal
Apache License 2.0
569 stars 179 forks source link

gpioa.pa9.into_alternate_push_pull(&mut gpioa.crh); #234

Closed pdgilbert closed 4 years ago

pdgilbert commented 4 years ago

(newbie so I may be asking something stupid, but I would appreciate understand this.)

I am trying to understand why some arguments are needed, and why they need to be mutable. In stm32f4xx-hal I can do

let pin_rx1 = gpioa.pa9.into_alternate_af7();

but to accomplish a similar thing in stm32f1xx-hal I need

let pin_rx1 = gpioa.pa9.into_alternate_push_pull(&mut gpioa.crh);

The compiler seems to know what argument I need, if I make a mistake it tells me what the single choice must be, so it is easy to fix. But if there is only one possible argument then why is it needed? (I do realize that the argument might have different values sometimes, I guess I am really asking why it cannot be part of the gpioa.pa9 structure?) And the fact that this argument needs to be mutable cascades back. It means with stm32f4xx-hal I get a warning if I don't declare

let rcc = p.RCC.constrain();

whereas with stm32f1xx-hal I need

let mut rcc = p.RCC.constrain();

I have the same question about

let clocks = rcc.cfgr.freeze(&mut p.FLASH.constrain().acr);

for stm32f1xx-hal while for stm32f4xx-hal I do

let clocks = rcc.cfgr.freeze();

The result of these hal differences is that my code has a lot more cfg() statements then I think I should need. A complete working example is below. (With stm32f3xx it builds but prints gjibberish on my hardware. With stm32l1xx it does not build.) I'm using git versions of the hals.

//! Echo console input back to console + semihost output, char by char
//!
//! Connect the Tx pin pa9  to the Rx pin of usb-ttl converter
//! Connect the Rx pin pa10 to the Tx pin of usb-ttl converter
//! Set up the serial console (e.g. minicom) with the same settings used here.
//! (Using 9600bps, could be higher but needs serial console to be the same.)

#![deny(unsafe_code)]
#![no_main]
#![no_std]

#[cfg(debug_assertions)]
extern crate panic_semihosting;

#[cfg(not(debug_assertions))]
extern crate panic_halt;

use cortex_m_rt::entry;
//use core::fmt::Write;  // for writeln
use cortex_m_semihosting::hprintln;
use core::str::from_utf8;
use nb::block;

#[cfg(feature = "stm32f1xx")]  //  eg blue pill stm32f103
use stm32f1xx_hal::{prelude::*,   pac::Peripherals, serial::{Config, Serial }, };   //StopBits}

#[cfg(feature = "stm32f3xx")]  //  eg Discovery-stm32f303
use stm32f3xx_hal::{prelude::*, stm32::Peripherals, serial::{ Serial}, };

#[cfg(feature = "stm32f4xx")] // eg Nucleo-64  stm32f411
use stm32f4xx_hal::{prelude::*,  pac::Peripherals, serial::{config::Config, Serial }};

#[cfg(feature = "stm32l1xx") ] // eg  Discovery kit stm32l100 and Heltec lora_node STM32L151CCU6
use stm32l1xx_hal::{prelude::*, stm32::Peripherals, serial::{Config, Serial }};

#[entry]
fn main() -> ! {
    let p = Peripherals::take().unwrap();

    #[cfg(feature = "stm32f1xx")]
    let mut rcc = p.RCC.constrain();
    #[cfg(feature = "stm32f3xx")]
    let mut rcc = p.RCC.constrain();
    #[cfg(feature = "stm32f4xx")]
    let rcc = p.RCC.constrain();
    #[cfg(feature = "stm32l1xx")]
    let rcc = p.RCC.constrain();

    #[cfg(feature = "stm32f1xx")]
    let clocks = rcc.cfgr.freeze(&mut p.FLASH.constrain().acr); //why can I do &mut p...  when p is not mutable?
    #[cfg(feature = "stm32f3xx")]
    let clocks = rcc.cfgr.freeze(&mut p.FLASH.constrain().acr);
    #[cfg(feature = "stm32f4xx")]
    let clocks = rcc.cfgr.freeze();
    #[cfg(feature = "stm32l1xx")]
    let clocks = rcc.cfgr.freeze();

    #[cfg(feature = "stm32f1xx")]
    let mut gpioa = p.GPIOA.split(&mut rcc.apb2);   // why an argument and why mutable?
    #[cfg(feature = "stm32f3xx")]
    let mut gpioa = p.GPIOA.split(&mut rcc.ahb); 
    #[cfg(feature = "stm32f4xx")]
    let gpioa = p.GPIOA.split();
    #[cfg(feature = "stm32l1xx")]
    let gpioa = p.GPIOA.split();

    #[cfg(feature = "stm32f1xx")]
    let pin_rx1 = gpioa.pa9.into_alternate_push_pull(&mut gpioa.crh);     //pa9
    #[cfg(feature = "stm32f3xx")]
    let pin_rx1 = gpioa.pa9.into_af7(&mut gpioa.moder, &mut gpioa.afrh);  //pa9
    #[cfg(feature = "stm32f4xx")]
    let pin_rx1 = gpioa.pa9.into_alternate_af7();                         //pa9
    #[cfg(feature = "stm32l1xx")]
    let pin_rx1 = gpioa.pa9.into_alternate_af7();                         //pa9

    #[cfg(feature = "stm32f1xx")]
    let pin_tx1 = gpioa.pa10;                                             //pa10
    #[cfg(feature = "stm32f3xx")]
    let pin_tx1 = gpioa.pa10.into_af7(&mut gpioa.moder, &mut gpioa.afrh); //pa10
    #[cfg(feature = "stm32f4xx")]
    let pin_tx1 = gpioa.pa10.into_alternate_af7();                        //pa10
    #[cfg(feature = "stm32l1xx")]
    let pin_tx1 = gpioa.pa10.into_alternate_af7();                        //pa10

    #[cfg(feature = "stm32f1xx")]
    let cnfg = Config::default() .baudrate(9600.bps());  //.stopbits(StopBits::STOP1),
    #[cfg(feature = "stm32f3xx")]
    let cnfg = 9600.bps();
    #[cfg(feature = "stm32f4xx")]
    let cnfg = Config::default() .baudrate(9600.bps());
    #[cfg(feature = "stm32l1xx")]
    let cnfg = Config::default() .baudrate(9600.bps());

    #[cfg(feature = "stm32f4xx")]
    p.USART1.cr1.modify(|_,w| w.rxneie().set_bit());  //need RX interrupt? 
    #[cfg(feature = "stm32l1xx")]
    p.USART1.cr1.modify(|_,w| w.rxneie().set_bit());  //need RX interrupt? 

    let txrx1 = Serial::usart1(
        p.USART1,
        (pin_rx1, pin_tx1),
        #[cfg(feature = "stm32f1xx")]
        &mut p.AFIO.constrain(&mut rcc.apb2).mapr,
        cnfg,
        clocks,
        #[cfg(any(feature = "stm32f1xx", feature = "stm32f3xx"))]
        &mut rcc.apb2,
    );

    #[cfg(any(feature = "stm32f4xx", feature = "stm32l1xx"))]
    let txrx1 = txrx1.unwrap();

    // end hal specific conditional setup

    // Split the serial txrx1 struct into a receiving and a transmitting part
    let (mut tx1, mut rx1) =txrx1.split();

    hprintln!("testwrite to console ...").unwrap();

    for byte in b"\r\nconsole connect check.\r\n" { block!(tx1.write(*byte)).ok(); }

    hprintln!("test read and write by char. Please type into the console ...").unwrap();
    //writeln!(tx1, "\r\nPlease type (slowly) into the console below:\r\n").unwrap();
    for byte in b"\r\nType (slowly) below:\r\n" { block!(tx1.write(*byte)).ok(); }

    loop { // Read a byte and write
       let received = block!(rx1.read()).unwrap();
       block!(tx1.write(received)).ok();
       hprintln!("{}", from_utf8(&[received]).unwrap()).unwrap();
     }
}
TheZoq2 commented 4 years ago

This was discussed briefly in the embedded matrix chat yesterday. The reason, as I understand it is that the f4 HAL isn't as strict in terms of preventing race conditions as the f1 crate. See https://github.com/stm32-rs/stm32f4xx-hal/issues/171 for more details.

By requiring a &mut to the registers, we ensure that there are no race conditions when writing to the registers during setup. The f4 HAL does not have this check, if I understood correctly.

Regarding the clocks, my guess is that it is a hardware difference. The f1 requires something in the flash peripheral to freeze the clocks, while the f4 does not. It is also possible that that is the cause of the other differences as well, for example, there may be hardware locking present in the f4, but not the f1.

I think @therealprof who is familiar with both crates can give a less "guessy" answe

Also as I hinted in your previous issue, I'd probably consider trying to use the same code for two HALs a bit of an antipattern. The HALs should only really be used for setting up the peripherals, and after that, most of the heavy lifting should be done by the embedded-hal traits which should be implemented the same way for all HALs. So generally, you'd want to write most of your code genericallly using EH, and then pass some intialised periphrals to that code.

burrbull commented 4 years ago

The original design of this API was created by japaric: http://blog.japaric.io/brave-new-io/

pdgilbert commented 4 years ago

Also as I hinted in your previous issue, I'd probably consider trying to use the same code for two HALs a bit of an antipattern. The HALs should only really be used for setting up the peripherals, and after that, most of the heavy lifting should be done by the embedded-hal traits which should be implemented the same way for all HALs. So generally, you'd want to write most of your code genericallly using EH, and then pass some intialised periphrals to that code.

I'm new and a bit slow, probably need more than hints. I'm not sure I understand this. I thought by using, for example,

use stm32f1xx_hal::{prelude::*, serial::{Config, Serial }, };

I am getting stm32f1xx implementation of embedded-hal traits. So, my example is what I understood I have to do to pass the initialized peripherals to my code. Do you mean I should instead be doing something like

use embedded_hal::{prelude::*, serial::{Config, Serial }} ;

and working with that? Or do you mean something different by "most of the heavy lifting should be done by the embedded-hal traits"?

pdgilbert commented 4 years ago

The original design of this API was created by japaric: http://blog.japaric.io/brave-new-io/

I think that was the post that initially got me interested in Rust. Just re-reading after a few months, I understand a bit more, but will need to re-read again in a few more months. I think I've swallowed the abstraction but I'm still chewing on the details.

thalesfragoso commented 4 years ago

Also as I hinted in your previous issue, I'd probably consider trying to use the same code for two HALs a bit of an antipattern. The HALs should only really be used for setting up the peripherals, and after that, most of the heavy lifting should be done by the embedded-hal traits which should be implemented the same way for all HALs. So generally, you'd want to write most of your code genericallly using EH, and then pass some intialised periphrals to that code.

I'm new and a bit slow, probably need more than hints. I'm not sure I understand this. I thought by using, for example,

use stm32f1xx_hal::{prelude::*, serial::{Config, Serial }, };

I am getting stm32f1xx implementation of embedded-hal traits. So, my example is what I understood I have to do to pass the initialized peripherals to my code. Do you mean I should instead be doing something like

use embedded_hal::{prelude::*, serial::{Config, Serial }} ;

and working with that? Or do you mean something different by "most of the heavy lifting should be done by the embedded-hal traits"?

The HALs aren't expected to be completely interchangeable, because of hardware differences between families, etc. I think what they meant is that to be fully compatible, your API must depend only on the embedded-hal traits, it's somewhat expected that you might need to change the initialization code if you want to change chip families.

And about the f4xx-hal, both of the cases that you mentioned are unsound right now, we will probably fix them soon.

TheZoq2 commented 4 years ago

As thalesfragoso says, the init stuff in the HAL isn't supposed to be interchangeable, only the stuff behind the traits is. In your case, you may be able to do something like this:

use embedded_hal::serial;

fn main() {
    #[cfg(feature = "stm32f1xx")]
    let serial = setup_f1();
    #[cfg(feature = "stm32f4xx")]
    let serial = setup_f4();

    main_loop(serial);
}

#[cfg(feature = "stm32f1xx")]
fn setup_f1() -> stm32f1xx_hal::serial::Serial {
    // Do the setup you're already doing
}
#[cfg(feature = "stm32f4xx")]
fn setup_f4() -> stm32f4xx_hal::serial::Serial {
    // Do the setup you're already doing
}

// Main loop which uses the embedded_hal::serial::Write traits exclusively. This makes it
// independent of the HAL implementation
fn main_loop<S>(serial: S) where S: serial::Write {
    hprintln!("testwrite to console ...").unwrap();

    for byte in b"\r\nconsole connect check.\r\n" { block!(serial.write(*byte)).ok(); }

    hprintln!("test read and write by char. Please type into the console ...").unwrap();
    //writeln!(tx1, "\r\nPlease type (slowly) into the console below:\r\n").unwrap();
    for byte in b"\r\nType (slowly) below:\r\n" { block!(serial.write(*byte)).ok(); }

    loop { // Read a byte and write
       let received = block!(serial.read()).unwrap();
       block!(serial.write(received)).ok();
       hprintln!("{}", from_utf8(&[received]).unwrap()).unwrap();
     }
}

Though the two parallel setup functions would get unwieldy as the amount of things to initialise and return grows

pdgilbert commented 4 years ago

Thanks everyone for comments and explanations, and your patients. In TheZorq2's sketch I certainly see the logic of including hardware specific details in the setup functions and out of the scope of the main program. But I'm still struggling a bit with "behind the traits". Looking at the sketch, if I understand correctly, the serial object presents the traits interface. It deals with everything hardware specific and presents just the traits to my application code. But I understood that stm32f1xx_hal provides an implementation of embedded_hal::serial that is specific to the stm32f1xx hardware, so I don't see what

use embedded_hal::serial;

does, or why it is needed in addition too, or instead of, using serial from stm32f1xx_hal.

I also see the sketch uses serial.write and serial.read rather than splitting serial into rx and tx as is done in many examples. Is there a reason that would be preferable to splitting?

I have tried filling the details into the sketch. My setup function has signature

fn serial1_setup() -> stm32f1xx_hal::serial::Serial {

and returns the structure from

stm32f1xx_hal::serial::Serial::usart1(
    p.USART1,
    (pin_rx1, pin_tx1),
    &mut p.AFIO.constrain(&mut rcc.apb2).mapr,
    cnfg,
    clocks,
    &mut rcc.apb2,
    )

but compiling gives

fn serial1_setup() -> stm32f1xx_hal::serial::Serial {
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected 2 type arguments

and I'm stuck. (I see stm32f1xx_hal::serial::Serial::usart1 returns self, which seems to be a more complicated structure than just stm32f1xx_hal::serial::Serial.) I've tried several things without luck.

Finally, in the example in my original post I had

let p = Peripherals::take().unwrap();
...
let clocks = rcc.cfgr.freeze(&mut p.FLASH.constrain().acr);

which works but I don't understand why I can use &mut p... when p has not been declared mutable. The compiler will not let me do that in other cases.

I very much appreciate everyone's comments as I'm trying to put together some examples that not only work but also present reasonably good style, a bit of a challenge for a newbie.

burrbull commented 4 years ago

The sense of embedded-hal traits is to have hardware independent interface for external libraries like drivers for peripheral devices, connection to PC, etc. You can write driver using only these traits and be sure it will be work with any microcontroller that implements these traits. No matter stm32, riscv, esp32 or raspberry pi or something else.

But we can not write such common interface for configuring of controllers because of very big differences in low level. Some of differences you find are easy to be fixed, but other changes can break safety of access to registers. Another problem is small and splitted community. We want to have more homogeneous code, but it is hard to work with device you don't have in your hands or are not familiar with. You can help us if you'll find more such differences.

Splitting serial on tx(transmit) and rx(receive) is very useful, as they can work independent, and you can use they in different tasks.

And the most important thing. No more "sketches". Forget this word.

TheZoq2 commented 4 years ago

I also see the sketch uses serial.write and serial.read rather than splitting serial into rx and tx as is done in many examples. Is there a reason that would be preferable to splitting?

That's my fault, just take my code as pseudocode/proof of concept :)

But I understood that stm32f1xx_hal provides an implementation of embedded_hal::serial that is specific to the stm32f1xx hardware, so I don't see what use embedded_hal::serial; does, or why it is needed in addition too, or instead of, using serial from stm32f1xx_hal.

If you only want to use the f1 crate, you can just use the structs defined here and everything will be fine. However, if you want to use multiple HALs, embedded-hal provides a common interface to them.

By writing your code to be generic over the e-h traits, it becomes implementation independent.

I'll also close this since there isn't really anything with the code to fix. Feel free to continue the discussion though :)

pdgilbert commented 4 years ago

I was about to close this myself since the only issue seems to be my incomplete understanding, and comments have drifted a long way from the issue title. There does seem to be a jump to get from pseudocode to a working example. I think there may be an issue around having some more complete examples that work across multiple HALs, but I need to do more playing before I understand this well enough. Thanks everyone, and I will try to remember to use the term pseudocode rather than sketch next time:)