Move away from using feature flags for configuration

[gibbz00]

Would it be possible to move away from defining l2cap-rx-packet-pool-size, l2cap-tx-packet-pool-size, gatt-client-notification-max-subscribers and gatt-client-notification-queue-size configuration in Cargo.toml? Say by letting users instead provide them with generic const parameters. The primary reason for why this bums me out is because of:

...features should be additive. That is, enabling a feature should not disable functionality, and it
should usually be safe to enable any combination of features.

(From https://doc.rust-lang.org/cargo/reference/features.html#feature-unification)

Many expect this principle to be respected setting, for example; rust-analyzer.config.cargo.features = "all". The current method makes this option unusable.

Another alternative could be to instead define this configuration in a metadata table. I haven't looked too much into how viable of an optional that would be.

[lulf]

Would it be possible to move away from defining l2cap-rx-packet-pool-size, l2cap-tx-packet-pool-size, gatt-client-notification-max-subscribers and gatt-client-notification-queue-size configuration in Cargo.toml? Say by letting users instead provide them with generic const parameters. The primary reason for why this bums me out is because of:

Using const generics for these would leak into all APIs and types that you typically need to pass around in the application, making the code considerably harder to write, read and maintain in my experience. Using feature flags rather than const generics is a trade-off, and some parameters are still const generics, in the cases where the value space is large and they don't need to leak into the API.

I'd be strongly against moving back to const generics (it was all const generics a while back), since we moved away from it already.

...features should be additive. That is, enabling a feature should not disable functionality, and it
should usually be safe to enable any combination of features.

(From https://doc.rust-lang.org/cargo/reference/features.html#feature-unification)

Many expect this principle to be respected setting, for example; rust-analyzer.config.cargo.features = "all". The current method makes this option unusable.

Using non-additive features is quite a common pattern in embedded rust, and users of this crate would most likely run into this issue for embassy as well, where you need to use feature flags to select which chip variant you're using for instance.

Another alternative could be to instead define this configuration in a metadata table. I haven't looked too much into how viable of an optional that would be.

I haven't really looked into this to know if it's viable or not, so I think if there can be a proof of concept showing that this could work I'd be open to that.

[gibbz00]

Thanks a lot for the explanation 😊 It helped a lot to hear that const genericds
have been previously been used, and that feature-flags was a trade-off deemed
worth taking.

Using non-additive features is quite a common pattern in embedded rust, and users of this crate would most likely run into this issue for embassy as well...

Fair point. I'll give the package-metadata option a quick try to see if it can produce a POC, leaving it be if it seems to be taking too long.

[gibbz00]

Also, if I were to open up a PR: Would the specified values have to be in powers of two?

[gibbz00]

I've now done some experimentation with package metadata, and I don't think that it will work. Cargo does not pass any metadata set by the primary package to the dependency being built. Suppose this is a consequence of it building each package in isolation.

The config module also contains:

Changing ANY of these configuration settings changes the on-disk format of the database. If you change them, you won't be able to read databases written with a different configuration.

Suppose an application has two dependencies, both of which depend in turn on trouble, but with different configurations enabled. Am I wrong to assume this edge case results in the dependencies breaking each other?

Granted, this edge case is not unique to the configuration by feature flags method. (Especially if "Currently, mounting doesn't check the on-disk database uses the same configuration") Perhaps it's not the best idea to let different dependencies set troble's compile time configuration in the first place?

As such, I think I've come to the (humble) opinion that the current possibility of configuring trouble through env variables is the one only one that should be allowed. This is based on the assumption that they can only be set by the shell invoking cargo, and certainly not by any other dependency that tries for some reason to call std::env::set_var() in their build scripts. If there's a need to declare them in a file, then a .env should in most cases suffice.

How would feel about removing the configuration by feature flags functionality all together?

[lulf]

Sorry for the late answer, I didn't see that you replied back on this. Thanks for checking up on the metadata approach.

I've now done some experimentation with package metadata, and I don't think that it will work. Cargo does not pass any metadata set by the primary package to the dependency being built. Suppose this is a consequence of it building each package in isolation.

The config module also contains:

Changing ANY of these configuration settings changes the on-disk format of the database. If you change them, you won't be able to read databases written with a different configuration.

This is stale comment from https://github.com/embassy-rs/ekv which I used as a basis for the feature flags, so it can be removed.

Suppose an application has two dependencies, both of which depend in turn on trouble, but with different configurations enabled. Am I wrong to assume this edge case results in the dependencies breaking each other?

I guess they would, but generally a library using trouble would not set any of these feature flags, they are primarily for the firmware 'end application' to set to tune queue sizes etc, which a library should not really care a about (what a lib would care about are features such as central, peripheral, gatt etc, which are additive). So in practice this is a non-issue I think.

Granted, this edge case is not unique to the configuration by feature flags method. (Especially if "Currently, mounting doesn't check the on-disk database uses the same configuration") Perhaps it's not the best idea to let different dependencies set troble's compile time configuration in the first place?

As such, I think I've come to the (humble) opinion that the current possibility of configuring trouble through env variables is the one only one that should be allowed. This is based on the assumption that they can only be set by the shell invoking cargo, and certainly not by any other dependency that tries for some reason to call std::env::set_var() in their build scripts. If there's a need to declare them in a file, then a .env should in most cases suffice.

How would feel about removing the configuration by feature flags functionality all together?

I don't see an issue keeping the current feature-based configuration around for now, it's more convenient than inconvenient from my POV.

[lure23]

Suppose an application has two dependencies, both of which depend in turn on trouble, but with different configurations enabled. Am I wrong to assume this edge case results in the dependencies breaking each other?

Cargo will enable both features (say, sizes 8 and 32), and the code base can be created so that the larger of these is what matters. In an unrelated project, this is how I do it.

I'm staying tuned to this issue, since wanting to see what alternatives there are. Env.vars (that you mention) is the only one I'm aware that could work.