Skip to content

Clean up gpio.rs#155

Merged
therealprof merged 1 commit intomasterfrom
pin
Jun 12, 2020
Merged

Clean up gpio.rs#155
therealprof merged 1 commit intomasterfrom
pin

Conversation

@jonas-schievink
Copy link
Copy Markdown
Contributor

Fixes #146

This compacts the generic Pin struct to a single byte on all MCUs, encoding the port in the upper bit.

Breaking change.

@jamesmunns
Copy link
Copy Markdown
Member

If we're doing a breaking change, we might want to consider @hannobraun's suggestion to reduce the macro surface area, instead moving to a more Instance style way of doing things. We can do that in another PR though.

@Yatekii
Copy link
Copy Markdown
Contributor

Yatekii commented Jun 7, 2020

How is this performance wise? When I initially implemented this (I did the initial 840 impl) I thought that it rather save a few CPU cycles which are more important than the bit of memory we can save.
Do you think the other way round or do you think this has no perf implications anyways?

@jonas-schievink
Copy link
Copy Markdown
Contributor Author

This should have no performance implication on any chip with only one GPIO bank, and on the others most use cases should only be affected if the inliner fails. I'm happy to remove that change though, if anyone feels strongly.

@Yatekii
Copy link
Copy Markdown
Contributor

Yatekii commented Jun 7, 2020

I have no strong feelings. I was just curious what the actual implications might be. I had to decide what to do initially and picked the one I thought was a better fit, without any proof or strong oppinion :)

This change definitely seems ok to me.

@hannobraun
Copy link
Copy Markdown
Contributor

If we're doing a breaking change, we might want to consider @hannobraun's suggestion to reduce the macro surface area, instead moving to a more Instance style way of doing things. We can do that in another PR though.

For reference, James is referring to #8, I believe.

Copy link
Copy Markdown
Contributor

@therealprof therealprof 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!

@therealprof therealprof merged commit 3ab2af3 into master Jun 12, 2020
@therealprof therealprof deleted the pin branch June 12, 2020 09:45
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.

Pin's public fields let you break Pin's singleton property

5 participants