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

Gpio: use less unsafe code #36

Closed
wants to merge 3 commits into from
Closed

Gpio: use less unsafe code #36

wants to merge 3 commits into from

Conversation

aurelj
Copy link
Member

@aurelj aurelj commented Nov 19, 2018

No description provided.

Copy link
Member

@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.

Please let's not do this. The macro code is already close to unreadable as it is, I'd rather have less than more of it... We already have the pin number in macro call, is there no way to use that information to generate the required places in the macro?

@aurelj
Copy link
Member Author

aurelj commented Nov 20, 2018

Hum... The heart of the macro code itself is quite a bit more readable after the change I think. Compare:

&(*$GPIOX::ptr()).moder.modify(|r, w| { w.bits((r.bits() & !(0b11 << offset)) | (0b01 << offset)) });

vs.

gpio.moder.modify(|_, w| { w.$moderi().output() );

And overall those patches are removing 7 lines of code, so it is actually less of it...

Now, if you are talking about the calls of gpio!(), yes this is a bit ugly with quite some redundancy. I've tried to get ride of it, and making those calls much cleaner and simpler using an intermediate macro that would expand all the fields identifier for just the pin number. Unfortunately I've not found any viable stable rust way of doing it. The paste crate looks promising but unfortunately it doesn't seem to work along with a #![no_std] crate. Anyway, that's something that can be improved in the future.

But overall, I think that's a small price to pay for less unsafe code, less lines of code, and more readable code.

What do you think ? Any idea to improve on this ?

@therealprof
Copy link
Member

@aurelj I'm not sure yet, make it would make sense to have two types of pins with their own impls in the background, a low kind (using afrl) and a high kind (using afrh) (potentially derived from a generic version for less code duplication).

In general I'd like to see minimised macro use since they inflate compile times and are incredibly painful to read and debug, cf. #37

@aurelj
Copy link
Member Author

aurelj commented Nov 20, 2018

@therealprof Indeed, the idea in #37 is quite interesting. I guess it won't be that easy to do the same for gpio as there are separate gpioa::RegisterBlock, gpiob::RegisterBlock and another gpio RegisterBlock that is different on each stm32f4 device.

But anyway, I will give it a try at some point, so for now, you can consider this PR on hold.

@therealprof therealprof added the DNM Do not merge label Dec 17, 2018
@TeXitoi
Copy link
Contributor

TeXitoi commented Mar 7, 2020

Any update on this? or we can close it?

@aurelj
Copy link
Member Author

aurelj commented Mar 8, 2020

I think the idea behind this patch is still worthwhile, but it is not a good idea to pursue before trying to reduce macro usage as much as possible.
So yeah, better closing it.

@aurelj aurelj closed this Mar 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DNM Do not merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants