Skip to content

Conversation

@nmarley
Copy link
Contributor

@nmarley nmarley commented Jun 9, 2023

Hi, I was just running the blinky example and noticed the comments about an LED being connected to pin GPIO25. I was thinking it might makes more sense to use the built-in LED pin instead, and no external hardware would be required.

Hi, I was just running the blinky example and noticed the comments about an LED
being connected to pin GPIO25. I was thinking it might makes more sense to use
the built-in LED pin instead, and no external hardware would be required.
@bjoernQ
Copy link
Contributor

bjoernQ commented Jun 9, 2023

Thanks! Would be good to mention on which dev-boards it's like this and mention the need for an external LED in other cases. Besides that, if there are dev-boards with an LED on gpio 2 this totally makes sense

@nmarley
Copy link
Contributor Author

nmarley commented Jun 9, 2023

Yeah, that makes sense. I currently only have the ESP32 Devkitv1 (ESP-WROOM32) without any -Cx or -Sx variant, but after a bit of research it seems like the LED pin is all over the place based on which devkit is used. I've seen GPIO7, GPIO8, GPIO18 (RGB LED), etc. But since this change is only for the ESP32 Xtensa hal, maybe that doesn't matter. I've added the note back since this isn't only used for dev boards, but kept the GPIO2 change.

I added commit be690ef to make the PR pass the changelog check, but happy to remove it if that doesn't belong in the changelog. My personal thoughts are that it's trivial enough not to. Maybe the changelog check could be tightened to not include examples or only include changes in esp-hal-common?

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM

I'm fine with the change - using a GPIO that matches one dev-board is better than using a GPIO that matches no dev-board at all

@jessebraham any thought if s.th. like this should be mentioned in the CHANGELOG.md?

@jessebraham
Copy link
Member

@jessebraham any thought if s.th. like this should be mentioned in the CHANGELOG.md?

I think it's probably okay to omit, though we do have some example-related PRs in the CHANGELOG currently (I may squash these down to "Add support for ESP32-H2" or something). Guess we should discuss our policy for this a bit more at some point.

@jessebraham jessebraham merged commit c41e156 into esp-rs:main Jun 14, 2023
@nmarley nmarley deleted the esp32-example-builtin-led branch June 14, 2023 15:42
SergioGasquez pushed a commit to SergioGasquez/esp-hal that referenced this pull request Jun 15, 2023
* Use built-in LED pin (gpio2) in blinky example

Hi, I was just running the blinky example and noticed the comments about an LED
being connected to pin GPIO25. I was thinking it might makes more sense to use
the built-in LED pin instead, and no external hardware would be required.

* Add note on GPIO2 led

* Add GPIO2 LED pin change to changelog
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.

3 participants