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

Refactor the analog module #1100

Merged
merged 7 commits into from
Jan 25, 2024
Merged

Conversation

jessebraham
Copy link
Member

@jessebraham jessebraham commented Jan 19, 2024

This is still largely untested, so opening as a draft for now, but will get it verified early next week. Just wanted to get some eyes on it in the meantime.

This refactoring had a few goals: improved code organization, reduced duplication, and improved documentation, primarily.

There are quite a few changes, but they are mostly pretty straight-forward.

Regarding the analog::dac module, I was able to eliminate the macro entirely and shrink the line count of the module overall, so that's a big win IMO. This came at the cost of a small amount of duplication, however it's quite minimal and this module is not likely to change significantly any time soon, so I think it's a reasonable tradeoff.

The analog::adc module was re-organized a bit, and I was able to extract out some common types from the various implementations, further reducing duplication. I also updated the documentation, which looks reasonably good now and has examples for both peripherals. I've additionally decoupled this driver from the embedded-hal traits while I was in here, by introducing the AdcChannel trait.

The peripheral constructors have changed a bit, and you no longer need to split a peripheral to get access to the ADCs/DACs, but other than that there aren't really any user-facing changes here, I don't think.

@jessebraham jessebraham marked this pull request as ready for review January 24, 2024 17:30
@jessebraham
Copy link
Member Author

jessebraham commented Jan 24, 2024

Did some really quick testing and things at least seem to still be working 😁

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.

I think I like the idea to get rid of the .split() - I wonder if it would be a good idea to do the same with SYSTEM.split()?

Overall looks good to me - changes things to the better. Maybe just consider hiding / sealing internal helper traits

Copy link
Contributor

@JurajSadel JurajSadel left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@jessebraham jessebraham added this pull request to the merge queue Jan 25, 2024
Merged via the queue into esp-rs:main with commit f52aa13 Jan 25, 2024
18 checks passed
@jessebraham jessebraham deleted the refactor/analog branch January 25, 2024 16:48
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