Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better Config struct #7

Open
Dirbaio opened this issue Sep 13, 2020 · 5 comments
Open

Better Config struct #7

Dirbaio opened this issue Sep 13, 2020 · 5 comments
Labels
help wanted Extra attention is needed

Comments

@Dirbaio
Copy link
Member

Dirbaio commented Sep 13, 2020

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

  • Very unergonomic, especially when they have bitfields.
  • All fields are Options, which is
  • Users override a field all-or-nothing. If they want to change a single setting of a group they can't, they have to specify Some and then the whole struct.

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.

@Dirbaio Dirbaio added the help wanted Extra attention is needed label Sep 13, 2020
@kuon
Copy link

kuon commented Sep 13, 2020

What pattern do you want to use? Builder pattern?

@Dirbaio
Copy link
Member Author

Dirbaio commented Sep 14, 2020

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
Copy link

kuon commented Sep 14, 2020

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
Copy link
Member Author

Dirbaio commented Sep 14, 2020

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

@kuon
Copy link

kuon commented Sep 14, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants