Skip to content

Makerdiary boards: add missing pin aliases, and canonicalize some others #3438

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

Merged
merged 4 commits into from
Sep 19, 2020
Merged

Conversation

nitz
Copy link

@nitz nitz commented Sep 18, 2020

Despite the silk on the dock board, the SDA/SCL pins weren't defined. Though, they were already defined in mpconfigboard.h.

Same for RX/TX. It looks like it declared TXD and RXD, so I didn't want to remove those, but I think it makes sense to have the "standard" pin names, but I moved ithem to illustrate they were all referencing the same pins.

I mimicked the whitespace I saw in the metro_nrf52840_express port.

Despite the [silk on the dock board](https://wiki.makerdiary.com/nrf52840-m2-devkit/resources/nrf52840_m2_devkit_hw_diagram_v1_0.pdf), the SDA/SCL pins weren't defined. Though, they were already defined in `mpconfigboard.h`.

Same for RX/TX. It looks like it declared `TXD` and `RXD`, so I didn't want to remove those, but I think it makes sense to have the "standard" pin names, but I moved ithem to illustrate they were all referencing the same pins.

I mimicked the whitespace I saw in the metro_nrf52840_express port.
Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

The build checks for trailing whitespace. There is trailing whitespace on lines 64, 68, 89, and 94 (at least).

It would be OK to remove the TXD and RXD names in favor of TX and RX, especially if this is a recently added board.

@nitz
Copy link
Author

nitz commented Sep 18, 2020

Doh, that's what I get for trying to just do a quick edit from the github website. I pulled the branch locally and was looking at every line, losing my mind... because an addon cleaned up the whitespace for me. 🙃

Re the TXD/RXD: I feel similarly. I wasn't the one who added this board, and I didn't want to break anyone's things, but I did take a look at the official examples, and none of them are using those pin names, so I'll remove them!

@dhalbert
Copy link
Collaborator

Could you also change the TXD and RXD in the other makerdiary boards to TX and RX? Or does the silkscreen say "TXD" and "RXD", in which case I'm not sure what to do. These are the only boards that use that "D" suffix,

@dhalbert
Copy link
Collaborator

OK, I looked at the silks, and the Arduino-shaped M.2 devkit uses TX and RX on the board, so that change makes sense. The other boards seem to use TXD and RXD fairly consistently, so we can leave them alone. So the changes you made so far are fine.

There seem to be other labels on some boards that are not included, but we don't have to fix those here.

@nitz
Copy link
Author

nitz commented Sep 18, 2020

So, on the Pitaya Go (pitaya_go), they list them as TXD/RXD in their pinout, but just the PXX on the silk. the MDK, they list TXD/RXD in the pinout, as well as on the silk. Same goes for the M60 keyboard.. The MDK Dongle Seems to skip them entirely.

@nitz
Copy link
Author

nitz commented Sep 18, 2020

And I should check for comments before replying!

I'll make a list of pins that seem to be missing tomorrow and will make a sweep and see if I can get them all.

@dhalbert
Copy link
Collaborator

Thanks for looking. We generally try to use the silk markings as "ground truth", and then add aliases as appropriate for convenience.

@nitz
Copy link
Author

nitz commented Sep 18, 2020

Okay it looks like most of the other boards matched their silk. I actually found a typo in the Pitaya Go silk, so I made them an issue for that.

So on the user button for the M.2 Devkit, the silk says "USR->D2", because it's tied to D2 as well as that button. The pin name in CP was named "USR_BTN", which makes sense, but doesn't match the "USR" on the board. Opinions there?

The changes I made to the MDK USB Dongle was to comment in the alt names for the NFC pins. They were already commented in in the Pitaya Go, and were just out there because of CP's lack of support for NFC... but that's something I'm also tackling now in #3379, so it makes sense to at least have the Pitaya Go/MDK USB Dongle at least have parity in that sense.

@dhalbert
Copy link
Collaborator

The pin name in CP was named "USR_BTN", which makes sense, but doesn't match the "USR" on the board. Opinions there?

We usually call buttons BUTTON_A, BUTTON_UP, and the like, so BUTTON_USR is would be consistent with other boards.

We are having trouble with the automated builds on GitHub getting swamped and stalled for some reason, so I will be canceling the runs for your pushes for a while to let other PR's finish, and then when this is ready to test and merge I will re-run manually.

@nitz
Copy link
Author

nitz commented Sep 18, 2020

No worries about the cancels, makes perfect sense!

Okay, I did BUTTON_USR where it made sense (either having USR on the board, or if the button was unlabeled,) so that way it will have consistency among that family.

Any others to take a look at?

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Thank you for the original work, and your patience and diligence. 👍

@dhalbert dhalbert merged commit b28b311 into adafruit:main Sep 19, 2020
@dhalbert dhalbert changed the title [Makerdiary nRF52840 M.2 Devkit] Add SDA & SDL, RX & TX to pins.c Makerdiary boards: add missing pin names, and canonicalize some others Sep 19, 2020
@dhalbert dhalbert changed the title Makerdiary boards: add missing pin names, and canonicalize some others Makerdiary boards: add missing pin aliases, and canonicalize some others Sep 19, 2020
@nitz nitz deleted the patch-1 branch March 10, 2021 20:22
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.

2 participants