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

inherent gpio methods #1284

Merged
merged 1 commit into from
Apr 3, 2024
Merged

inherent gpio methods #1284

merged 1 commit into from
Apr 3, 2024

Conversation

burrbull
Copy link
Contributor

@jessebraham
Copy link
Member

Thanks for the PR.

We're still discussing what we're going to do regarding the various embedded-hal traits, so I'm hesitant to start refactoring our drivers specifically to accommodate them. Assuming that embedded-hal-compat gets updated, I'm leaning towards dropping the [email protected] trait implementations entirely, though there still needs to be some discussion surrounding this.

I think given our current implementations that this PR is completely reasonable, however I think I would prefer to come to a decision regarding embedded-hal before committing to refactoring a bunch of drivers.

@burrbull burrbull force-pushed the inherent-gpio branch 3 times, most recently from 3bded44 to aba68bc Compare March 13, 2024 21:39
@burrbull burrbull marked this pull request as ready for review March 13, 2024 21:39
@MabezDev
Copy link
Member

Maybe we could split this PR into two bits, the inherent infallible methods on the GPIO structs and the eh trait refactoring? Imo the inherent infallible methods we can accept now, before figuring out what to do with the traits.

@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 14, 2024

I like the idea of putting the hal implementations into modules

@burrbull
Copy link
Contributor Author

Rebased.

@jessebraham
Copy link
Member

I like the idea of putting the hal implementations into modules

I very much don't, but guess I won't be too stubborn about it. I'd like some guarantee that all of the other modules implementing these traits will be updated, because otherwise I have a feeling I will have to do this myself.

@burrbull
Copy link
Contributor Author

I very much don't, but guess I won't be too stubborn about it. I'd like some guarantee that all of the other modules implementing these traits will be updated, because otherwise I have a feeling I will have to do this myself.

Is problem in mod's or different files?

@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 15, 2024

I like the idea of putting the hal implementations into modules

I personally like to have it in modules - I don't like to have it in different files t.b.h.

@jessebraham
Copy link
Member

Is problem in mod's or different files?

At the end of the day it's largely just a matter of consistency, I guess my personal preferences don't matter as much (I seem to be in the minority as is often the case, anyway).

I've been putting a lot of work into trying to make our code base more consistent (this is still ongoing), and this feels like a step backwards unless these changes are made for all applicable modules. I think it's reasonable for contributors to expect our drivers to be structured in similar ways, and I can see having only one or two driver like this causing confusion (or, we'll have to explain to contributors constantly what is expected in this regard).

Given the above, it follows that we should also split out modules for all other third-party trait packages, and this seems tedious and like a lot of work for essentially zero tangible gain.

But, I won't argue any more about this I suppose. Just my $0.02

@burrbull
Copy link
Contributor Author

Is now better?

@burrbull burrbull force-pushed the inherent-gpio branch 2 times, most recently from 0fed74b to 2c9f9fb Compare March 21, 2024 04:12
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.

From my perspective, this LGTM. I'm a big fan of the infallible inherent methods, and I'm indifferent about the module layout, though I think cfg gated modules in the same file is a nice compromise between the two approaches.

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

@burrbull burrbull force-pushed the inherent-gpio branch 2 times, most recently from 3e957bf to 913a63a Compare April 2, 2024 13:00
@burrbull
Copy link
Contributor Author

burrbull commented Apr 2, 2024

Rebased.

@MabezDev MabezDev enabled auto-merge April 3, 2024 11:32
@MabezDev MabezDev added this pull request to the merge queue Apr 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 3, 2024
@MabezDev
Copy link
Member

MabezDev commented Apr 3, 2024

Sorry @burrbull, could you also update the tests inside hil-test? I will update the CI so we are building these before it gets to the HIL stage :).

examples/src/bin/ledc.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.

Thanks!

@MabezDev MabezDev added this pull request to the merge queue Apr 3, 2024
Merged via the queue into esp-rs:main with commit 13c8117 Apr 3, 2024
21 checks passed
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