SPI Ds from_bits is either incorrect, or very misleading

[dlaw]

https://docs.embassy.dev/stm32-metapac/git/stm32g473cc/spi/vals/enum.Ds.html#method.from_bits

What I expected to find: Ds::from_bits(8) = Ds::BITS8 = Ds::from(7)

The actual situation: Ds::from_bits(8) = Ds::BITS9 = Ds::from(8)

I'm not totally clear on where the from_bits function comes from in the metapac, so my apologies if this is not the right place for this issue to be filed.

[Dirbaio]

from_bits/to_bits convert to/from the binary value that's written to the register.

In this case indeed "8 bits" is binary value 0b0111, so 7. https://github.com/embassy-rs/stm32-data/blob/2b7ec569a5510c324693f0515ac8ea20b12917a9/data/registers/spi_v2.yaml#L442-L444

matches the RM:

so this is working as intended. ST decided to have the values "off by one", we can't do anything to "fix" it.

[dlaw]

@Dirbaio Thanks for the quick reply!

I understand that normal from()/into() is off by one because of the STM definitions.

I saw that from_bits() is a special definition for the Ds (data size) type only, because none of the other fields that I checked have from_bits() defined.

That is the reason why I thought that from_bits() was intended to match the BITSx values, instead of being another name for from().

In this case, I think it would be a good idea to add a documentation comment to from_bits(), to avoid this off-by-one pitfall for future users. I can contribute a PR, but would appreciate a pointer to which section of code is even responsible for generating the from_bits() impl.

[Dirbaio]

You could add a description field to the enum here, but it'll show up in the rustdoc for the enum, not the to_bits/from_bits methods. Those are fully autogenerated, there's no way to put docs in them.

Note all enums in stm32-metapac have from_bits, to_bits, it's not specific to DS.

the "bits" refer to "the binary value that's written to the register". I can see how it's confusing in particular for DS because it can be interpreted as "number of bits in the data width", but fixing this would require renaming from_bits, to_bits everywhere, which would be very breaking, and whatever name we pick could be similarly confusing in some other random enum.