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

Export FmcSdramConfiguration and FmcSdramTiming structs #2

Merged
merged 5 commits into from
Nov 7, 2020

Conversation

glindstedt
Copy link
Contributor

This allows implementing SdramChip outside of this crate.

This is useful when implementing a new device. Projects depending on this crate would not have to depend on a fork while waiting for a PR to be merged with the new device.

It would also make it much easier implementing a new device when depending on a HAL crate. In my case I was depending on stm32h7xx-hal (which depends on stm32-fmc) as well as depending on a local version of stm32-fmc with a new device implementation. This caused the issue described here: rust-lang/rust#22750. Not being able to implement SdramChip outside of the crate would force me to maintain forks of both stm32-fmc as well as stm32h7xx-hal until 1) stm32-fmc merges a PR, and 2) stm32h7xx-hal would adopt the new release.

I'm not sure if the structs were ever meant to be exposed, but I think having some way to implement the SdramChip trait outside of this crate would be really helpful.

@glindstedt
Copy link
Contributor Author

glindstedt commented Nov 7, 2020

Found a small workaround for my issue, which is to use a [patch] section as described here https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html#the-patch-section.

However, I still think it would be useful to be able to implement the trait outside the crate.

This allows implementing SdramChip outside of this crate
@richardeoin
Copy link
Member

Thanks! Agreed that exporting these is correct. Actually the naming is a bit confusing also, perhaps this is a good time to rename them to SdramConfiguration and SdramTiming? The Fmc prefix makes me think they come from fmc.rs (where they would have been exported).

It would also be useful to add a new test for implementing SdramChip outside the crate, so this doesn't get forgotten again.

Once this PR is merged I can make a patch release (0.2.1) which will be automatically used by stm32h7xx-hal after running cargo clean/rm Cargo.lock (the hal has a caret requirement). Whilst developing the [patch] section is the correct solution, a note about that could be added to README.md / DEVELOPING.md to help others who don't know about it.

@glindstedt
Copy link
Contributor Author

I've implemented what you suggested. For the test I just copied the existing sdram_pins_12a_4b test but used the DummyChip device (which has the configuration from is42s32800g_6). I believe just using it in the Sdram::new() method is enough to ensure that the trait is properly implemented.

@richardeoin
Copy link
Member

Awesome, looks good to me

@richardeoin richardeoin merged commit b658172 into stm32-rs:master Nov 7, 2020
@richardeoin
Copy link
Member

v0.2.1 released https://crates.io/crates/stm32-fmc

@glindstedt
Copy link
Contributor Author

Thanks! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants