search

rust-embedded-community / sh1106

SH1106 driver for use with embedded_hal and (optionally) embedded_graphics
Apache License 2.0
34 stars 36 forks source link

Make NoOutputPin generic #14

Closed pferreir closed 4 years ago

pferreir commented 5 years ago

This library doesn't seem to work OK with stm32f4xx-hal in its current incarnation. The problem seems to be the fact that NoOutputPin implements OutputPin<Error=()>, while stm32f4xx-hal expects <Error=Infallible>. The solution I've found was to make NoOutputPin generic, through a PinE type argument. I haven't found a way to avoid this Builder::<()>::new() nonsense. I would expect the compiler to infer that, but apparently that's not the case. Anyway, I hope this PR is useful. I'll be happy to make changes if it's at least partially usable.

jamwaffles commented 5 years ago

Hi, thanks for the PR! I'll do a quick review and see if I can fix the ::<()> when I have some free time.

pferreir commented 5 years ago

Cool! Thanks a lot. It would indeed be nice to get rid of that.

jamwaffles commented 5 years ago

I've come up with a solution to the turbofish issue. It's a bit long to put as review comments so I've added it to this gist. The main change being the CS type param is moved to the connect_spi() function. This now requires connect_spi() to always take a CS pin, but that can be placeholdered with NoOutputPin. Another option is to have another function like connect_spi_cs() for end users that want to use a CS pin.

I would've liked to be able to use .with_cs() on the SPI interface struct itself, but I couldn't get the generics working with NoOutputPin as a default.

Let me know what you think.

pferreir commented 5 years ago

Cool! LGTM :+1:

jamwaffles commented 5 years ago

Mint. Can you leave a comment or request me for review when you've made the changes? That way I get an email notification.

pferreir commented 4 years ago

Sorry, I had forgotten about this. So, you'd like me to commit the code anyway? I'm perfectly OK with you committing your own modified version, as much as I'd appreciate the credit 😃

jamwaffles commented 4 years ago

Hah me too. Feel free to take the solution in the gist I linked above and add it to this PR. I'll re-review it when you're done and we can get this merged :slightly_smiling_face: