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

Support for different PWM pins configurations out of the box #124

Closed
wants to merge 2 commits into from

Conversation

katyo
Copy link
Contributor

@katyo katyo commented Oct 22, 2019

I think this is quite usable thing here.

@katyo
Copy link
Contributor Author

katyo commented Oct 22, 2019

This PR doesn't includes doc updates yet.

@TheZoq2
Copy link
Member

TheZoq2 commented Oct 23, 2019

Looks pretty similar to my attempt in #120, except yours is shorter and easier to read 👍.

Is there a reason you decided to use a trait for the remap stuff instead of just passing the bytes to the macro directly? I think that would be more readable.

@katyo
Copy link
Contributor Author

katyo commented Oct 24, 2019

Hmm, maybe. It seems it looks like most obvious solution.

@katyo
Copy link
Contributor Author

katyo commented Oct 24, 2019

My solution is not optimal because I introduced special type for mapping (remap).
Actually, I think we can (and should) avoid it.

Becaue it hard to solve using macro rules so I defined types for mappings to complettely avoid this problem.
I haven't know the easy ways here. Maybe it's time call for help build.rs? :)

@TheZoq2
Copy link
Member

TheZoq2 commented Oct 26, 2019

I think you can get rid of the extra Remap trait by doing this:

    ( $( $TIMX:ident: $REMAP:expr ( $( ( $($PINX:ident),+ ), ( $($ENCHX:ident),+ ), ( $($DISCHX:ident),* ); )+ ); )+ ) => {
        $( $(
            impl Pins<$TIMX> for $($PINX<Alternate<PushPull>>,)+
            {
                const REMAP: u8 = $REMAP;
                $(const $ENCHX: bool = true;)+
                    $(const $DISCHX: bool = false;)*
                type Channels = ($(Pwm<$TIMX, $ENCHX>),+);
            }
        )+ )+

With that change, and the docs update, this will be good to merge in my eyes

@katyo
Copy link
Contributor Author

katyo commented Oct 28, 2019

This doesn't work because as I say before it produces same implementations of Pins trait for similar mappings with different pin sets.
I added remap ID to tuple to avoid it.

@TheZoq2
Copy link
Member

TheZoq2 commented Oct 29, 2019

Ah, makes sense. So users would have to specify the remap trait?

Could you do something similar to what I did in my PR where there is a special pins_impl which only impls the pins that change during a partial remap?

@katyo
Copy link
Contributor Author

katyo commented Oct 29, 2019

Of course, I tried to write something similar to it, but in a result it is tricky or not so clear macros.

Maybe enumerating all available configurations in macro invocation is a trade-of solution in this case.

@TheZoq2
Copy link
Member

TheZoq2 commented Nov 8, 2019

Maybe enumerating all available configurations in macro invocation is a trade-of solution in this case.

I think that's fine. This macro won't be seen by users, and I don't think we'll be modifying it much

@geomatsi geomatsi mentioned this pull request Nov 26, 2019
@burrbull burrbull mentioned this pull request Nov 27, 2019
@TheZoq2
Copy link
Member

TheZoq2 commented Dec 2, 2019

Since #147 which does the same thing is now merged, i'll close this in favour of that

@TheZoq2 TheZoq2 closed this Dec 2, 2019
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.

2 participants