rust-embedded-community / ssd1306

SSD1306 OLED driver
Apache License 2.0
286 stars 68 forks source link

examples/graphics*.rs: use new 'Triangle' primitive #97

Closed spicyjack closed 4 years ago

spicyjack commented 4 years ago

NOTE: I don't have a 3-wire SPI OLED board to test on, I only have a 4-wire SPI board available to me at the moment, so I was only able to test the refactored I2C demos. The SPI OLED example (graphics.rs) compiles, I just can't test it 😕. I tried grounding the CS line, but I wasn't able to get my 4-wire board to work.

If you're interested, I'd like to also add a blinkenled to the graphics examples, that way you can tell if the demo is running even if nothing shows up in the display.

128x32 I2C demo with a STM32F103C8T6 Black Pill black_pill_i2c_ssd1306_128x32

128x64 I2C demo with a STM32F103C8T6 Black Pill black_pill_i2c_ssd1306_128x64

spicyjack commented 4 years ago

A couple of small things:

  1. Please rebase master

I must have missed commits, I thought I was on master already, my apologies.

  1. Please run cargo fmt --all - this should get CI passing again

I figured there was magick going on here to cause the CI failures, I'll reformat.

  1. Please add a changelog entry to the Changed section under Unreleased (you'll have to add Unreleased to the top).

Sure, no problem.

If you're interested, I'd like to also add a blinkenled to the graphics examples, that way you can tell if the demo is running even if nothing shows up in the display.

I wouldn't be opposed to that as long as it doesn't make the examples significantly more complex. Some comments in the code as well as the //! module comment explaining why the LED blinks would be great.

There is a bit of increased complexity, you need to add a Delay, and route gpioc to reach pin 13, but I think the advantage to new-to-Rust/new-to-embedded people is that they will get feedback that something is working when they compile & flash. I'd be more than happy to sprinkle comments liberally throughout the graphics*.rs examples to explain the existing code and any new code that I add.

I have a little test bed setup for the SSD1306 and the SPI example works fine so no worries there.

Excellent! I'll have a few 3/4 wire SPI displays here in a few days for testing in any case.

jamwaffles commented 4 years ago

There is a bit of increased complexity, you need to add a Delay, and route gpioc to reach pin 13

That's absolutely fine. I was a little worried interrupts and other stuff would be introduced, only because it dilutes the actual e-g code.

I'll have a few 3/4 wire SPI displays here in a few days for testing in any case.

Pending a final review, I think this PR is good for merge. Would you like to wait until you've tested with your displays first?

spicyjack commented 4 years ago

There is a bit of increased complexity, you need to add a Delay, and route gpioc to reach pin 13

That's absolutely fine. I was a little worried interrupts and other stuff would be introduced, only because it dilutes the actual e-g code.

I agree, hence the question. Let me finish the blinkenLED changes and submit the PR, and if it turns out it adds too much complexity, I'm alright with not merging.

Pending a final review, I think this PR is good for merge. Would you like to wait until you've tested with your displays first?

No, if it's working for you on your SPI/3 demo board, I'd say merge it now, and SPI displays that I ordered can be used when I test the changes for the blinkenLED in the graphics.rs example.

jamwaffles commented 4 years ago

I agree, hence the question. Let me finish the blinkenLED changes and submit the PR No, if it's working for you on your SPI/3 demo board, I'd say merge it now

Works for me! Please leave a comment or re-request me for review with the 🔄 button when you're done so I get a notification 🙂