embassy-rs / stm32-data

69 stars 101 forks source link

Let's make the GPIO::AFR API sane #449

Closed Ecco closed 5 months ago

Ecco commented 5 months ago

Let's say you want to set both GPIO A7 and A11 to alternate function 12. The current API that somewhat mimics the datasheets from ST is extremely annoying:

  1. You need to figure out if you need to modify afr(0) or afr(1)
  2. Then you need to (maybe) subtract 8 from your pin number.
GPIOA.afr(0).modify(|v| v.set_afr(7, 12));
GPIOA.afr(1).modify(|v| v.set_afr(3, 12));

This is very hard to read, write, and generally maintain. I would much rather write:

GPIOA.afr().modify(|v| v.set_afr(7, 12));
GPIOA.afr().modify(|v| v.set_afr(11, 12));

I'm not familiar with this repository just yet, but hopefully this might be rather easy to implement since the AFRL/AFRH registers are right next to each other. Really we could just consider the AFR as a single 64-bit register.

Dirbaio commented 5 months ago

I don't think this is a good idea. The PAC mirrors the hardware registers, and it's a fact that the AFs are split in two registers.

Modeling it as a single 64bit register might work (assuming the core doesn't try to do 64bit transfers on the bus, which are most likely not supported), but would be slower (2x 32bit loads, some 64bit math to set the right bits, then 2x 32bit writes), and the compiler wouldn't be able to optimize that because volatile.

Ecco commented 5 months ago

You do make a very valid point: in terms of the machine code that will eventually be emitted, there are some scenarios where it is indeed going to be (slightly) more efficient to keep these two registers separate. Fair enough!