-
-
Notifications
You must be signed in to change notification settings - Fork 19.2k
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
[1.1.x] Update pin definitions for PRINTRBOARD REV F. #7286
[1.1.x] Update pin definitions for PRINTRBOARD REV F. #7286
Conversation
be13028
to
703531f
Compare
Marlin/pins_PRINTRBOARD_REVF.h
Outdated
// reassign different functions to EXP1. | ||
|
||
// Define NO_EXTRUDRBOARD_OUTPUT_SWAP if you have a REV F5 or lower and | ||
// want to use EXTRUDRBOARD A for E1 and EXTRUDRBOARD B for E2. |
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.
Please provide the defines, ready to be used:
// Define NO_EXTRUDRBOARD if you don't have an EXTRUDRBOARD and wish to
// reassign different functions to EXP1.
//#define NO_EXTRUDRBOARD
// Define NO_EXTRUDRBOARD_OUTPUT_SWAP if you have a REV F5 or lower and
// want to use EXTRUDRBOARD A for E1 and EXTRUDRBOARD B for E2.
//#define NO_EXTRUDRBOARD_OUTPUT_SWAP
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.
Wouldn't those #defines belong in your Configuration.h
? I tried to avoid any need for an individual builder to edit pins_PRINTERBOARD_REVF.h
.
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.
I suggest keeping them in the pins file, since they pertain to no other board.
Marlin/pins_PRINTRBOARD_REVF.h
Outdated
#define HEATER_BED_PIN 14 // C4 PWM3C | ||
|
||
#define FAN_PIN 16 // C6 PWM3A | ||
#define PRINTRBOARD_F6_HEATER_FAN_PIN 44 // pin F6 on REV F6, see above. | ||
|
||
#ifndef NO_EXTRUDRBOARD |
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.
For compatibility with Makefile and other build methods, these should be:
#if DISABLED(NO_EXTRUDRBOARD)
#if DISABLED(NO_EXTRUDRBOARD_OUTPUT_SWAP)
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.
Will do.
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.
You may want to double check these pin assignments.
Has anyone tested these pin assignments on a Printrboard Rev F with an Extrudrboard connected to Exp1? I have this configuration set up and the my pin assignments do not match the above configuration.
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.
I am currently running this on my REV F5 with a stock extrudrboard and I've
checked this against the schematic for the REV F4 and REV F6 (available at
https://github.com/Printrbot/printrboard ). I'm guessing you're patching Printrbot's fork, which still uses the old fastio pin numbering and that's why these don't match your configuration. This is correct arduino numbering.
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.
I just checked and the pins I was using were fastio.
Marlin/pins_PRINTRBOARD_REVF.h
Outdated
@@ -24,14 +24,17 @@ | |||
* Rev B 2 JUN 2017 | |||
* | |||
* Converted to Arduino pin numbering | |||
* at90usb1286 to arduino pin mapping is here: | |||
* https://labitat.dk/wiki/Panelolu_and_Printrboard_the_easy_way |
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.
Marlin no longer uses the so-called "Marlin mapping" or "fastio mapping" which numbers A0-A7 as 0-7, B0-B7 as 8-15, etc. We have killed that mapping. Thus the numbers marked in green on the linked page do not apply.
Marlin has switched to a unified Arduino-style pin mapping under fastio.h
, including for all AT90USB processors. All Marlin code (with few exceptions) uses FastIO macros —such as READ
, WRITE
, etc.— allowing us to follow this mapping consistently.
Nor do we use the mapping provided by the Teensyduino headers. It's Arduino-style mapping all the way, in which the pin's "DIO number" is used as the pin number (except, 46 is pin E2 and 47 is pin E3.)
With that in mind, please make any necessary changes.
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.
Yes, the linked page contains some additional pin numbers. But it also contains the arduino pin numbers, and that's what this comment explicitly references. I don't know why you are continuing to object to "fastio" when that doesn't appear anywhere in the patch.
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 problem is there is some misinformation at that link, which is outdated and has shown to cause mounds of issues in the past. If history hadn't proven this over and over again (that link you found just one example of dozens online, and in this tracker), it wouldn't be such a stickling point.
If I have some time I may update the ASCII version of a similar chart that PrintrbotCo has at the top of their fork's pins.h file and incorporate it (or you can here !), but for now it is surely best to omit links that cite the old pins numbering in any form.
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.
Correct. My comment is mainly due to the link being outdated and containing information that no longer pertains to Marlin. We have a website now, so if someone just throws together an update in Markdown format (e.g., a comment on Github will do), we may want to document this at marlinfw.org.
@cscott Some comments for your consideration…… |
Sorry I haven't had time to try your build on my RevF - will try to soon ! |
Works over here @cscott ! |
Marlin/pins_PRINTRBOARD_REVF.h
Outdated
#ifndef USBCON | ||
#error "USBCON should be defined by the platform for this board." | ||
#endif | ||
|
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.
I asked about this previously only because of issues around this historically with various instabilities surrounding USBCON (see #6550, #6737, #4171, #2433, etc)
So it sounds like it compiled correctly without this because the Teensyduino extension must define USBCON when the correct board is selected in the AIDE ? If so, defining again (in case user is not using Teensyduino) certainly sounds correct. If not, it begs the question how this is currently working without it.
Thanks,
-=dave
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.
Concerns over the existence of USBCON
were resolved with #6737 (comment) and #6889.
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.
I just tested compilation of the other boards (with SD and ABL enabled but no LCD): TEENSYLU, TEENSY2, SAV_MKI, PRINTRBOARD, BRAINWAVE_PRO, 5DPRINT, (and of course PRINTRBOARD_REVF) with USBCON define removed from respective pins_[board].h and all compiled w/o issue.
It seems removal of USBCON define from all pins_[board].h would be appropriate, allowing serial.h to pull it in as appropriate from Arduino.h ?
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.
And just tested BRAINWAVE (AT90USB647) with @Bob-the-Kuhn 's Marlin extension and it too compiled as expected w/o USBCON defined in the board pins file (of course the resulting sketch wouldn't fit, but i'm sure it could be made to).
Submitted #7347 for this to save you a few seconds in case this way you want to go Scott. If this isn't correct, just reject.
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.
Did you find that the compilation size was identical with the USBCON
defines removed?
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.
Didn't check before, but just tested PRINTRBOARD with SD, Viki2, 3-PT (essentially stock config) and it came out identical.
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.
...and just checked BRAINWAVE (only 647 board and uses Bob's custom extension) and also identical 👍
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.
So the consensus is to remove the #ifndef
check and #error
here then?
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.
Yeah. USBCON defines for all other AT90USB boards have already been removed.
Sorry, haven't had time to amend and retest the patch yet. Will do soon. |
As you find the time. I had to put off the 1.1.5 release for some additional patches, so if this is ready in the next day or two it may get into that release just under the wire. |
If this PR is in good shape, then we should get it merged, and also apply these changes to |
@thinkyhead - i have a bunch of changes to make to pb pins (and teensy group pins) to fix a grip of compile warnings that make Sublime Text 3/DevIoT PIO compiles take about 5x longer due to overhead of catting the never-ending stream of warnings to screen, but forgot about this PR. I can incorporate this PR into mine, or you can merge this now, and then I'll rebase mine over the top. Whichever you prefer. Let me know. -=dave |
We're now into the weeds with 2.0.x, so I'll need to port this over to 2.0.x at the same time this gets merged. Will take care of that tomorrow…. |
bb58d68
to
524f4ee
Compare
Just sent up all my pending RevF changes, so I'll look at putting together PRs for this one sometime in the next month or so, since it appears somewhat abandoned. |
Anytime is good, Dave. Assuming no major issues, I hope to put out version 1.1.7 some time in the next 24 hours. If you're too busy to do this one, I'll try to squeak it in for the release. |
Marlin/pins_PRINTRBOARD_REVF.h
Outdated
#define TEMP_2_PIN TEMP_A_PIN | ||
#define HEATER_2_PIN HEATER_A_PIN | ||
|
||
#endif // NO_EXTRUDRBOARD_OUTPUT_SWAP |
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.
I get the general reasoning for doing it with intermediate named pins, such as for easier re-assignment, but really this is not needed. The pins file, once set, doesn't change, and if users need to edit the pins file for reassignment I'm sure they can figure out which of these blocks applies.
703531f
to
f7e8497
Compare
I've got this one. Rebased, squashed, cleaned up, force-pushed. |
7e5ecf5
to
d65c99f
Compare
Allow use of Extrudrboard and REV F6. Disable JTAG to allow additional
pins to be used, and allow swapping channel A and B of the Extrudrboard
to accomodate REV F6.