Setting/changing DAC values doesn't really work

[0r31]

Hi,

Prerequisites

I have a board with a mcp4728.
I set DAC_MOTOR_CURRENT_DEFAULT to { 30, 30, 20, 30 }.
The M908 command works well and can be used as a workaround.

Setting DAC values at initialization

I manually set all dac channels to 0 to be sure values will be set from DAC_MOTOR_CURRENT_DEFAULT in StepperDAC::init() (line 55).

M908 P0 S0
M908 P1 S0
M908 P2 S0
M908 P3 S0
M909
_echo:Stepper current values in % (Amps):_
_echo: X:0.00 (0.00) Y:0.00 (0.00) Z:0.00 (0.00) E:0.00 (0.00)ok_
M910

After resetting the board, i check the dac values.

M909
echo:Stepper current values in % (Amps):
echo: X:0.00 (0.00) Y:0.00 (0.00) Z:0.00 (0.00) E:0.00 (0.00)ok

Dac values are still equals to 0.

Setting DAC values from screen

I manually set all dac channels to any non zero values.

M908 P0 S1200
M908 P1 S1200
M908 P2 S800
M908 P3 S1200
M909
echo:Stepper current values in % (Amps):
echo: X:29.30 (-13963.64) Y:29.30 (-13963.64) Z:19.53 (18618.18) E:29.30 (-13963.64)ok
M910

Then, i check if the screen shows the same values.

Then, i changed the x driver power from screen.

The value is not changed.

Explanation

In setDrvPct(), the multiplication operator is used.
If values are 0, then multiplying by 0 leads to 0.
If values are different from 0, the goal is to set a percentage of the max power, not a percentage of the previous percentage.

So we must use the assignment operator and not the multiplication assignment one.
The following line

dac_values *= pct.asFloat() * 0.01f * (DAC_STEPPER_MAX);

must be replaced by

dac_values = pct.asFloat() * 0.01f * (DAC_STEPPER_MAX);

By doing this, values are really changed but it still doesn't work.
Indeed, manually setting all dac values to 0 to force DAC_MOTOR_CURRENT_DEFAULT usage at initialization leads to weird values.

M909
echo:Stepper current values in % (Amps):
echo: X:0.02 (0.00) Y:0.02 (0.00) Z:0.02 (0.00) E:0.02 (0.00)ok

Configuration change

If we change the DAC configuration (ie. change any DAC_MOTOR_CURRENT_DEFAULT value), it is not handled by the current code of StepperDAC::init().

Solutions

About initialization

The quik and dirty way i found to make it work is to use LOOP_XYZE.

LOOP_XYZE(i) dac_values[i] = pct[i] * 0.01f * (DAC_STEPPER_MAX);

The root cause is there are some types discrepancies and/or missing struct operators.

Example : dac_values is type of xyze_uint_t whereas pct is type of xyze_uint8_t

4 solutions :

• Avoid using structs operators and replace them by LOOP_XYZE
• Align types (e.g. declare pct as xyze_uint_t)
• Add a asUnsignedInt() method in structs defined in types.h
• Add an assignment operator with a XYZEval<float> or XYZEval<uint8_t> typed argument in structs defined in types.h

Do you have any preference?

About configuration change

I guess the condition setting DAC values in StepperDAC::init() doesn't fit the needs.
We should compare current DAC values and DAC_MOTOR_CURRENT_DEFAULT values.

It means replacing

if (mcp4728.getDrvPct(0) < 1 || mcp4728.getDrvPct(1) < 1 || mcp4728.getDrvPct(2) < 1 || mcp4728.getDrvPct(3) < 1 ) {
    mcp4728.setDrvPct(dac_channel_pct);
    mcp4728.eepromWrite();
}

by

// Check current values against config ones and reset them if needed
if (ABS(dac_channel_pct[0] - mcp4728.getDrvPct(0)) > 1 \
|| ABS(dac_channel_pct[1] - mcp4728.getDrvPct(1)) > 1 \
|| ABS(dac_channel_pct[2] - mcp4728.getDrvPct(2)) > 1 \
|| ABS(dac_channel_pct[3] - mcp4728.getDrvPct(3)) > 1 ) {
  mcp4728.setDrvPct(dac_channel_pct);
  mcp4728.eepromWrite();
}

[boelle]

have you tested your suggestion and are they working? if yes then maybe submit a PR

[0r31]

Hi @boelle ,

Yes i tested and code is ready. I just ask for a decision for the right solution to choose regarding setDrvPct() as i see 4 different ones.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.