esp-rs
Refactor I2S master module
Problem
esp-hal/src/i2s/master.rs has grown to roughly 2.7 kLOC. The single file now mixes configuration objects, clock math, hardware/register plumbing, blocking driver logic, async helpers, and per-chip special cases, which makes the driver hard to navigate, reason about, and review.
Proposed Solution
Reorganize i2s::master as a directory module so each concern lives in its own file while preserving the existing public path, esp_hal::i2s::master. The async submodule keeps the current asynch spelling because async is reserved and downstream code already depends on that name.
esp-hal/src/i2s/master/ ├── mod.rs // existing docs + re-exports (replaces master.rs) ├── config.rs // Config/UnitConfig/ConfigError and validation helpers (~460 lines) ├── clock.rs // I2S clock divider math (~100 lines) ├── hardware.rs // “private” hardware abstraction, register access, per-SoC logic (~1,200 lines) ├── driver.rs // Blocking driver types (I2s/I2sTx/I2sRx), DMA plumbing (~500 lines) └── asynch.rs // Async DMA wrappers and futures (~220 lines)
Benefits
- Maintainability: Focused modules map to clear responsibilities, easing reviews.
- Readability: Developers can jump straight to config, clocks, driver, async, etc.
- Compilation ergonomics: Smaller translation units reduce incremental build churn.
- Extensibility: Leaves headroom for new features without re-inflating a single file.
- No API churn: master/mod.rs re-exports the same items, so downstream crates continue to compile unchanged.
Almost every other driver is organized in the same way. Having this one be different is probably not ideal.
Some structs in esp-hal/src/i2s/master.rs could probably be moved to esp-hal/src/i2s/mod.rs, if it's going to be shared with the slave driver.
There's also another refactor brewing here and the file might become easier to parse after that. The author hasn't been able to drag it to the finish line yet but I'd prefer if the judgement about how big the file is happened after that PR (or a similar one achieving the same refactor) lands.
Almost every other driver is organized in the same way. Having this one be different is probably not ideal.
I guess that means we should refactor everything!
There's also another refactor brewing here and the file might become easier to parse after that. The author hasn't been able to drag it to the finish line yet but I'd prefer if the judgement about how big the file is happened after that PR (or a similar one achieving the same refactor) lands.
yep, this makes sense. I guess we'll wait on #3639 before we do anything, but I feel like I²S deserves some love.
Besides the seemingly LLM-generated list of advantages, what would be the point in doing this? My monitor can display ~50 lines of code comfortably, and rust-analyzer/IDEs already makes navigating the codebase pretty easy. Especially since you aim to end up with a 1200 LOC file where the actualy fun bits are.
I guess that means we should refactor everything!
We appreciate enthusiasm, but is this really a meaningful change to make?
My monitor can display ~50 lines of code comfortably, and rust-analyzer/IDEs already makes navigating the codebase pretty easy.
I mean, we could technically fit all of esp_hal in a single file, but that's probably not the way to go.
If you don't find this issue necessary, feel free to close it. The reason I created this specific issue is because it felt like the first step in enhancing the i²s code in esp-hal (a thing that I would like to assist with and I find interesting), and that would make it more manageable.
I was sarcastic with this, should've been clearer:
I guess that means we should refactor everything!
That was just uncalled for though, and this attitude will repel new contributors from joining:
Besides the seemingly LLM-generated list of advantages
I guess we'll wait on #3639
No idea when the author will get back to it, feel free to cherry pick the commits and continue the changes.