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

Blink gpio10 #350

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Blink gpio10 #350

wants to merge 4 commits into from

Conversation

d-adamson
Copy link

@d-adamson d-adamson commented Mar 8, 2023

Propose an additional blink example that

  1. Uses an external led to work with both the pico and pico w
  2. Provides a serial out message
  3. Shows how to declare a variable used for debugging with "volatile" to avoid the inspected value showing as "optimized out"

@lurch
Copy link
Contributor

lurch commented Mar 8, 2023

I'm guessing this PR shouldn't include the JSON file?

@d-adamson
Copy link
Author

Correct. Sorry, my mistake.

What's the best way to correct this?

@aallan
Copy link

aallan commented Mar 9, 2023

As far as I know the existing blink.c using the on-board LED works on both Pico and Pico W? https://github.com/raspberrypi/pico-examples/blob/master/blink/blink.c

However, if we were going to include something like this — an additional blink example using an external LED — just like the two "hello world" examples (hello_usb and hello_serial) it should live in a sub-directory of the existing blink example, which should then be renamed and moved to its own subdirectory inside the existing blink/ directory.

In addition, since it's a getting started example, I think it should have a wiring diagram as it requires external wiring, e.g. see https://github.com/raspberrypi/pico-examples/tree/master/i2c/bmp280_i2c where there is a README.md file with an included wiring example.

That said, a blink example needs to be the simplest thing possible, parsable by folks that don't even know C, and are just familiar with other languages, and this is far longer and much more complex than the existing blink example. I don't believe this is suitable for folks that'll be looking to a blink example as their very first experience of the SDK.

@peterharperuk
Copy link
Contributor

That blink example doesn't work on Pico W. There's a separate one for Pico W https://github.com/raspberrypi/pico-examples/blob/master/pico_w/wifi/blink/picow_blink.c

@d-adamson
Copy link
Author

d-adamson commented Mar 9, 2023

In the spirit of keeping it simple, someone just starting may purchase the pico w as their first board (I did because I want to do an IOT project).

  • It is not immediately obvious that the current blink example does not work on the pico w.
  • The pico w blink example is pretty well buried in the examples directory tree.
  • picow_blink it is a slightly more complex program because the onboard led is tied to the wifi module rather than being a standard gpio => not a good teaching example.

For those reasons, the current blink example should be updated to be pico w friendly.

No problem leaving out the serial coms and the "volatile" declaration for simplicity.

@urbasus
Copy link

urbasus commented Mar 18, 2023

For the record I fell into this trap getting started, flashing the pico examples on a pico w, scratching my head and flailing about until I realized my mistake. A few more breadcrumbs and safety nets wouldn't hurt the experience.

@peterharperuk
Copy link
Contributor

Is it going to be any more obvious when you need a led and a resistor to make it work?

@@ -21,6 +21,7 @@ pico_sdk_init()
include(example_auto_set_url.cmake)
# Add blink example
add_subdirectory(blink)
add_subdirectory(blink-10)
Copy link
Contributor

Choose a reason for hiding this comment

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

blink_external_led is probably a more useful name than blink-10 ?

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.

None yet

5 participants