Unclear interval in M42

gloomyandy

@Otso You are probably not going to like the answer! From the source code: https://github.com/Duet3D/RepRapFirmware/blob/3.5-remote-input-shaping/src/GCodes/GCodeBuffer/GCodeBuffer.cpp#L809

and:
https://github.com/Duet3D/RepRapFirmware/blob/47435151e110f373fc80721a2c265e38b8ec5e3b/src/GCodes/GCodes.cpp#L4770

Remember that these things have almost certainly been this way for some time ande/or in effect defined by how things like slicers and other firmware use them. So may not be easy to change without causing issues for existing gcode etc.

Otso

@fcwilt said in Unclear interval in M42:

To have values between 0 and 1 they must be fractional.

Thanks for clarifying. I think this should be said in the manual, as it is not fully clear right now, although one maybe could assume this is the way it is. It could say something like

“ The S field may be in the range 0..1 or 0..255. The values in the range 0..1 must be written as a fractional numbers, for example ‘1.0’.”

(0 will be 0 in any case, so it doesn’t matter which range it uses.)

P.S. Actually I 0 to 255 - clear and unambiguous.

Agree, as long as you read it in isolation. It’s when you look at G1 that you start wondering if either is a typo. Why would the range be 0 to 255 in one place and 0 to 254 in another, when both are basically setting the PWM value. To me it feels like G1 is a typo (i.e., that the range is actually 0..255), although it might be correct. I think the documentation for G1 could add some reassuring text explaining why it doesn’t go up to 255.

Otso

@gloomyandy Thank you for pointing out the code location. (Did you accidentally paste the same location twice?).

So giving “S1” (without a fraction) will actually convert to “1.0” (full PWM) and not 1/255, right?

        float v = GetFValue();
	if (v > 1.0)
	{
		v = v/255.0;
	}
	return constrain<float>(v, 0.0, 1.0);
}

I actually thought I might as well have a look at the code, but I thought this requires documentation clarification I wanted to discuss it here. So apparently the fractional range 0..1 is the new preferred way. I didn’t yet dig out the code for G1, but I’d assume it uses the same GetPwmFrequency() function, and thus accept the range 0..255 and not 0..254 as the documentation says. Maybe I should have a look at the code.

It would be nice if G1 also accepted the fractional range.

Anyway, if M42 really uses the above function then I’d consider it a bug. I’ll try “S1” it in practice right away.

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.