search

embassy-rs / stm32-data

75 stars 111 forks source link

enum of `lsbfirst`of SPI #328

Closed eZioPan closed 10 months ago

eZioPan commented 10 months ago

hi, @Dirbaio though we have discussed in PR #320 , for currently field and enmu, we will need to type

pac::SPI1.cr1().modify(|v| v.set_lsbfirst(vals::Lsbfirst::LSBFIRST));

to set the field, which is a little too verbose (lsbfirst 3 times).

Personally, I perfer

pac::SPI1.cr1().modify(|v| v.set_lsbfirst(true));

It's clear that set_lsbfirst(true) is related to sending data in LSB first. Only possible confusion is that, what is set_lsbfirst(false) mean? Could the false make user think there is some other bit order other than MSBFIRST? If there is little chance, then we can safely replace Lsbfirst enum with true/false, save both a few metapac size and user typing.

Dirbaio commented 10 months ago

as discussed in https://github.com/embassy-rs/stm32-data/pull/320#issuecomment-1868502581

when the choices are "do this, or do that", an enum is clearer, so we should keep it. When the choices are "enable this, or don't" then a bool is better.

The field name from ST is unfortunate yes, it'd make more sense to make it "byteorder", but it's what we have...

eZioPan commented 10 months ago

yeah, I’m agree with you on "it should have a better field name". Especially the description of this field in RM is "Frame format", which drives me a little mad :P

anyway, feel free to close this issue if it’s better not touch it, or tell me to do some modification. I’m ok with both choices.

Dirbaio commented 10 months ago

yeah, let's keep the enum for now. thanks!