grblHAL / core

grblHAL core code and master Wiki
Other
305 stars 74 forks source link

Ammend description for $9 settings #442

Closed SienciLabs closed 4 months ago

SienciLabs commented 5 months ago

Setting currently claims to enable spindle signal past S=0, but is actually dependent on what value is set for Minimum Spindle Speed in EEPROM

SienciLabs commented 5 months ago

Found during testing for our SuperLongBoard, not sure if the behaviour is correct and the setting needs to be updated, or the behaviour is a bug and the setting is meant to stay the same, but figured I'd put in a PR for it anyway since it doesn't hurt.

Thanks Terje! -Chris

terjeio commented 5 months ago

It is a bug in the core: S > 0 and S < min RPM should turn on the spindle at min RPM. The real-time report correctly report min RPM.

terjeio commented 5 months ago

This is a little bit tricky to fix. Legacy Grbl uses PWM generator with 255 steps of resolution and by default set the minimum value to 1 (about half a percent). And the PMW frequency is fixed, IIRC @ 1KHz. grblHAL uses one with minimum 65535 steps and allows changing the PWM frequency. And some drivers supports auto-tuning of the PWM peripheral by setting a clock prescaler to achieve the optimal resolution. E.g. the range for my STM42F4xx Morpo board @ 5 Khz is 0-36000. If I implement the fix by setting the minimum value to 1 like legacy Grbl does it will likely produce no PWM output at all as it will be a pulse witdh of about 6 nS. At least it will turn on the spindle enable signal... So the choice is to set the minimum value to 1 or to set it to about half a percent of the max like legacy Grbl does. I favour setting it to 1, or perhaps a little bit more - 5?. This since when setting minimum RPM > 0, $35 (minimum PWM value) should also be set > 0 to get some relevant signal on the PWM pin?

What do you think?

SienciLabs commented 4 months ago

Ohh, ok I see then what the situation is causing. So firstly, that means that the existing description is actually correct but the behaviour is currently not. If that means that there needs to be a minimum value sent to power on the Spin Enable as well as the PWM, then perhaps half a percent or quarter percent would be good? If you can set it to 1 and it can still work for 60000+ steps, then in that case I would use 1 since that would give a speed closest to the minimum spindle speed 👍

terjeio commented 4 months ago

in that case I would use 1 since that would give a speed closest to the minimum spindle speed

Not neccesarily...

I think I'll go for the percentage - and add a note to the description that $35 (min PWM) should be set so that the spindle spins with the minimum RPM specified. I do not know how VFDs are behaving with 0V at the PWM output, my mini router that has a DC motor do not turn at all so I have to adjust $35 to get the minimum RPM specified by $31.

SienciLabs commented 4 months ago

Ok, glad to have helped contribute in some way. If it's ok I'll close the pull request since it's not the right change, thanks Terje

terjeio commented 4 months ago

It think it is ok now, thanks for reporting.

Change these lines to:

        pwm_data->max_value = (uint_fast16_t)(pwm_data->period * settings->pwm_max_value / 100.0f) + pwm_data->offset;
        if((pwm_data->min_value = (uint_fast16_t)(pwm_data->period * settings->pwm_min_value / 100.0f)) == 0 && spindle->rpm_min > 0.0f)
            pwm_data->min_value = (uint_fast16_t)((float)pwm_data->max_value * 0.004f);

if you want to try the fix before I commit the changes.