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.

Sign up for GitHub

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

Jump to bottom

board: arduino-mega2560: updated arduino pin mapping #5194

Merged
merged 1 commit into from
Apr 1, 2016
Merged

board: arduino-mega2560: updated arduino pin mapping #5194

merged 1 commit into from
Apr 1, 2016

Conversation

pdNor
Copy link
Contributor

@pdNor pdNor commented Mar 29, 2016

The Arduino pins was not correctly mapped for the Arduino mega 2560, so i remapped them using this as reference. Pin are tested.

@OlegHahm OlegHahm added this to the Release 2016.04 milestone Mar 29, 2016
@OlegHahm OlegHahm added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Platform: AVR Platform: This PR/issue effects AVR-based platforms labels Mar 29, 2016
@kYc0o
Copy link
Contributor

kYc0o commented Mar 30, 2016

I think here there is a misunderstanding about this "pin mapping". This file was aiming to provide the GPIO regarding its number on the CPU, not its number on the Arduino layout.

Maybe the name arduino_pinmap.h should be changed to atmega2650_pinmap.h (ATMEGA2560_PIN_1 ...) and accept this PR as arduino_pinmap.h since all mappings correspond to the arduino layout.

@haukepetersen you told me something about this file that you wanted to change, is it?

Tested and works.

kYc0o
kYc0o reviewed Mar 30, 2016
View reviewed changes
boards/arduino-mega2560/include/arduino_pinmap.h
@@ -31,93 +32,78 @@ extern "C" {
*
* @note Some pins (e.g. 10, 11) are not mapped, as they are not usable as
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change this since the pins are actually used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do

@haukepetersen
Copy link
Contributor

nice catch! ACK once comments are addressed

@pdNor
Copy link
Contributor Author

pdNor commented Mar 30, 2016

Comment has been addressed.

@OlegHahm
Copy link
Member

You cannot ACK your own PR. ;-) The general procedure is that a maintainer reviews your PR and gives an ACK once he/she agrees that it is ready to be merged into master.

@OlegHahm
Copy link
Member

Also, please prefix the commit message with something like board arduino-mega2560:.

@pdNor
Copy link
Contributor Author

pdNor commented Mar 30, 2016

Sorry...learning by doing ;-)

@OlegHahm
Copy link
Member

No problem. We're here to help. :)

@OlegHahm OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Mar 30, 2016
@kaspar030 kaspar030 changed the title Uppdated pin mapping board: arduino-mega2560: updated arduino pin mapping Mar 30, 2016
kaspar030
kaspar030 reviewed Mar 30, 2016
View reviewed changes
boards/arduino-mega2560/include/arduino_pinmap.h
@@ -16,7 +16,7 @@
* You can use the defines in this file for simplified interaction with the
* Arduino specific pin numbers.
*
* @author Hauke Petersen <hauke.petersen@fu-berlin.de>
* @author Daniel Nordahl <nordahl.d@gmail.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually just add new authors, unless you basically replace the whole file. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad

@haukepetersen
Copy link
Contributor

@kYc0o sorry, overlooked your comment above: i think we had a missunderstanding: this file is meant to do what it's doing now, providing a mapping from the CPUs GPIO pins to the Ardunino pin numbers.

@pdNor: Could you remove this one newline and squash all you commits to a single one? Thx

@kYc0o
Copy link
Contributor

kYc0o commented Mar 31, 2016

Ok, anyways it was in the name. Thanks!

@pdNor
Copy link
Contributor Author

pdNor commented Mar 31, 2016

@haukepetersen: Ok, done.

@haukepetersen
Copy link
Contributor

sorry for being so picky - could you re-add the boards/arduino-mega: prefex to the commit message? Thanks. Otherwise ACK once this is addressed and the CI gives green light.

@miri64
Copy link
Member

miri64 commented Mar 31, 2016

In general the commit message should be cleaned up: prefix missing, summary contains typo, description part is not very descriptive (my recommendation either remove it or describe you motivation for the change and how you changed it).

@miri64 miri64 removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Mar 31, 2016
@miri64
Copy link
Member

miri64 commented Mar 31, 2016

(you can do that with git commit --amend in case you did not know)

@pdNor
Copy link
Contributor Author

pdNor commented Mar 31, 2016

Ok, I am sorry for not doing this right. But I am learning a lot so hopefully it will go a lot smoother next time around =)

@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 31, 2016
@miri64
Copy link
Member

miri64 commented Mar 31, 2016

No need to apologize :-) We all started at one point or another.

miri64
miri64 reviewed Mar 31, 2016
View reviewed changes
boards/arduino-mega2560/include/arduino_pinmap.h
@@ -29,95 +30,95 @@ extern "C" {
/**
* @brief Mapping of MCU pins to Arduino pins
*
* @note Some pins (e.g. 10, 11) are not mapped, as they are not usable as
* GPIO, as they are pins for voltage supply, reset, etc.
* @note ISCP pins are not mapped.
Copy link
Member

Choose a reason for hiding this comment

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

Please remove trailing whitespaces.

@cgundogan
Copy link
Member

nitpicky: You added the prefix as boards arduino-mega: instead of boards/arduino-mega:

@pdNor
Copy link
Contributor Author

pdNor commented Apr 1, 2016

fixed

@pdNor
boards/arduino-mega: Changes in Arduino pin mapping
c8f51e2 
The MCU pins was not correctly mapped to Arduino pins.
Updated the mapping so that MCU pins respons to the correct Arduino pins.
@kYc0o
Copy link
Contributor

kYc0o commented Apr 1, 2016

Murdock + comments addressed + ACK = GO!

@kYc0o kYc0o merged commit ceab9bd into RIOT-OS:master Apr 1, 2016
@haukepetersen
Copy link
Contributor

awesome, thanks for the fix and your patience :-)

@pdNor
Copy link
Contributor Author

pdNor commented Apr 1, 2016

I guess the patience thing goes both ways ;-)

@OlegHahm
Copy link
Member

OlegHahm commented Apr 1, 2016

Congrats and thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@kYc0o kYc0o

Labels
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: AVR Platform: This PR/issue effects AVR-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2016.04
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@pdNor @kYc0o @haukepetersen @OlegHahm @miri64 @cgundogan @kaspar030