Default values for const generic parameters MAX_DIRS, MAX_FILES, don't work as expected.

[jannic]

Commit https://github.com/rust-embedded-community/embedded-sdmmc-rs/commit/566f2023653dace195a43b05a6429f47e6be8a50 added two const generic parameters MAX_DIRS and MAX_FILES to Controller.

Those parameters have default values equal to the values used before, and the new docs say: "By default the Controller will initialize with a maximum number of 4 open directories and files. This can be customized by specifying the MAX_DIR and MAX_FILES generic consts of the Controller"

However, the default values don't work as expected. The following code causes a compile failure:

let mut cont = Controller::new(block, DummyTimesource::default());

error[E0282]: type annotations needed for `embedded_sdmmc::Controller<BlockSpi<'_, rp_pico::rp2040_hal::Spi<rp_pico::rp2040_hal::spi::Enabled, SPI0, 8>, rp_pico::rp2040_hal::gpio::Pin<Gpio5, rp_pico::rp2040_hal::gpio::Output<rp_pico::rp2040_hal::gpio::PushPull>>>, DummyTimesource, MAX_DIRS, MAX_FILES>`
   --> boards/rp-pico/examples/pico_spi_sd_card.rs:276:9
    |
276 |     let mut cont = Controller::new(block, DummyTimesource::default());
    |         ^^^^^^^^
    |
help: consider giving `cont` an explicit type, where the the value of const parameter `MAX_DIRS` is specified
    |
276 |     let mut cont: embedded_sdmmc::Controller<BlockSpi<'_, rp_pico::rp2040_hal::Spi<rp_pico::rp2040_hal::spi::Enabled, SPI0, 8>, rp_pico::rp2040_hal::gpio::Pin<Gpio5, rp_pico::rp2040_hal::gpio::Output<rp_pico::rp2040_hal::gpio::PushPull>>>, DummyTimesource, MAX_DIRS, MAX_FILES> = Controller::new(block, DummyTimesource::default());
    |                 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

For more information about this error, try `rustc --explain E0282`.

(This was noticed in rp-hal, see this ticket for context.)

There is a blog post which nicely explains why this happens: https://gankra.github.io/blah/defaults-affect-inference/#default-type-parameters

[jannic]

One way to handle this without a breaking change, and without forcing every caller to specify the two parameters, would be to provide two separate constructor methods:

       pub fn new(block_device: D, timesource: T) -> Controller<D, T>; // this one uses the default values and keeps the old API stable
       pub fn new_custom_max(block_device: D, timesource: T) -> Controller<D, T, MAX_DIRS, MAX_FILES>; // this one allows - and forces - the caller to specify the maximum values

This is essentially the same workaround as was used for HashMap with the two constructor methods HashMap::new and HashMap::default.

[ost-ing]

Hi @jannic, I'm sorry my change has caused an issue for you.

I think there are some other underlying issues here at play, mainly that there is an expectation that no breaking changes can be committed to the develop branch. No official release was made, so in my opinion breaking changes should always be expected.

For me personally I would expect that a library like this can change its API freely on its development branches, and when its time for a breaking version release (a major semver release I guess?) the community can react accordingly and update its packages and fix the issues in a controlled way. What I would like to see is controlled releases to crates.io.

Evidently we aren't doing this here, so your request for backward compatibility is justified. I have no issue with having two constructors, but I think @eldruin or @thejpster should chime in here with what the best approach is

[jannic]

Sorry if it sounded like I was complaining, that was not intended. Of course breaking API changes are perfectly OK at this point.

I was just wondering if the change was intended. The sentence "By default the Controller will initialize with a maximum number of 4 open directories and files" appears to indicate that there is a default value. But as it is, that default just isn't effective and the caller has to specify the values manually.

And as @eldruin was planning to do a release, an unintended breaking change at this point would be unfortunate.

I'm not qualified to evaluate which API would be best, as I'm not using this library myself, I just noticed the issue when an example in the rp-hal repo failed to build. (And the fact that this example relies on a development git version of embedded-sdmmc is of course the root cause of the build failure.)

[9names]

I think there are some other underlying issues here at play, mainly that there is an expectation that no breaking changes can be committed to the develop branch

The problem here is not that users expect the develop branch to not have breaking changes, it's that no release has happened in nearly 3 years. There's no point using the stable branch (or crates.io) from the end-user perspective if it is effectively unmaintained.

[eldruin]

We should do a release. @jannic 's workaround looks good to me. Would you mind adding it in a PR along with some documentation? Then we can move forward with this.

[jannic]

I opened https://github.com/rust-embedded-community/embedded-sdmmc-rs/pull/69. However I'm not entirely happy with the function name new_custom_max. Any better suggestions?

[thejpster]

with_capacity? I know the value isn't an argument so it's not exactly the same, but the precedent is set that constructor functions don't have to be called 'newX'.