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

add support for Microduino SD #4428

Closed
wants to merge 1 commit into from
Closed

Conversation

JasonSheng
Copy link
Contributor

No description provided.

@JasonSheng JasonSheng closed this Jan 15, 2016
@JasonSheng JasonSheng reopened this Jan 15, 2016
@JasonSheng
Copy link
Contributor Author

Add enhance for SD pin layout to supprot the microduino board. Just like board that listed in this Sd2PinMap.h, such as Teensy, Sanguino.

@matthijskooijman
Copy link
Collaborator

The commit looks good by itself, though I cannot verify the actual pinouts.

However, I'm wondering if all this pin mapping code is really needed at all? Nowadays, most variants' pins_arduino.h files expose constants for special pins (like MOSI, MISO, etc.). Also, the pin mappings are available through macros (https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/Arduino.h#L169-L175). I haven't looked at the code how these things are used, though.

@JasonSheng
Copy link
Contributor Author

Actually we have tested it. And it only can be used if the Macro is true, so it won't impact the Arduino IDE.

@JasonSheng
Copy link
Contributor Author

BTW, the SD library is very special, and can't change your listed file to support new pinout, so have to change the file "Sd2PinMap.h", this is the only method. Hope you can accept this patch.

@cmaglie
Copy link
Member

cmaglie commented Jan 18, 2016

Also, the pin mappings are available through macros (https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/Arduino.h#L169-L175). I haven't looked at the code how these things are used, though.

there is this PR that did it already: #121

@JasonSheng may I ask you give it a try with the Microduino? (it does need some minor edits to merge cleanly, but you should be able to use the file SD2PinMap.h from here https://github.com/arduino/Arduino/pull/121/files)

@cmaglie cmaglie added feature request A request to make an enhancement (not a bug fix) Library: SD The SD Arduino library labels Jan 18, 2016
@JasonSheng
Copy link
Contributor Author

@cmaglie , I checked the file Sd2PinMap.h, it didn't use PR 121's change. I am not sure the PR #121 has been accepted. Could you please double check it? If use PR #121, our PR no need again.

The PR #121 was submitted in 2012, that directory is /libraries/SD/utility/Sd2PinMap.h, so far the directory has changed to /libraries/SD/src/utility/Sd2PinMap.h.

@cmaglie
Copy link
Member

cmaglie commented Jan 18, 2016

I am not sure the PR #121 has been accepted.

it wasn't, my request is if you can check if it's working for you so we can move it forward :-)

@JasonSheng
Copy link
Contributor Author

@cmaglie , thanks for your suggestion. I tested the PR#121, it can work perfectly with Microduino and no more impact to Arduino board. That is a very good PR. Could you consider to accept this PR #121 in official repor? Thansk in advance.

@JasonSheng
Copy link
Contributor Author

@cmaglie , @matthijskooijman , I tested the PR#121, works well, after this patch, the user can change the pin layout easily. It is a wonderful PR. Could you consider how to move on?

@JasonSheng
Copy link
Contributor Author

@matthijskooijman , @cmaglie , what's your opinion for this new patch, We have verified it, no issue found.

@matthijskooijman
Copy link
Collaborator

@JasonSheng, I'm for merging #121, but I expect it is now on @cmaglie's stack, waiting for him to find time to test and merge it.

@cmaglie
Copy link
Member

cmaglie commented Jan 25, 2016

Hi @JasonSheng,

yes we are working on this one, #121 will get merged very soon!

@cmaglie cmaglie self-assigned this Jan 25, 2016
@cmaglie cmaglie added this to the Release 1.6.8 milestone Jan 25, 2016
@cmaglie
Copy link
Member

cmaglie commented Jan 25, 2016

#121 has been merged.

@cmaglie cmaglie closed this Jan 25, 2016
@JasonSheng
Copy link
Contributor Author

@cmaglie , @matthijskooijman thanks a lot.. we weill try our best to popularize the Arduino...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request A request to make an enhancement (not a bug fix) Library: SD The SD Arduino library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants