-
Notifications
You must be signed in to change notification settings - Fork 238
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
Consistent SPI Timing (Need review) #943
Conversation
esp-hal-common/src/spi/master.rs
Outdated
@@ -2708,23 +2708,25 @@ pub trait Instance { | |||
// FIXME: Using something like `core::slice::from_raw_parts` and | |||
// `copy_from_slice` on the receive registers works only for the esp32 and | |||
// esp32c3 varaints. The reason for this is unknown. | |||
#[inline] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I guess the comment above this attribute is no longer right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, since the S3 and ESP32 should have the same processor?
@@ -2628,6 +2629,7 @@ pub trait Instance { | |||
/// you must ensure that the whole messages was written correctly, use | |||
/// [`Self::flush`]. | |||
// FIXME: See below. | |||
#[ram] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to state here, too, that I think singling out SPI here isn't very elegant. Ideally all code should be as fast as possible, these getting the #[ram]
treatment seems very arbitrary (to some who doesn't know the context). IMO before we start moving stuff into instruction ram somewhat haphazardly, we should probably come up with a systematic approach to tackle the problem.
That being said, good find!
Related: #946 |
@ProfFan thanks for this, sorry that it's just been sitting here for so long. Did you intend to rebase this and wrap up the work at any point, or can we go ahead and close this PR? |
I think the key perf improvments were already merged? @ProfFan correct me if I'm wrong. |
Yes! |
Thank you!
With this I am getting consistent 9ns for 20 bytes at 36MHz with LTO on.
Write-only is ~8ns, so at least not too much penalty.
Thank you for your contribution.
Please make sure that your submission includes the following:
Must
errors
orwarnings
.cargo fmt
was run.CHANGELOG.md
in the proper section.Nice to have