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

Two minor fixes, add missing include and provide pin definitions for A0 to A7 #122

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

hlovdal
Copy link
Contributor

@hlovdal hlovdal commented Feb 25, 2019

No description provided.

@ianfixes
Copy link
Collaborator

I googled the name of those pins and found a file from @PaulStoffregen :
https://github.com/PaulStoffregen/cores/blob/master/teensy3/pins_arduino.h

Is there a larger effort in defining pins that we need to make here? (I won't delay merging this, I'll just open some issues)

@hlovdal
Copy link
Contributor Author

hlovdal commented Feb 26, 2019

Teensy appears to be a Arduino compatible ARM Coretex board with 34 I/O pins. So most proper Arduino hardware targets will have less, but I do not feel that there is any problem in overcommitting on providing possible non-existing pin definitions in the test environment. I will add and rebase the branch.

@hlovdal
Copy link
Contributor Author

hlovdal commented Feb 26, 2019

One question though. The pins have different numerical values depending on which hardware target, and some of them are conflicting, e.g. A20 is either 31 or 39 in the pins_arduino.h linked above.
And this is also the case for other pin definitions like

#define PIN_WIRE_SDA         (2)

in ~/.arduino15/packages/arduino/hardware/avr/1.6.209/variants/leonardo/pins_arduino.h vs

#define PIN_WIRE_SDA        (18)

in ~/.arduino15/packages/arduino/hardware/avr/1.6.209/variants/standard/pins_arduino.h

Do we need to keep a 1-to-1 consistency with all hardware targets? That will be a lot of work, or could I just create an enum with consecutive values (possibly starting on a non-compatible value like 100 just to avoid the situation that some of the pin definition are the same as the real hardware while some are not, and at least be consistent in that none are)).

Copy link
Collaborator

@ianfixes ianfixes left a comment

Choose a reason for hiding this comment

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

I'm still figuring out how this review thing works. I thought you would have seen these comments 7 hours ago with my other comment, but apparently not.

@@ -1,3 +1,5 @@
#include <ostream>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm assuming that I was breaking compilation (somewhere) without this. Can you describe where the trouble was?

@@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
## [Unreleased]
### Added

- Provide pin definitions for A0 to A7.
Copy link
Collaborator

Choose a reason for hiding this comment

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

But only on some platforms, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Most platforms have such definitions. Maybe this can be controlled using a NUM_ANALOG_PINS (which would be good to define in any case, along with NUM_DIGITAL_PINS, since those are at least defined for AVR-based boards: https://github.com/rlcamp/ArduinoCore-avr/blob/4c9f899f8eef12989903e474beea3a71a90f511c/variants/standard/pins_arduino.h#L28-L29)

@ianfixes
Copy link
Collaborator

It sounds like you understand my hesitation here, and I think you're asking the right questions.

Do we need to keep a 1-to-1 consistency with all hardware targets?

No. We just need to make sure that the mocks will work if you stay consistent (i.e. if within the library we refer to A20 and within the unittest we do something like assertEqual(A20, myLib->somePin), it should always work, and not give a 31 =/= 39 assertion error).

I do not feel that there is any problem in overcommitting on providing possible non-existing pin definitions in the test environment

It depends. One of the features offered by this library is the ability to unittest across Arduino hardware platforms, so I need to be somewhat careful that the test environments are accurate. (E.g. a library would break if it was doing #ifdef A20 to determine available hardware features.) I doubt that people are using A20 in particular for their macros, so that's probably fine. But that's how I'm approaching this.

The sort of pattern I'm trying to support is this one: https://github.com/adafruit/Adafruit-WS2801-Library/blob/master/Adafruit_WS2801.h#L59

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