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

pimoroni display pack 2 example #395

Closed
wants to merge 7 commits into from

Conversation

jgmartin
Copy link

adds an example of using the pimoroni display pack 2.0 with embedded_graphics.

@jannic
Copy link
Member

jannic commented Jul 31, 2022

Oops - sorry about that. I thought that clippy lint didn't apply to no_std crates?
rust-lang/rust-clippy#5086 but it seems it was re-enabled later with rust-lang/rust-clippy#6161

This is no longer a correctness issue, so you could just disable the clippy warning. Or add cortex_m::asm::nop();, as pico_i2c_pio.rs does. Or, of course, the continue you had before, just without the comment regarding the optimizer.

See rp-rs#408 for reasoning why this is sensible.
@jannic
Copy link
Member

jannic commented Aug 4, 2022

@jgmartin, is 962742c fine with you?

@jgmartin
Copy link
Author

Perfect ty @jannic

@jannic
Copy link
Member

jannic commented Sep 10, 2022

@jgmartin unfortunately, this branch needs an update: embedded_time was replaced by fugit. Do you want to update the example? I can do it as well, if you like. But I don't have a pimoroni display pack to test the result.

Comment on lines 98 to 106
// Configure button inputs
// 12 - A button
let _a_pin = pins.gpio12.into_floating_input();
// 13 - B button
let _b_pin = pins.gpio13.into_floating_input();
// 14 - X button
let _x_pin = pins.gpio14.into_floating_input();
// 15 - Y button
let _y_pin = pins.gpio15.into_floating_input();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These switches do not have external pull-ups, you should enable the internal ones or you'll never be able to detect a button press.

Suggested change
// Configure button inputs
// 12 - A button
let _a_pin = pins.gpio12.into_floating_input();
// 13 - B button
let _b_pin = pins.gpio13.into_floating_input();
// 14 - X button
let _x_pin = pins.gpio14.into_floating_input();
// 15 - Y button
let _y_pin = pins.gpio15.into_floating_input();
// Configure button inputs
// The push-buttons connect their respective pin to ground, this is often referred to as "active-low"
// Since the board does not have external pull-up resistors, enable internal pull-up on each input
// 12 - A button
let _a_pin = pins.gpio12.into_pull_up_input();
// 13 - B button
let _b_pin = pins.gpio13.into_pull_up_input();
// 14 - X button
let _x_pin = pins.gpio14.into_pull_up_input();
// 15 - Y button
let _y_pin = pins.gpio15.into_pull_up_input();

@9names
Copy link
Member

9names commented Sep 11, 2022

This example is also compatible with Pimoroni Display Pack (1.0) as well - the pinout for LED, buttons and LCD controller for both is the same.
The LCD controller handles the differences between panels so the circle still renders correctly on the smaller screen even if you leave the screen size set to 320x240 (instead of actual screen resolution of 240x135).

@jgmartin
Copy link
Author

@jgmartin unfortunately, this branch needs an update: embedded_time was replaced by fugit. Do you want to update the example? I can do it as well, if you like. But I don't have a pimoroni display pack to test the result.

No problem I will do this and test it.


// Setup SPI
let spi_cs = pins.gpio17.into_push_pull_output();
let spi_miso = pins.gpio16.into_push_pull_output();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pin is not used as miso so I'd suggest giving it an name matching its use. eg: di_data_command

Comment on lines +98 to +106
// Configure button inputs
// 12 - A button
let _a_pin = pins.gpio12.into_pull_up_input();
// 13 - B button
let _b_pin = pins.gpio13.into_pull_up_input();
// 14 - X button
let _x_pin = pins.gpio14.into_pull_up_input();
// 15 - Y button
let _y_pin = pins.gpio15.into_pull_up_input();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those pins are not used (and are not configured for the purpose of another peripheral) so I think they should be removed from the example.

Alternatively, the example could draw on the display which button is pressed (as a letter or any other symbol).

Comment on lines +48 to +50
/// External high-speed crystal on the Raspberry Pi Pico board is 12 MHz. Adjust
/// if your board has a different frequency
const XTAL_FREQ_HZ: u32 = 12_000_000u32;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The definition provided by the rp_pico crate should be used instead.

@ithinuel
Copy link
Member

This PR should be moved to https://github.com/rp-rs/rp-hal-boards.
I am closing it here but be assured these changes would be welcome over there :) .

@ithinuel ithinuel closed this May 13, 2023
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.

4 participants