Unclear interval in M42

Otso

Yes, this can be a pretty dangerous bug. I’m using firmware 3.4 and haven’t checked if it has been fixed.

I just tested with my laser and M42 S1 (no fraction) sets it at full power and not at 1/255 as one would expect from the documentation.

Either the code or the documentation needs to be fixed.

Personally I think this should be fixed in the code, especially if it is for backwards compatibility, where one can assume that “S1” previously set it at 1/255 of the power and since the introduction of fractions that changed it to full power.

With the firmware 3.4 implementation the documentation should say:

The S field may be in the range 0..1 or 2..255.

It should be noted that apparently values like 5.76 will probably also work, and be converted to 5.76/255. So the documentation could clarify that fractions are accepted, and the correct documentation would actually be:

The S field may be in the range [0,1] or ]1,255].

With further noting that the notation for the 1..255 range means any fractional value above, but not including, 1.

fcwilt

@Otso

As a programmer with 30+ years of experience I am never surprised to see code like that.

Yes, in practice it works. After all how many times are folks going to specify a PWM value of 1 out of the range 0 to 255.

But it does mean the documentation is wrong and unclear. If 1 is converted to 1.0 then the range is not 0 to 255 but 2 to 255.

If the other range is 0.0 to 1.0 then accepting a value of 1 as meaning 1.0 is just... something. Maybe they were short of code space and had to conserve.

But as a disclaimer I am somewhat OCD and my code reflects that - I do not like ambiguity.

The code could start with a test of the incoming value to determine if it was a float or fixed. Often programming languages have procedures like isFloat or isTime and many others to be used to test a value. Moving on, then you would either force the float value to be in the range 0.0 to 1.0 or force the fixed value to be in the range 0 to 255. Unless you wish to flag out of range values as errors which could help detect typos/bugs by the end user.

Like most things in life there are compromises to be faced.

Question: where are you seeing this 0 to 254?

Thanks.

Frederick

Otso

@fcwilt said in Unclear interval in M42:

Question: where are you seeing this 0 to 254?

https://docs.duet3d.com/User_manual/Reference/Gcodes G1 documentation says:

RRF 3, G0/G1 S parameter AFTER M452 Laser Mode.
S parameter sets laser power with range of 0 to 254.

gloomyandy

@Otso I've fixed the link to the code used for G1 S it is here: https://github.com/Duet3D/RepRapFirmware/blob/47435151e110f373fc80721a2c265e38b8ec5e3b/src/GCodes/GCodes.cpp#L4770

gloomyandy

@gloomyandy Oh and in case you are wondering laserMaxPower is set of M452 and defaults to 255.0

Otso

@gloomyandy said in Unclear interval in M42:

@Otso I've fixed the link to the code used for G1 S it is here: https://github.com/Duet3D/RepRapFirmware/blob/47435151e110f373fc80721a2c265e38b8ec5e3b/src/GCodes/GCodes.cpp#L4770

Thanks! I think I need to get to my computer to investigate the code more to figure where laserMaxPower is set, and what value it has in my case.

Pwm_t GCodes::ConvertLaserPwm(float reqVal) const noexcept
{
	return (uint16_t)constrain<long>(lrintf((reqVal * 65535)/laserMaxPower), 0, 65535);
}

Edit: Thanks for giving the info about laserMaxPower.

Otso

This post is deleted!

Otso

@fcwilt said in Unclear interval in M42:

Yes, in practice it works. After all how many times are folks going to specify a PWM value of 1 out of the range 0 to 255.

For example, if I use an application that converts a gray scale image to laser power, then some light pixels could map to 1 and cause the laser to blast out full power.

Or if you manually enable the laser at a very low level to set the laser focus or to line up the zero point. (I've been using S0.0001, but I could just as well have started with 1 and expecting it to be low power). Of course, you would notice the problem immediately and shut it down, but still, I don't like this I think it should be fixed.

gloomyandy

@Otso If you are converting from grey scale you should really be using G1 S, which I don't think has any of these issues.

gloomyandy

@Otso How do you propose fixing it without causing other problems with existing gcode/slicers/etc?

Otso

@gloomyandy said in Unclear interval in M42:

@gloomyandy Oh and in case you are wondering laserMaxPower is set of M452 and defaults to 255.0

Looking at the code for M452:

if (gb.Seen('R'))
{
    laserMaxPower = max<float>(1.0, gb.GetFValue());
}

I haven't tested, but based on that it seems like R is expected to be in the range 0..1 and defaults 1.0, which makes the formula for ConvertLaserPwm() make sense. If that's correct, the manual on M452 is wrong or misleading:

Rnnn The value of the S parameter in G1 commands that corresponds to full laser power, default 255

gloomyandy

@Otso I'm pretty sure the default is 255 (so the documentation is correct). If you specify a value with R it will be in the range 1 to whatever value you choose, that max function returns that largest value of 1 or your value, so in this case 1 is the minimum value you can set.

gloomyandy

@gloomyandy Going back to the problem with M42, remember it is used to control things other than laser output (it sets a general pwm level). I suppose a possible fix might be to add a parameter to the M950 used to create the pin and allow that to specify what the range is that will be used by M42. At least that's the best I can come up with that would probably still allow most things to work as they do today.

Otso

@gloomyandy said in Unclear interval in M42:

@Otso I'm pretty sure the default is 255 (so the documentation is correct). If you specify a value with R it will be in the range 1 to whatever value you choose, that max function returns that largest value of 1 or your value, so in this case 1 is the minimum value you can set.

Yes, you're totally correct, and so is the documentation. I wasn't thinking clearly.

Otso

@gloomyandy said in Unclear interval in M42:

@gloomyandy Going back to the problem with M42, remember it is used to control things other than laser output (it sets a general pwm level). I suppose a possible fix might be to add a parameter to the M950 used to create the pin and allow that to specify what the range is that will be used by M42. At least that's the best I can come up with that would probably still allow most things to work as they do today.

I'd expect it to work like @fcwilt assumed it worked (and I did too), and that the code would check if it is "1" or "1.", and make the decision based on that. I think that wouldn't break anything, and would retain best backwards compatibility because that was how "1" was interpreted before fractions were introduced, and I'd assume those using fractions now will use 1.0 for full width pulse.

The quick and simple fix would be to mention this is the manual, just as a caution if nothing else.

Otso

@gloomyandy said in Unclear interval in M42:

@Otso If you are converting from grey scale you should really be using G1 S, which I don't think has any of these issues.

You're right. I wrongly assumed the same issue would be with G1 S1. But that's not the case. The only potential issue if the other one I mentioned, but that would be less of a problem in practice if the operator takes proper care with the assumption that it can blast at full power regardless of what commands you feed it (by always wearing eye protection etc.) as one should.

So yes, I agree, not a big issue.

Otso

@gloomyandy said in Unclear interval in M42:

@Otso How do you propose fixing it without causing other problems with existing gcode/slicers/etc?

Just clarifying the documentation would be sufficient for me.

But like I said above, I think it already introduced potential problems when the meaning of value of "1" was changed with the introduction of fractions. But again, I'm not asking for a code change, just a documentation fix.

Otso

And the G1 documentation should be updated to say that the max value for S is defined by M452, which defaults to 255.

Otso

@gloomyandy said in Unclear interval in M42:

@Otso How do you propose fixing it without causing other problems with existing gcode/slicers/etc?

My snarky reply would be that you have the existing programs update accordingly, like I had to do in multiple places when M3 didn’t turn on the laser anymore. This affected both controlled moves and laser focusing/aligning. I still can’t understand why the support for that was removed as removing the support just removed it without adding anything that made it worthwhile, AFAIK. But’s that another topic, that doesn’t belong here.

droftarts

Thanks for highlighting this. I’ll check with @dc42 whether it’s a documentation and/or firmware change.

Ian