Probably you should rethink API structure

[rumatoest]

Hello everyone.

I'm developing software more than 10 years.
I've seen a lot of anti-patters, but your code really impressed me.
I can definitely say that this is worst API approach I've ever seen in my life.

Looks like you've never try to write anything except POC with your API.
I was trying to create some kind of library using embedded-hal and stm32f1xx-hal
and found out that it is not worth it.
You should understand that I'm a really stubborn developer and I can dive deep into code
to understand how it works.

So what is wrong with your code.

Everything in your type system is wrong.
Sometimes it is even looks like some kind of joke to me, the same as bool x = true; if (x.toString().length() == 5) {}

Lets examine :gpio::gpiob::PB0 it is a separate struct that has same api as :gpio::gpiob::PB1
But PB1 is another struct so they can not be swapped in the code easily, you have to rewrite it.
Are you seriously thinking that defining separate types for any possible abstraction is good idea.

Also you've groupped pins into namespaces (probably is is somehow related to ports) and this is where really fun stuff begins.
Somehow deep inside your soul you are understand that using separate struct for each pin is bad idea.
So you've decided to make an abstraction via downgrade method that returns generic pin struct.
So now I'm able to downgrade PB0 to ::gpio::gpioa::PBx which is still bind to its namespace and it is not the same as ::gpio::gpioa::PAx.
Even generic pins in your API are not the same.

Next not so pleasant thing is pin function that mutates it to another "type" like fn into_push_pull_output(self, cr: &mut CRL) -> PA1<Output<PushPull>>.
It is not bad to mutate to another abstraction on state change, but only if method signature is like fn into_push_pull_output(self) -> PA1<Output<PushPull>>.
Expecting reference to second mutable parameter ruins everything.

My point here is that whole approach to API structure is wrong.
It is hard to understand and almost impossible to write flexible code.
But why we are using Rust here? We must remove complexity not adding new one.
Right now kernel GPIO API looks much more convenient than yours https://www.kernel.org/doc/Documentation/gpio/consumer.txt

Should you care about it?

Well I've suggest you too look into Arduino ecosystem. Arduino IDE is terrible and libraries system is half-work solution but it has Wiring with it's so called std library. Some devs clams that wiring is to high level and sometimes to slow comparing to C.
But it is simple, thus may devs were able to start their projects quickly.

stm32f1xx-hal should be more high level abstraction, for low level API we have cortex-m crates.

BTW I'm not only one who thinks that way nrf-rs/nrf-hal#8

[malcx95]

Torvalds, is that you?

[TheZoq2]

I can definitely say that this is worst API approach I've ever seen in my life.

Wow, that's some pretty fierce criticism...

I was trying to create some kind of library using embedded-hal and stm32f1xx-hal
and found out that it is not worth it.

https://github.com/rust-embedded/awesome-embedded-rust Lots of people have written libraries for this ecosystem, perhaps you can look there for inspiration.

Anyways, while your issue comes off as a bit aggressive, your issues are still valid, so let's discuss them.

Lets examine :gpio::gpiob::PB0 it is a separate struct that has same api as :gpio::gpiob::PB1
But PB1 is another struct so they can not be swapped in the code easily, you have to rewrite it.
Are you seriously thinking that defining separate types for any possible abstraction is good idea.

This might seem counter intuitive, but it has a large advantage when working with peripherals. It allows us to statically make sure that pins are only used once, and that the pins you intend to use are available. As an example, if you write the following code:

// USART1 on Pins A9 and A10
let pin_tx = gpioa.pa9.into_alternate_push_pull(&mut gpioa.crh);
let pin_rx = gpioa.pa11; // PA11 can not be used with USART1
// Create an interface struct for USART1 with 9600 Baud
let serial = Serial::usart1(
   p.USART1,
   (pin_tx, pin_rx),
   &mut afio.mapr,
   9_600.bps(),
   clocks,
   &mut rcc.apb2,
);

This will produce an error, something along the lines of expected ...(PA9, PA10)..., got (PA11).

In general, the API is designed to never compile unless a peripheral is properly configured, that includes all pins, their modes and all registers.

You mention the arduino API, which I think is pretty decent, but I have no idea what this code would do:

USART1.begin()
pinMode(PA10, Output)

So you've decided to make an abstraction via downgrade method that returns generic pin struct.
So now I'm able to downgrade PB0 to ::gpio::gpioa::PBx which is still bind to its namespace and it is not the same as ::gpio::gpioa::PAx.

This is indeed a problem which I've heard discussed quite often. I could be wrong about this as I didn't write the original GPIO code, but I believe this is related to ownership over registers. I think this is something we should look into fixing though.

Also, in general, the whole embedded API is designed around generics rather than runtime resolution of things. I.e. if you want to use 3 pins in a function you have two theoretical options:

// Prefer this
fn do_stuff<(P1, P2, P3)>(pins: (P1, P2, P3)) -> ...
    where P1: OutputPin, P2: OutputPin, P3: OutputPin
{}

// Over this
fn do_stuff(pins: (GenericOutputStruct, GenericOutputStruct, GenericOutputStruct)) {}

It's cumbersome to write libraries like this, but the resulting code will be faster, and rusts type inference will hide most of the generic types on the user side, especially if you use some typedefs.

Next not so pleasant thing is pin function that mutates it to another "type" like fn into_push_pull_output(self, cr: &mut CRL) -> PA1<Output>.

This is also valid, and something I've run into myself a couple of times. I think it too is related to ownership over the CRL struct, but I don't see a fundamental reason that we couldn't safely write to the relevant part of the register if we have a PAx struct. After all, we write to a shared data register.

My point here is that whole approach to API structure is wrong.
It is hard to understand and almost impossible to write flexible code.

I would probably argue that this isn't an API issue, apart from some kinks like the one you mentioned. Instead, I think it is a documentation issue, because I know from experience that it's possible to write functioning software with it. Heck, the keyboard i'm writing this reply on is running on top of this API.

But why we are using Rust here? We must remove complexity not adding new one.
In my eyes we're adding a bit of complexity with initial setup to ensure that compiled code is working code.

Ok, that's a long reply, hopefully it clears things up. I'm going to look into making the GPIO pins a bit easier to use in a generic way. Also, you're always free to submit PRs fixing the issues you find, after all, you can dive deep into code to understand how it works 😉

[TeXitoi]

Heck, the keyboard i'm writing this reply on is running on top of this API.

Just curious, what keyboard are you using?

[Emiluren]

@TeXitoi Hello, I am a friend of TheZoq2. He built his own keyboard which explains why it is running code using a library he maintains.

[TeXitoi]

Any link to the code? I myself also write on a keyboard that use this library, so curious to know if that's the same code or if we can share the effort.

Mine: https://github.com/TeXitoi/keyberon

[TeXitoi]

I agree that it would be great to have a downgrade method that is only 1 type, not one by gpio device. I have to do some hacky things to workaround that in a generic way here:

https://github.com/TeXitoi/keyberon/blob/master/src/matrix.rs#L7-L99

[TheZoq2]

Any link to the code? I myself also write on a keyboard that use this library, so curious to know if that's the same code or if we can share the effort.

The main repo is here: https://gitlab.com/TheZoq2/kthreeyboard_firmware. Keebrs is a fork of another keyboard project with some minor fixes. https://gitlab.com/TheZoq2/polymer is the board specific code.

The workaround for the issues mentioned in this issue are mainly in https://gitlab.com/TheZoq2/polymer/blob/dev/src/matrix.rs

Sidenote: with your keyboard project, there are three separate rust keyboard projects that i'm aware of :P

[TeXitoi]

I also know https://github.com/ah-/anne-key

[TheZoq2]

Ok, that makes it 4. Here is the other one I know of https://github.com/TyberiusPrime/keytokey

Edit:

I myself also write on a keyboard that use this library, so curious to know if that's the same code or if we can share the effort.

Sharing the effort sounds like a pretty good idea. It would be nice to start something like QMK but rusty. I believe that is the intent of the keebrs project, but I'd be open to something else as well

[rumatoest]

Torvalds, is that you?

No, it just a normal way of conversation in my country 😄

Lots of people have written libraries for this ecosystem, perhaps you can look there for inspiration.

Probably later when my internal masochist would wake up.

In general, the API is designed to never compile unless a peripheral is properly configured, that includes all pins, their modes and all registers.

Whew. I knew that you are trying to do not sacrifice performance at any cost.

It's cumbersome to write libraries like this, but the resulting code will be faster, and rusts type inference will hide most of the generic types on the user side, especially if you use some typedefs.

It is really complicated for me to imagine how can I create some kind of application where pin numbers configured via constants in some configuration block. For example as it was done in Marlin firmware.
In your case I have to get instance of exactly pin struct derived from some kind of port instance and then I would pass reference to pin as trait or boxed trait inside function.

You mention the arduino API, which I think is pretty decent, but I have no idea what this code would do

IMHO arduino lib is not perfect, but probably having generic pin struct is good idea

let sensor_location = PortA::PA10;
let p: Pin = whatewer.gpioa.acqurePin(sensor_location).unwrap();
let output = p.into_output();
// etc.

I don't see a fundamental reason that we couldn't safely write to the relevant part of the register if we have a PAx struct. After all, we write to a shared data register.

I assume that you have put excessive "don't" in your notion and you've said that it is bad idea write into shared register, but using Result types is not so good option because you want it to be checked at compile time. But this also restricts you to write using "low level" abstractions.

I think it is a documentation issue, because I know from experience that it's possible to write functioning software with it.

Documentation could help, but there is a possibility that it can be cumbersome because of overall API structure.

I'm going to look into making the GPIO pins a bit easier to use in a generic way. Also, you're always free to submit PRs fixing the issues you find, after all

I have no intend to submit PR with global refactoring, but I'll think about something.

Heck, the keyboard I'm writing this reply on is running on top of this API.

Good for you.
My main issue is that I was not able to implement API for some sensor that is using only one pin for data communication. And this pin have to be switched between input/output mode. Plus I have several other components like shift registers that works use custom protocols and single pin for data transferring (surely not spi / i2c). I do not want to say that it is not possible to do, but implementation tends to be ugly because it would require to many code on client side in order to configure my library.

[TheZoq2]

Whew. I knew that you are trying to do not sacrifice performance at any cost.

I wouldn't say at any cost, but when we can do something at compile time, we
try our best. Of course we could, and probably should add slower more generic
things where possible.

It is really complicated for me to imagine how can I create some kind of application where pin numbers configured via constants in some configuration block.

Do you mean something like this?

const int BUTTON_PIN = 5;
const int LED_PIN = 7;
void init() {
    pinMode(STEPPER_PIN, Input);
    pinMode(LED_PIN, Output);
}

If so, you can achieve the same thing using "typedefs"

type ButtonPin = PA5<Input<PullUp>>;
type LedPin = PA7<Output<PushPull>>;

fn main () {
    // Here you would need to specify the pins again, i.e.
    let led_pin = gpioa.pa7.into_push_pull_output(&mut gpioa.CRL);
}

// But everywhere else, you can just use the types
fn light_led(led: &mut LedPin) {
    led.set_high();
}

// And if a function doesn't care about which pin is used, make it generic
fn light_any_led(led: impl embedded_hal::OutputPin) {
    led.set_high();
}

Is this kind of what you had in mind, or did I missinterpret what you said?

I assume that you have put excessive "don't" in your notion and you've said
that it is bad idea write into shared register, but using Result types is not
so good option because you want it to be checked at compile time. But this
also restricts you to write using "low level" abstractions.

No, the don't is meant to be there. I believe we could create a better solution
that still guarantees correctness at compile time. I'll have a look at doing
just that later

My main issue is that I was not able to implement API for some sensor that is
using only one pin for data communication. And this pin have to be switched
between input/output mode. Plus I have several other components like shift
registers that works use custom protocols and single pin for data
transferring (surely not spi / i2c). I do not want to say that it is not
possible to do, but implementation tends to be ugly because it would require
to many code on client side in order to configure my library.

This seems like a completely different issue from what you've talked about previously, and also something I've run into. Since you have another repo for communicating with DHT22 sensors, I pressume that's what you're talking about.

I happen to have code for communicating with such a sensor here
https://github.com/TheZoq2/weather/blob/master/stmhardware/src/dhtxx.rs. I'm
not super happy with it, because it requires taking ownership over the pin
while doing the processing, but it does work. This code is a few months old, so
I think it could be improved slightly.

However, as I said, this is a separate issue and one that I am aware of, if we manage to
bake the GPIO CR[LH] into the pins themselves, this should be a fairly easy
problem to solve. I'll look into doing that later, probably.

Also part of the reason that InputOutput pins aren't implemented here is that
it's not implemented in embedded_hal.

[therealprof]

@rumatoest

No, it just a normal way of conversation in my country 😄

This tone is absolutely not welcome here.

However, as I said, this is a separate issue and one that I am aware of, if we manage to
bake the GPIO CR[LH] into the pins themselves, this should be a fairly easy
problem to solve. I'll look into doing that later, probably.

The problem is that if you do read-modify-write operations on registers you'll need to protect those. The only way to do that without locking is by taking a &mut to guarantee exclusive ownership. Other than that the options are:

• implicit critical section
• taking a critical section as a parameter
• moving it into another clonable component providing internal locking e.g. using atomic instructions

Providing proper type erased pins without tons of overhead is something I've been trying to implement for STM32F0 for many days now. I haven't yet found a way to do it using just the type system...

[TheZoq2]

The problem is that if you do read-modify-write operations on registers you'll need to protect those.

Oh right, I thought that was what we're doing when setting pin values, but I guess not. https://github.com/stm32-rs/stm32f1xx-hal/blob/master/src/gpio.rs#L163

Also, unless I'm missing something, you wouldn't be able to mix your proposed solutions with the current solution as you would need a lock around crl and crh, right?

[therealprof]

Also, unless I'm missing something, you wouldn't be able to mix your proposed solutions with the current solution as you would need a lock around crl and crh, right?

Well, you can do implicit locking in the implementation but that is frowned upon, especially by the RTFM guys.

[rumatoest]

Since you have another repo for communicating with DHT22 sensors, I pressume that's what you're talking about.

Yes right now I've stuck there. Looks like @TheZoq2 driver is not universal. It is created only over stm32f1xx_hal crate and can not utilize embedded_hal API only. That is why I was trying to create my hal only implementation.

The problem is that if you do read-modify-write operations on registers you'll need to protect those. The only way to do that without locking is by taking a &mut to guarantee exclusive ownership.

In this case for now having structure that wraps PIN trait inside it is not possible for my case.
The only one reasonable solution is to have a separate function that would accept several &mut references and produce some result.

Also part of the reason that InputOutput pins aren't implemented here is that
it's not implemented in embedded_hal.

I'm trying to reach them and share my ideas.

No, it just a normal way of conversation in my country smile

This tone is absolutely not welcome here.

Don't be so boring or you'll end up writing boring APIs 🌝

[TeXitoi]

Don't be so boring or you'll end up writing boring APIs 🌝

https://www.rust-lang.org/policies/code-of-conduct

[TeXitoi]

For the one interested in the keyboard crate discussion, I propose to go to TeXitoi/keyberon#14 to continue, as it is off topic here.

[TeXitoi]

@therealprof

Providing proper type erased pins without tons of overhead is something I've been trying to implement for STM32F0 for many days now. I haven't yet found a way to do it using just the type system...

A simple generalization of the downgrade function should be quite trivial, something like:

enum Gpio<MODE> {
    PAx(PAx<MODE>),
    PBx(PBx<MODE>),
    PCx(PCx<MODE>),
    PDx(PDx<MODE>),
}

impl<MODE> OutputPin for Gpio<Output<MODE>> {
    type Error = Void;
    fn set_low(&mut self) -> Result<(), Self::Error> {
        match self {
            PAx(g) => g.set_low(),
            PBx(g) => g.set_low(),
            // ...
    }
    fn set_high(&mut self) -> Result<(), Self::Error> {
        // ...
    }
}

// ...

Or I missed something?

[TheZoq2]

I also had that thought and I don't see a reason it wouldn't work. In fact, I'll go implement it now to see if it works

[therealprof]

A simple generalization of the downgrade function should be quite trivial, something like:

Interesting. That sounds rather simple and might work. I take it you'd generate the enum via the usual macro spaghetti code?

[TeXitoi]

Interesting related crate:https://docs.rs/enum_dispatch/0.1.4/enum_dispatch/

[rumatoest]

I believe we could create a better solution
that still guarantees correctness at compile time. I'll have a look at doing
just that later

Is it possible to use atomc CAS (compare and swap) ARM instructions while working with gpio registers or something similar. Probably it would allow not to use &mut reference to registers structs.

[rumatoest]

This is how I see generic solution in general 😕

Having some king of global singleton structure that controls gpio registers (AllGPIO)
AllGPIO can produce AnyPin structure only once per pin plus it can mutate AnyPin. If you are already have AnyPin it should be consumed by AllGPIO

Each AnyPin contains reference AllGPIO. AnyPin can mutate itself by consuming self and calling appropriate methods from AllGPIO

When pin mutates (changes its state) unsafe code with atomic CAS or "synchonized like" blocks used to change pin registers (http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dai0321a/BIHBFEIB.html).

This may allow to modify pin in place without necessity to use &mut link to another structure, and having generic pin struct, and generic port controller.

[TeXitoi]

It would be great to remove this &mut. It would be sufficient to fix these problems (no globals needed).

It may require additional feature on sdv2rust to have an atomic modify.

[TheZoq2]

Is it possible to use atomc CAS (compare and swap) ARM instructions while working with gpio registers or something similar. Probably it would allow not to use &mut reference to registers structs.

IIRC, atomic CAS still adds a performance penalty because you essentially have to do this:

loop {
    let value = read_register();
    let new_value = value.modify();
    if cas(new_value) == value {break;}
}

Some sort of atomic modify might also be nice. It would also probably require additional features in svd2rust, but perhaps that's worth investigating.

Also, I think it may be worth implementing even if it has a performance penalty, I doubt a lot of code is doing enough GPIO toggles to really be affected.

This is how I see generic solution in general

I'm not sure I undestand the whole solution you're proposing, but it's certainly something we can investigate.

[TheZoq2]

Actually, I feel like doing atomic instructions might be a bit overkill. Can't we just disable interrupts around the crx.modify call?

AFAIK, there are no multi core f1 processors, so disabling interrupts should be safe. Also, the modify calls should probably be quite fast so I don't think we'd be adding that much extra interrupt latency. Also, I can't see a case where you would be switching pin modes very often.

[TeXitoi]

As a gpio pin "own" a subset of the bits of the CR, in theory, it can be done freely using the bit-band.

[TeXitoi]

I agree that CAS and critical section have a cost, but that it should not be a problem in practice.

Intuitively, I'd prefer the CAS version to the critical section version, but that's just intuition.

Bit-banding seems to be the solution, but will need some big modifications on svd2rust.

[rumatoest]

User experience suffer update report.

Well I was able to manage DHT sensors protocol in not so cool way, but it works. Fortunately it was because it have pull up resistor in module by default and stm32f1xx-hal supports open drain pins.

But now I'm trying to deal with tm1637 and it is extremely hard to do somethin. tm1637 module does not have pull up resistor. From C++ you can see that in order to get ACK from chip pin have to be switched into pull up input mode for a while. And this is not possible with stm32f1xx-hal using HAL traits. Only one pin mode support Input+Output trait and it is Output.

I think It will be possible to use this module when I add pull up resistor, but it does not look good solution because other API allows me to work without circuit modifications.

Second thing I should notice is Delays & Timers AFAIK I can have only one delay or timer object and because HAL traits require &mut access it is not possible to pass references to several places where delay is used.

Looks like I have to pass delay in each function and can not wrap it somewhere in struct as reference. This is really bad and sad and whatever ...

[eldruin]

I looked into writing a driver for the TM1637 a long time ago but dropped it because of all the bitbanging necessary.
Regarding the delay/timer problems, nowadays we have the bigbang-hal. Maybe you can use some portions of code since the device speaks kind of I2C but without the addressing part. Maybe you can do something similar like what they do with the i2c CLK here

[TheZoq2]

With #211 merged, I believe all actionable issues here have been fixed :)