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

Add debug derive for the GPIO structures #861

Merged
merged 10 commits into from
Oct 19, 2024
Merged

Conversation

robamu
Copy link
Contributor

@robamu robamu commented Oct 17, 2024

Is there a general problem adding these derives to more structures? I am trying to use a driver for a LCD display, and I can not unwrap the returned result structure because Debug is not implemented by the pin structures used by the driver. This applies to both the concrete pins and the downgraded pins.

These were only the derives I needed to pass both dyn pins and concrete pins to the driver. There are probably a lot more structures where t his derive could be added.

@robamu robamu changed the title implement Debug support for the GPIO structures Add debug derive for the GPIO structures Oct 17, 2024
Older versions contain a soundness bug.
…tate

spi: remove undesired parameter to `set_format`
@jannic
Copy link
Member

jannic commented Oct 17, 2024

Adding more Debug derives is fine. The only disadvantage are slightly increased build times, but that's not a reason to avoid them in cases where they are useful.

Could you apply the same changes to rp235x-hal? It would be great if we could keep the two hal implementations as similar as possible.

@jannic
Copy link
Member

jannic commented Oct 17, 2024

CI failed due to a new warning in rustc 1.82.0.
Fixed in #863.

Since rust 1.82.0, addr_of is no longer unsafe:
rust-lang/rust#125834

Just removing the unsafe statement would break builds with older
versions of rust. Instead, allow the unused_unsafe lint.
Update critical-section dependency to version 1.2.0
@thejpster
Copy link
Member

@robamu can you rebase?

@robamu
Copy link
Contributor Author

robamu commented Oct 18, 2024

I can do that

@thejpster
Copy link
Member

This PR picked up a lot of unrelated commits. Maybe we can fix it by doing a rebase when we merge. The diff itself looks fine.

@jannic
Copy link
Member

jannic commented Oct 19, 2024

I think the pull request looks a bit strange because @robamu merged with main instead of rebasing. Git should handle that fine. Worst case the history will have a few unnecessary branch points, but yes, "rebase and merge" should avoid even that.

@jannic jannic merged commit 1bce4c9 into rp-rs:main Oct 19, 2024
49 checks passed
jannic added a commit to jannic/rp-hal that referenced this pull request Oct 19, 2024
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