Skip to content

Conversation

@soundanalogous
Copy link
Member

This is an alternative approach to: https://github.com/firmata/arduino/pull/247/files. However neither approach allows the user to override VERSION_BLINK_PIN if it is specified already in Boards.h for the board they are compiling against. This is because VERSION_BLINK_PIN is defined per board (that have an onboard LED) in Boards.h and Firmata.cpp includes Boards.h, so there is no way to undef VERSION_BLINK_PIN from the user's sketch.

@cmaglie
Copy link

cmaglie commented Dec 21, 2015

👍 LGTM

However I'm not sure how the feature can be disabled as you requested, other than the user modifying Boards.h to undefine VERSION_BLINK_PIN for a particular board.

Yes, the user should change an existing board or define his own. BTW this is mainly intended for boards without LEDs.

Thanks for taking the time to look at this one!

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nitpick, but should this also be FIRMATA_ prefixed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally but it has been defined in Boards.h for years. I'm not sure if anyone is using it in their sketches - probably not because it is only used in Firmata.cpp, but nothing prevents anyone from using it in a sketch as well. This is a issue I've been having lately. Each new Arduino board has included a duplicate define somewhere in it's code base that has broken something in Firmata so prefixing is necessary... just trying to figure out a way to do it without breaking existing sketches.

soundanalogous added a commit that referenced this pull request Dec 24, 2015
make VERSION_BLINK_PIN optional
@soundanalogous soundanalogous merged commit 59fb214 into master Dec 24, 2015
@soundanalogous soundanalogous deleted the optional-blink branch December 26, 2015 22:38
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