search

markwal / GPX

Gcode to x3g conversion post processor
GNU General Public License v2.0
98 stars 80 forks source link

Change M126 return value #23

Open pszczelaszkov opened 7 years ago

pszczelaszkov commented 7 years ago

Well its not direct pull request but rather point to discuss, Since everything thats not 0 is always true, why there is casting to 1:0 ? Imho it complicate things. I think the only firmware that uses this call is Sailfish. I even created pull request on Sailfish making use of M126 https://github.com/jetty840/Sailfish-MightyBoardFirmware/pull/183 We tested this change on s3d and slic3r (which i modified a little too) but i wont pull request there until we discuss this change. Its nice feature on MB having dynamic fan change.

markwal commented 7 years ago

I think the kicker is to make sure that MakerBot's firmware also does & 0x01 on the one byte operand and/or uses any non-zero value as "true".

markwal commented 7 years ago

Just checked. At least for Mightyboard's it looks ok: https://github.com/makerbot/MightyBoardFirmware/blob/207c6db181565dacba22a0d4cc271dbe71cfcd7a/firmware/src/MightyBoard/Motherboard/Command.cc#L420

dcnewman commented 7 years ago

On 13/08/2017 4:46 PM, Mark Walker wrote:

I think the kicker is to make sure that MakerBot's firmware also does & 0x01 on the one byte operand and/or uses any non-zero value as "true".

https://github.com/makerbot/MightyBoardFirmware/blob/master/firmware/src/MightyBoard/Motherboard/Command.cc#L419

markwal commented 7 years ago

Seems like we might want to just pass through the value as the byte instead of normalizing to 0-100. Mainly so that M106 S255 still works the same as before for non upgraded firmwares. And then if necessary normalize on the firmware side to 0-100 if that works better on that side. It's a few more bytes on the firmware side that way, but more likely to be backward compatible. Yes?

pszczelaszkov commented 7 years ago

The point is i was looking on Sailfish while i was writing it. They are storing values of fan in EEPROM as 0-100. I added just possibility to change this value via gcode. If there is way to switch from 0-255 to 0-100 without using floats and dividing in this function. It will be ok. I just dont want to put another cpu consuming operations,cuz look ahead how the final formula looks like, its just phat. By backward compatibility i mean that from sailfish perspective u can still use slicers that are sending plain M126 without value like replicatorG with skeinforge and slic3r with with makerbot profile. Because take in mind that im talking about M126 not M106. It looks twisted, but yeah trying to simplify merging on sailfish side it ended like that on other side. So maybe adding some kind of additional argument when launching GPX, will solve problem?

d235j commented 7 years ago

@pszczelaszkov: That function is currently doing ((uint16_t)(1 << FAN_PWM_BITS) * fan_pwm) / 100.0) to generate the output value. fan_pwm is the input in 0-100, and there's one division here, so the most computationally efficient solution would be to set a flag if the value was read from the eeprom, and divide by 255 in that case, or divide by 100 otherwise. Would be messy though.

My suggestion to ensure things work OK on Makerbot default firmware is to |= 0x1 the value if it's >0 (while still treating fan=1 as a special case - but having GPX-output gcode never output fan=1). Yes, this will increase the desired fan rate slightly in 50% of cases, but that's better than not having the fan turn on at all.

In this case, for M106/M107, it would be the responsibility of GPX to convert that to the proper commands, ensuring that the fan is on.