I recently managed to create a PAC for the Vorago VA108xx family of devices which are based on ARM Cortex-M0 and now I'm interested in writing a HAL. I've written a first version for a HAL which is heavily based on the stm32fXxx GPIO HAL implementations. Some of these implementation differ in style, which might be because the hardware might obviously be slightly different. For example, The stm32f0xx implementation used critical sections and then stole the pointer to the peripherals directly. Other implementations simply took a mutable reference to the peripheral instead, which is what I chose on a later iteration. Other than that, A lof of the code is similar to the other implementations, but there are some hardware specific details.
For example, the output pins are only stateful if a certain bit is set, which is different from the stm32 implementations, so I had to get a little bit creative there.
@robamu, I wrote the latest version of the GPIO module for the atsamd-hal. I took a slightly different approach that doesn't require macros. I'm not sure if you're interested, but I did a small write-up of it here and you can see the actual docs and implementation here.
Some people don't like the type-level approach, but I really appreciate when it catches mistakes at compile-time, which it has already done for me in real-life code.
it looks very interesting, thanks for t he good write up as well. I guess there is not "best-practice" yet when it comes to writing HAL code, and maybe there never will be. I think I will focus on getting working code for all major peripherals first, but I might consider refactoring it in the future
For example, The stm32f0xx implementation used critical sections and then stole the pointer to the peripherals directly. Other implementations simply took a mutable reference to the peripheral instead, which is what I chose on a later iteration.
Depending on your chip, critical sections or a reference to the peripheral may not be needed. For your chip, is it possible to change a pin's mode without modifying bits from other pins? I believe the stm32 family of devices often requires one of those two approaches, because the GPIO peripheral fundamentally cannot set pin modes individually. But not all chips are like that.
For instance, gpio::v1 of atsamd-hal looked a lot like your module. Each pin required access to the PORT peripheral when changing pin modes. But in gpio::v2, I was able to use the WRCONFIG register to set all of the pin mode bits independently for each pin. With that change, we no longer needed to arbitrate access to the PORT peripheral. Each pin has complete control over itself. (See the RegisterInterface trait in atsamd-hal for the way we did it)
Also note that, by requiring a reference to the peripheral, you still require some level of critical section in any non-trivial code. For instance, if you're using RTIC and need to access pins from multiple tasks (interrupt handlers) at multiple priority levels, you'll need to provide the peripheral as a shared resource. And shared resources need to be locked before use. The RTIC critical section is less than a full, interrupt::free critical section, because it takes priorities into account, but it's still a critical section.
I don't know that char is the best choice for port_id. You're using a four-byte, UTF-32 character, with thousands of possible options, to represent a choice between A and B. I think it might make more sense to create a custom enum here.
The *const dyn GpioRegExt seems unnecessary here. It's a fat pointer and uses dynamic dispatch, which is a high price to pay for basic IO. I think there's probably a better way to do that. Couldn't you make the GpioRegExt trait functions inherent functions on Pin instead? Maybe there's an issue with the PAC that makes that difficult? You could always patch the PAC. Or you might be able to work around it by making your own PAC-like struct (see example here).
You call Pin "fully-erased", but it still uses a type for the pin mode. If you wanted to create an array of multiple pins in different modes, it wouldn't be possible with Pin as it currently exists. In atsamd-hal, we have DynPin for the fully-erased version, if you're looking for more examples.
Does anything prevent users from calling .split() twice? I don't see anything. If not, users could safely create two copies of the same pin.
Thanks for the comments. I implemented port_id as an enum now.
Concerning your first point: Changing a pins mode requires two peripheral blocks right now: IOCONFIG and PORTX (X = A or B). I'm not sure what you mean with other pins. In the PORTA / PORTB block it only set individual bits depending on the pin number on that port. For IOCONFIG, each pin has an own register block with fields like pull up enable/disable , function select etc. But if I understand correctly: No, bits from other pins are not affected. I need to look into your implementation more deeply but it sounds interesting for my hardware because I am simply writing a mask to a write only register.
I have to look into the GpioRegExt trait again. It is again something I adapted from the stm32f0xx and stm32f1xx implementations. It is implemented for the register block, which allows all pin definitions to use it. If I implement this for Pin, I can't use the register block (which is a member variable of the struct) anymore to do this, and I would need to pass the peripheral as a reference / retrieve the raw pointer. The first way would not work for the embedded_hal traits, because the peripheral would then need to be a member. This would probably require it to be an additional associated type for Pin as well? I did this for the TIM peripheral, where the CountDown struct consumes the peripheral, but this is not possible here because PORTA is used for all GPIOs on PORTA. Retrieving the raw pointer of course is possible but would require an additional macro I think.
The fully erased pin being truly fully erased sounds very nice too and I will keep it in mind. I might focus on getting components like SPI, UART, I2C etc. working on a basic level first
Actually, there is nothing in place to call split() twice, I just tested it. The issue is that if the split functions consumes PORTA, I can't use it anymore, and a lot of GPIO functionality requires that I can still pass references to the block.. One solution for now might be to implement a singleton in the HAL. UPDATE: I added a singleton using the singleton! macro and it works like a charm to ensure the function is only called once
Without an in-depth understanding of the peripherals on your chip, it's hard for me to offer specific advice. Maybe it would help to explain in a little more detail what I do? I'm not sure.
In our code, the RegisterInterface trait is the core abstraction. It is meant to be implemented by each Pin<I, M> type, along with the type-erased DynPin. The id function returns the pin's corresponding DynPinId, which contains the pin group (A, B, C, D) and number (0..32). For Pins, the DynPinId is defined as an associated constant on the PinId trait, so it is known at compile-time. For DynPins, the DynPinId is stored within the struct and only known at run-time. The group function takes the DynPinId and unsafely conjures a reference to the corresponding pin group. The pin number can be used to index arrays within the pin group, giving access to the control bits for individual pins (see the pincfg function as an example).
Each Pin or DynPin is now completely free-standing, because it knows how to create pointers and references to the PAC structs it needs to access. But the code is careful to ensure that changing one pin can't possibly change another pin, even if interrupted.
In fact, each Pin struct is similar to a PAC struct. They are zero-sized structs that conjure pointers to the peripheral on demand (see the PORT PAC struct as an example).
I would think you could take a similar approach. You could unsafely create references to the IOCONFIG and PORTX structs. You just need to make sure that modifying one pin can't affect another pin, even when operations get interrupted (e.g. during calls to .modify(|r,w| {...})).
I've started to refactor the GPIO module. I used a lot of your code patterns, but I now encounter an issue when implementing the RegisterInterface . The IOCONFIG register block has 32 blocks for PORTA and 32 blocks (only 24 used here) for PORTB, but their types are different. This leads to ugly duplicate code which also will incur runtime overhead. Do you have any idea how to solve this? Maybe model the PORTA/PORTB blocks like you modelled the GROUPS block?
Yeah, I suspected that might be a problem when I looked through your code. I think that's a limitation of your SVD. It must not declare PORTA and PORTB as an array of a common type. Instead, it defines the same type twice.
One solution would be to patch the SVD and regenerate the PAC. I haven't done this myself, but we have done it within atsamd-hal. I'm not sure how difficult it is. Feel free to join us on Matrix here. You might also check out the Rust Embedded Matrix channel. It's another good resource.
There might be a way to use my GROUPS approach, but I'm not sure. I would have to dig into the different types.
You might also be able to modify the trait. Maybe you could define a new associated type, Port and have a function that returns Self::Port?
I'm not really sure what the best/easiest solution is. If you post the code, maybe I can take a look.
Edit: wait, I didn't see that you posted a link. I'll look
Ok, it looks like the va108xx::ioconfig::porta and va108xx::ioconfig::portb modules of your PAC are identical. Can you confirm there are no hidden differences?
If so, I think you can solve it like this. Create your own version of ioconfig::RegisterBlock, and have it define both the porta and portb fields using the same type (I chose porta::PORTA_SPEC).
One other comment, I wouldn't return &'static references. Let the lifetime of the reference be tied to the lifetime of the Pin or DynPin. I think you can only cause trouble by extending the lifetime, and you really don't need to do it.
Also, I think you have some bugs. Line 153 should probably be Self::PORTB. And lines 188 and 190 are identical.
Okay, the first version is working for my Blinky examples
One little thing: The Pins struct now includes every pin. This means that the new function now needs to enable both peripheral blocks for PORTA and PORTB for me because I can't make any assumptions about which ports will be used. I guess there is no way to make this work at compile time? The new function then looks like this
impl Pins {
/// Take ownership of the PAC
/// [`PORT`](crate::pac::PORT) and split it into
/// discrete [`Pin`]s
#[inline]
pub fn new(syscfg: &mut SYSCONFIG, enb_clk_porta: bool, enb_clk_portb: bool) -> Pins {
syscfg.peripheral_clk_enable.modify(|_, w| {
if enb_clk_porta {
w.porta().set_bit();
}
if enb_clk_portb {
w.portb().set_bit();
}
w.gpio().set_bit();
w.ioconfig().set_bit()
});
Pins {
// Safe because we only create one `Pin` per `PinId`
$(
[<$Id:lower>]: unsafe { Pin::new() },
)+
}
}
}
Actually, it would be nice if the new function consumes PORTA and PORTB, but this would ideally be two functions.. I'm not sure how to best solve this.
My first instinct would be to create PinsA and PinsB structs. Then have three different functions that consume IOCONFIG and one or both of PORTA and PORTB. Then return one or both of the Pins structs as necessary.
/// Create a new struct containing all the Pins. Please note that you need to
/// pass the PORTA and PORTB peripherals to enable their respective peripheral
/// clocks
#[inline]
pub fn new(
syscfg: &mut SYSCONFIG,
iocfg: IOCONFIG,
porta: Option<PORTA>,
portb: Option<PORTB>
) -> Pins {
syscfg.peripheral_clk_enable.modify(|_, w| {
if porta.is_some() {
w.porta().set_bit();
}
if portb.is_some() {
w.portb().set_bit();
}
w.gpio().set_bit();
w.ioconfig().set_bit()
});
Pins {
iocfg,
porta,
portb,
// Safe because we only create one `Pin` per `PinId`
$(
[<$Id:lower>]: unsafe { Pin::new() },
)+
}
}
/// Consumes the Pins struct and returns the port definitions
pub fn release(self) -> (IOCONFIG, Option<PORTA>, Option<PORTB>) {
(self.iocfg, self.porta, self.portb)
}
Unfortunately, this porbably still allows using PB pins even though their clock is disabled.. maybe your approach is the better idea
but that does not works because it requires the dyn keyword and the compiler complains about some other issues as well like that I would need to move the constants into another trait.. is there any other solution for this?
I think the general problem you describe is that there is no direct way for a struct to delegate a trait implementation to one of its fields. I think there have been some RFCs on the topic, but I'm not sure where they stand now. I know there are procedural macros for that purpose, e.g. ambassador and delegate, but I've never used them.
However, I think there might be a way to implement what you want using default implementations of trait functions.
I would keep the RegisterInterface trait private, since it provides access to the raw internals. You don't want to provide that to users directly. But you could probably define a PinInterface trait like this:
I just realized one shortcoming of this approach. It means you have to publicly expose the RegisterInterface trait. You could #[doc(hidden)] the regs and regs_mut functions, but it wouldn't stop users from accessing them if they want to.
Personally, I would probably just keep the boilerplate code. I wouldn't want to expose RegisterInterface to users.
I just kept it like this now, it would only be an internal change in any case.
Interestingly enough, the generated code size for a blinky example application is smaller with the newer type level API
Type-Level API:
Finished release [optimized + debuginfo] target(s) in 0.01s
text data bss dec hex filename
2804 0 4 2808 af8 blinky-leds
Old API:
Finished release [optimized + debuginfo] target(s) in 2.97s
text data bss dec hex filename
3276 4 4 3284 cd4 blinky-leds
Might be related to the GpioRegExt dynamic dispatch not being there anymore
When we first implemented this approach, we did make sure to test the code size relative to the old implementation. I'm not surprised it's the same or smaller.
Be aware, though, that you really need to be vigilant with #[inline]. I don't think Rust will inline across crate boundaries without it. And it's important to inline the calls, or I think you would get a lot of bloat.
.