-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
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.
There was a problem hiding this 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.
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! |
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, |
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. |
So, on the Pitaya Go ( |
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. |
Thanks for looking. We generally try to use the silk markings as "ground truth", and then add aliases as appropriate for convenience. |
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. |
We usually call buttons 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. |
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? |
There was a problem hiding this 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. 👍
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
andRXD
, 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.