MCP4728 consistency & fix ultralcd.cpp (PRINTRBOARD Rev F and RIGIDBOARD V2)#6124
MCP4728 consistency & fix ultralcd.cpp (PRINTRBOARD Rev F and RIGIDBOARD V2)#6124Bob-the-Kuhn merged 1 commit intoMarlinFirmware:RCBugFixfrom Bob-the-Kuhn:MCP4728-consistency
Conversation
|
Duplicating comment on PR directly:
|
|
Thinking more, if this band-aid is what we're going with, why not just call a spade a spade and name them according to the specific implementation method: DIGIPOT_MOTOR_CURRENT -> MOTOR_CURRENT_AD5206 (0-100%) Would need corresponding warnings in Sanitycheck to alert users to change their Config*.h's and would be one babystep toward getting people warmed up to a more elegant/understandable long-term solution for later ? Alternately, if @thinkyhead has other plans to address this in 1.2, your PR is prob fine for now, but I know that I did not understand what method I needed to use when I was setting up my Printrboard for the first time, so I just kept trying different ones until it worked, which was tedious. It would be nice to prevent others from having to do the same trial and error (which listing the DAC model might help with ?) |
|
Supporting legacy boards isn't fun. It would be nice to have a single all inclusive routine but ... The biggest problems are:
Putting the known chips in the comments is a good idea. Changing the names could confuse current users so they'll stay as is. How about this? /**
* @section stepper motor current
*
* Some boards have a means of setting the stepper motor current via firmware.
*
* The power on motor currents are set by:
* PWM_MOTOR_CURRENT - used by MINIRAMBO & ULTIMAIN_2
* known compatible chips: A4982
* DIGIPOT_MOTOR_CURRENT - used by A4JP, BQ_ZUM_MEGA_3D & RAMBO
* known compatible chips: AD5206
* DAC_STEPPER_DFLT - used by PRINTRBOARD_REVF & RIGIDBOARD_V2
* known compatible chips: MCP4728
* DIGIPOT_I2C_MOTOR_CURRENTS - used by 5DPRINT & AZTEEG_X3_PRO
* known compatible chips: MCP4451
*
* Motor currents can also be set by M907 - M910 and by the LCD.
* M907 - applies to all.
* M908 - A4JP, BQ_ZUM_MEGA_3D, RAMBO, PRINTRBOARD_REVF & RIGIDBOARD_V2
* M909, M910 & LCD - only PRINTRBOARD_REVF & RIGIDBOARD_V2
*/
//#define PWM_MOTOR_CURRENT {1300, 1300, 1250} // Values in milliamps
//#define DIGIPOT_MOTOR_CURRENT {135,135,135,135,135} // Values 0-255 (RAMBO 135 = ~0.75A, 185 = ~1A)
//#define DAC_STEPPER_DFLT { 70, 80, 90, 80 } // Default drive percent - X, Y, Z, E axis
/* 5DPRINT & AZTEEG_X3_PRO */
//#define DIGIPOT_I2C // uncomment to enable
#define DIGIPOT_I2C_NUM_CHANNELS 8 // 5DPRINT: 4 AZTEEG_X3_PRO: 8
/* actual motor currents in Amps, need as many here as DIGIPOT_I2C_NUM_CHANNELS */
#define DIGIPOT_I2C_MOTOR_CURRENTS {1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0} // AZTEEG_X3_PRO |
|
The comments are helpful — please add them. Changing the names according to Dave's suggestion seems like a good plan for 1.2 — but better not to change the whole group of them at this point in time. The one thing good about keeping the names different from one another is that it emphasizes that they all use completely different units. |
Marlin/pins_RIGIDBOARD_V2.h
Outdated
There was a problem hiding this comment.
The DAC_STEPPER_DFLT setting is recent, and never meant to be a user-facing setting, which is why it has an uglier-than-usual name. So I recommend also renaming this setting in addition to moving it to Configuration_adv.h. Either use the suggested name, MOTOR_CURRENT_MCP4728, or just make it more readable, as in DAC_STEPPER_DEFAULT. Whatever seems more forward-thinking, I guess….
Probably there's no need to leave a breadcrumb trail. Just remove it from here.
thinkyhead
left a comment
thinkyhead
left a comment
There was a problem hiding this comment.
Just one typo found in need of a patch.
There was a problem hiding this comment.
This comment line got broken. There may be other instances. Searching \n\n=== will probably turn them up.
|
Implemented all the requested changes & squashed.
ultralcd.cpp is back on this PR even though the latest RCBugFix already has this change changed name from A4JP to SCOOVO_X9H per PR #6139 |
There was a problem hiding this comment.
I see a problem here… Ordinarily I'd patch this myself, but your turnaround is quick!
|
Typo fixed, rebased & squashed. |
# This is the 1st commit message: MCP4728 consistency & fix ultralcd.cpp The MCP4728 DAC controls the stepper motor current strenth on the PRINTRBOARD Rev F and RIGIDBOARD V2 boards. PR #5792 on 9 FEB 2017 implemented default drive percentages but only on the RIGIDBOARD V2. This change moves the default settings to Configuration_adv.h. Also, ultralcd.cpp won't compile because of a type def conflict. Changed it to match the one in stepper_dac.cpp =========================================================== reword stepper curent section for clarity =========================================================== change name & improve comments =========================================================== changed name from A4JP to SCOOVO_X9H per PR #6139 # This is the commit message #2: fix typo
|
Bob- Do you think current RCBF in a state ready to test on a Printrboard (are most other things still working, like basics ABL ?) |
|
Yes, Printrboard is fully supported by the current RCBugFix. The older automated bed leveling schemes have not changed for a while. UBL is working fairly well. There's a few things that aren't implemented yet (like the G option). |
|
I'll kick the tires this weekend to make sure all this Printrboard changes work as designed without hand tweaking outside of the expected places. |
3 participants
The MCP4728 DAC controls the stepper motor current strenth on the PRINTRBOARD Rev F and RIGIDBOARD V2 boards.
PR #5792 on 9 FEB 2017 implemented default drive percentages but only on the RIGIDBOARD V2.
This change moves the default settings to Configuration_adv.h.
Also, ultralcd.cpp won't compile for these boards because of a type def conflict. Changed it to match the one in stepper_dac.cpp