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

Add set_config method to RP SPI driver #3419

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dstric-aqueduct
Copy link
Contributor

@dstric-aqueduct dstric-aqueduct commented Oct 15, 2024

Add a set_config method to Spi struct to allow reconfiguring SPI mode after creation.

The existing implementation of the embassy-embedded-hal trait SetConfig is changed to use the new method.

Existing uses of SetConfig trait may need to explicitly call the trait method to maintain current return type.


// change stuff
p.cpsr().write(|w| w.set_cpsdvsr(presc));
p.cr0().write(|w| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is copypasted from new_inner. could you extract it to a private function fn apply_config() instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the CI failure, I see that the SetConfig trait in the embassy-embedded-hal actually does what I want, so this change may not be needed. Should we consider changing the trait implementation to disable/reenable the peripheral as the set_frequency method does?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whatever's needed for it to work. I don't know if it's required

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Pico SDK disables the driver before changing frequencies, as does the set_frequency method. I've updated to add a private apply_config method and then reused the Spi struct's set_config method in the trait implementation.

Copy link
Contributor Author

@dstric-aqueduct dstric-aqueduct Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would also be a breaking change as the methods could conflict depending on Trait in scope or not - I propose we just remove the proposed Spi set_config method and update the Trait implementation to disable/reenable the driver at the expense of some code duplication - I'll wait for your thoughts before touching again. Thanks for all your work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't worry about breaking changes, nextreleasewill be breaking anyway due to other changes.

Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@Dirbaio
Copy link
Member

Dirbaio commented Oct 15, 2024

  • Can you fix CI?
  • Can you squash all commits, and give it a good description of what you changed? "Update spi.rs" is not descriptive at all. (I recommend you do PRs by cloning the repo and using a real desktop git client, the github web ui encourages terrible practices...)

Add a `set_config` method to `Spi` to allow reconfiguring SPI mode after creation.

The existing implementation of the `embassy-embedded-hal` trait `SetConfig` is changed to use the new method.

Existing uses of `SetConfig`  trait may need to explicitly call the trait method to maintain current return type.
@dstric-aqueduct dstric-aqueduct changed the title Update spi.rs Add set_config method to RP SPI driver Oct 15, 2024
@dstric-aqueduct
Copy link
Contributor Author

  • Can you fix CI?
  • Can you squash all commits, and give it a good description of what you changed? "Update spi.rs" is not descriptive at all. (I recommend you do PRs by cloning the repo and using a real desktop git client, the github web ui encourages terrible practices...)

Squashed all the commits and cleaned up the PR description.

I think the CI is running cached tests so the modified spi_sdmmc example in the PR is not propagating to CI? Not sure how to sync this.

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