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

Spi master: remove public hidden APIs, add Config/apply_config #2448

Merged
merged 8 commits into from
Nov 6, 2024

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Nov 1, 2024

Not the nicest PR, I didn't take the concept to its extremes yet. But the doc(hidden) APIs are gone so that might be a valid first step.

cc #2416 and #1919

There are a few bugfixes included as well.

@bugadani bugadani added the skip-changelog No changelog modification needed label Nov 1, 2024
@bugadani
Copy link
Contributor Author

bugadani commented Nov 1, 2024

What should we do with interrupts? We only expose GDMA DMA interrupt flags, not SPI peripheral interrupt flags (meaning none for PDMA devices). IMHO since set_interrupt_handler refers to peripheral interrupts, we should expose the peripheral interrupt flags. However, the underlying DMA's interrupt flags and handler are inaccessible.

@Dominaezzz
Copy link
Collaborator

However, the underlying DMA's interrupt flags and handler are inaccessible.

This is part of why I want to split them out into a separate object.

@bugadani
Copy link
Contributor Author

bugadani commented Nov 1, 2024

Yeah a separate .peripheral_interrupts()/.dma_interrupts() returning &InterruptControl would work. But returning either of them isn't trivial - which one to return, what if the user wants both? Do we end up with a single split_interrupt_control that returns all, or multiple ones? Neither of them sound ideal.

@Dominaezzz
Copy link
Collaborator

Do we end up with a single split_interrupt_control that returns all

This is probably best imo. At least for the PDMA case where the interrupt registers are shared between the DMA and peripheral. I definitely don't see this being a chip-agnostic API.

@bugadani bugadani force-pushed the spi-driver branch 2 times, most recently from 9efa3d8 to 7ff8039 Compare November 4, 2024 12:01
@bugadani bugadani marked this pull request as ready for review November 4, 2024 12:01
@bugadani bugadani changed the title Spi master: remove public hidden APIs Spi master: remove public hidden APIs, add Config/apply_config Nov 4, 2024
@bugadani bugadani force-pushed the spi-driver branch 4 times, most recently from f24670b to 7f8e7de Compare November 4, 2024 13:06
Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM

@bugadani
Copy link
Contributor Author

bugadani commented Nov 4, 2024

mmm I removed uart::config to allow uart::Config, but spi::master::Config/i2c::master::Config is somewhat annoying 😅 oh well...

@bugadani bugadani removed the skip-changelog No changelog modification needed label Nov 4, 2024
@MabezDev
Copy link
Member

MabezDev commented Nov 5, 2024

mmm I removed uart::config to allow uart::Config, but spi::master::Config/i2c::master::Config is somewhat annoying 😅 oh well...

at least master/slave adds some context, the config prefix on UART was just extra characters to type :D.

esp-hal/src/spi/master.rs Show resolved Hide resolved
esp-hal/src/spi/master.rs Show resolved Hide resolved
esp-hal/src/spi/master.rs Outdated Show resolved Hide resolved
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

LGTM, lets address my other review comments seperately.

@bugadani
Copy link
Contributor Author

bugadani commented Nov 5, 2024

I have elected to pick a random option and resolve the CI warnings. We'll have to touch the code anyway if we figure out what to do about #2416 so I'll take the tech debt and move on.

@bugadani bugadani added this pull request to the merge queue Nov 6, 2024
Merged via the queue into esp-rs:main with commit 8860aba Nov 6, 2024
28 checks passed
@bugadani bugadani deleted the spi-driver branch November 6, 2024 09:27
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.

4 participants