embassy-rs / nrf-softdevice

Apache License 2.0
269 stars 80 forks source link

Better Config struct #7

Open Dirbaio opened 4 years ago

Dirbaio commented 4 years ago

The current Config struct is very ugly because it uses the original C structs.

The reason for the Options this is to allow users to not set them, in which case we don't even call sd_ble_cfg_set, and the softdevice uses a defaul configuration. Unfortunately the softdevice headers don't document the default of many settings :(

Ideally it'd be a pure-Rust struct with much better ergonomics and safety, implementing Default::default with defaults matching the softdevice's. We'd call sd_ble_cfg_set for absolutely every setting even if they're defaults.

kuon commented 4 years ago

What pattern do you want to use? Builder pattern?

Dirbaio commented 4 years ago

Not sure. Builders are nice but implementing them in the library is a bit verbose.

An alternative is just implementing Default, and then the user code can do this:

let config = nrf_softdevice::Config {
   field31: 123,
   field52: false,
   ..Default::default(),
}

This would make it so all fields are the default value excepts the ones explicitly set by the user.

kuon commented 4 years ago

Yeah, builders are really verbose, but I think they are more idiomatic. Personally I like them mostly because they can have signatures like:

pub fn set_name<S: Into<String>(self, name: S) -> Self {
}

Using traits on the builder allows for cleaner user code.

Dirbaio commented 4 years ago

True. Most of the config fields are integers or enums though.

kuon commented 4 years ago

I still think a builder would be the way to go eventually, but it's not a priority. And also we can wait to see how the library is used and if this is a requested feature.