Some comments

[japaric]

Hey @etrombly. You asked for some feedback. Here are my thoughts:

• First of all, it's really nice that you have taken to time to extend the original SVD to make the API safer. Great work!

• I think that HAL traits should never include references to device specific register blocks otherwise they won't be able to be device agnostic and usable across different device families / vendors. This pretty much means that init doesn't belong in a trait as it's highly device dependent. Initialization would be the reponsability of the application writer. The rest of the application logic can use the HAL traits.

• I can't comment on the clock code as I haven't run into the need of increasing the clock speed given that the scheduler is so fast and efficient. However, I do think that all API that deals with time should work with ticks rather than with units of time or frequency; that way their implementations can be device agnostic. For example, in your code you require a reference to &Rcc to get the system clock frequencies; at some point that &Rcc will leak into the signature of some API making less portable across devices. With ticks, the responsability of mapping seconds / Hz to ticks is up to the application which knows exactly what the clock frequencies are and thus can do the conversions (effetively) at compile time and without involving reads to registers.

• The reason why I use a newtype around e.g. Tim6 instead of using a trait to add methods to Tim6 is that I hope that eventually we'll settle on some HAL traits and those would end up being in a crate that most crates in the ecosystem would depend on. In that case you would need the newtype because you can't implement a trait that you didn't defined for a type you didn't define so I'm getting use to the idea early on.

• You can use the fact that Tim{2,3,4,5} all deref to tim2::RegisterBlock to reduce duplicated code. You could probably write something like this impl Timer for T where T: Deref<tim2::RegisterBlock> { .. } once and the implementation would work for all the timers.

[etrombly]

Thanks, this is really helpful. I've started making some of the changes you recommended. I had two main questions for now though. I'm going to be writing the GPIO/PIN hal stuff next, and wanted to have a pwm/analog output, but to configure the duty cycle you need to set several of the timer registers. Since this is device dependent will an analog output not be compatible with the HAL you were envisioning? I can still implement at the board level, but would like everything to be consistent with what other people are doing.

The second question has to do with what level certain things should be defined at. Most of the work I'm doing for the bluepill board is applicable to any STM32F103 chip, the only board specific things are the memory.x file and the leds. Should this stuff be going in to the stm32f103xx crate instead? Also if that's the case I think it would be nice to be able to mask hardware for certain boards. That way the stm32f103xx crate can have everything defined (for example timer 8) but if the STMf103c8t6 chip in the bluepill board doesn't have that timer, I can mask it. It's not necessary, but would be a nice feature to have.

[japaric]

Since this is device dependent will an analog output not be compatible with the HAL you were envisioning?

In my HAL model only methods need to be free of device dependent stuff so PWM is doable. Something like this:

pub trait Pwm {
    type Ticks;  // PWM resultion e.g. u8 or u16

    fn set_period(&self, ticks: Self::Ticks);
    fn set_duty(&self, ticks: Self::Ticks);
}

And the implementer could look like this:

// 4 PWM channels per timer
pub enum Channel<'a> {
    One(&'a Tim2),
    Two(&'a Tim2),
    Three(&'a Tim2),
    Four(&'a Tim2),
}

// Alternatively you could have 4 different structs

Note that the API doesn't care about which channel maps to which pin. That's the concern of the configuration / initialization layer. So the configuration part of your application code could look like this:

// Assuming we are using structs for the channels rather than one enum

pub type PA0<'a> = ChannelOne<'a>;
pub type PA1<'a> = ChannelTwo<'a>;

fn init(..) {
    // Configure PA0 and PA1 to work as TIM2 PWM pins
}

Should this stuff be going in to the stm32f103xx crate instead?

No, I don't think it should be there. Otherwise that crate would have to know about every framework / API out there. In my mind, you would have a stm32f103xx-hal crate on top of the stm32f103xx device crate that provides the HAL implementation and that's why you would need newtypes in that implementation crate.

Also if that's the case I think it would be nice to be able to mask hardware for certain boards.

Yeah, I don't have a great solution for that. You could rely on the RTFM implementation to forbid use of some peripherals. For instance if you declare this:

#[export_name = "TIM8"]
static UNUSED: () = ();

in the bluepill crate then you won't be able to use TIM8 in the peripherals! macro in applications that depend on the bluepill crate.

[nikhilkalige]

@etrombly I have been trying to find ways to write hal driver for 48 channel PWM driver. I don't like the fact that you would have to recreate the structure every time if you need it, as you have done for Stepper(pin1, pin2...). I would be interested in knowing if there are any alternatives to this approach.

I would be interested in knowing if @japaric has any thoughts about the same...

[etrombly]

You should probably look in to https://github.com/japaric/embedded-hal I've stopped working on my hal since japaric has started his own. With the recent changes he's made to rtfm you should be able to store the pwm state as a resource, which should make the driver easier to work with.

[nikhilkalige]

I am just a beginner in rust.. Is there a reason behind why references of peripherals are not stored, rather than passed in every time.

For example for my 48 channel led PWM driver, it would need spi, dma (rx, tx), pwm , would this mean i would have to pass in these params every time I make a library call into my driver?

[etrombly]

If you read japaric's blog posts about rtfm http://blog.japaric.io/tags/rtfm/ it should explain the reasoning. He is the one that created all the libraries this is built on.