Laser Pause While Changing Power

Also if your in the USA, they do not have a US store, so they would be a Chinese company trying to sue a US citizen internationaly, based on copyrights that China doesn’t follow anyway. (Thats how you get so many Chinese clones). So I think they would be in a tough position to do anything even if they wanted to.

Marlin is GPL3.0
The SM2 firmware is based on this as well, and thus as a consequence, the adaptation is as well. (GPL is a very viral license, hate it or love it)

When you have GPL3.0 code, (and other GPL versions as well). You are allowed to share it again, make modifications etc according to the rules set out in the GPL license. As long as you only use them for personal use, you don’t have to make the source code available, you can keep it to yourself. As soon as you distribute binaries of it, sell it, devices using it, derivatives etc. you have to make the source code available. One of the main differences between GPL3 & 2 is that you cannot offer cloud services using GPL3 without making your changes available again.

Note that the GPL licenses don’t stipulate how you should make it available, only that you have to do so when someone asks for it and you may even charge a (reasonable) cost for it. So publishing the license here on the forum actually is enough to comply with the license. Theoretically they could even have a procedure in place where you get a cd sent to you by post.

It would be a lot easier to understand what changes were made etc when it came with the full commit log obviously. Just by making a public repo. But in short, today SM isn’t doing anything (obviously) wrong according to the Marlin licensed code as far as I can tell as they are publishing all their Marlin adaptations on this forum.

Edwin promised a publication this month (November) in the source code thread, so I’m hoping it will be published on Github soon.

If not, It would be a great addition to have a repo with all the changes they have made tracked. Ideally even as a branch of the original Marline code base so it’s easy to compare both and merge changes from the origin as well.

2 Likes

It doesn’t matter for our case, but the cloud stuff sounds more like the change between AGPL and GPL than the change between GPL2 and GPL3. If I’m not mistaken there is no such rule about cloud services in the GPLv3.0.

There is one (for us) important difference between GPL2 and GPL3 though: GPL3 prohibits “tivoization”:

If you convey an object code work under this section in, or with, or specifically for use in, a User Product, and the conveying occurs as part of a transaction in which the right of possession and use of the User Product is transferred to the recipient in perpetuity or for a fixed term (regardless of how the transaction is characterized), the Corresponding Source conveyed under this section must be accompanied by the Installation Information.

Therefore Snapmaker has to provide instructions how to install the firmware built from the source code which they AFAICT never did.

2 Likes

Yes, you are correct on this one. mixed those up there. (Although even with standard GPL3 there are some weird edge cases when building web-applications, but it doesn’t matter here)

True, although you could get technical and wonder if anyone already formally asked :wink: And the instructions on how to build the Marlin part are included. And more importantly, they are not actively prohibiting it by requiring it to be signed with a secret key preventing the device from actually running the code. (Which was an important aspect in the tivo-debate)

But thanks for the corrections/additions, Good catch.

2 Likes
2 Likes

Looks like there now is a tool in Luban to create the firmware package (haven’t tried, just based on the change-log):

2 Likes

@brent113 mind sharing the grid gcode? Would make a an excellent calibration job regardless of the acceleration issues. I’ve only spent scant time with the laser tool, and setting power and speed is a dark art to me still. Very dark and slow. :smile:

Could you open an issue for that on the snapmaker Repository or even create a pull request?

I will when I feel ‘finished’. I’m still slowly working out some kinks and edge case instabilities. I’ve found a certain sequence of commands that when issued at boot cause a crash, which is weird.

1 Like

I’m really looking forward to your results :grin:

Got it working, this is the first successful test. Dark to medium to dark gradient, the bottom part of the sign from above.


image

Compared to the original it’s perfect:

None of the banding, and it doesn’t stutter.

Only quirk that I can’t figure out is you have to get the laser fan on before running any inline power commands, otherwise the firmware does an illegal thing that locks the chip. Specifically it enters a critical section from an ISR illegally, which FreeRTOS checks for and kills the running task. You have to use the ISR specific critical section entrance, but this only occurs in this one specific instance.

4 Likes

Where’s the defective code found? Either the caller or the ISR or both have defects.

In this case it’s a caller defect I added.

In the original code the laser power would not be modified during motion, and for this new inline power to work the motion ISR updates laser power. It’s something about this new functionality that is causing the issue, but not consistently. Only once at startup, which for the moment can be worked around.

Here’s the steps I use to reproduce and the stacktrace:

  1. Build and load the firmware from https://github.com/brent113/Snapmaker2-Controller/tree/2.0.x_laser_inline (edit: since fixed, have to build and test commit 193f311cbe)
  2. Connect with Luban via USB, the machine will home.
  3. Issue the following command: G1 X50 P2

The laser and fan do not turn on and no motion occurs.

  1. At this point issue a debugger pause and break, the firmware will be paused in vPortRaiseBASEPRI in portmacro.h

The stack trace is:

#0  0x0800fa40 in vPortRaiseBASEPRI () at snapmaker\lib\GD32F1\libraries\FreeRTOS1030\utility\include/portmacro.h:195
#1  vPortEnterCritical () at snapmaker\lib\GD32F1\libraries\FreeRTOS1030\utility\port.c:421
#2  0x0800ff98 in xQueueSemaphoreTake (xQueue=0x20005330 <ucHeap+6576>, xTicksToWait=<optimized out>, xTicksToWait@entry=4294967295) at snapmaker\lib\GD32F1\libraries\FreeRTOS1030\utility\queue.c:1448
#3  0x08014976 in CanChannel::Write (this=0x2000cf18 <can>, packet=...) at snapmaker\src\module\can_channel.cpp:67
#4  0x08014e2e in CanHost::SendStdCmd (this=<optimized out>, message=...) at snapmaker\src\module\can_host.cpp:149
#5  0x080173ee in ToolHeadLaser::CheckFan (this=<optimized out>, pwm=pwm@entry=1) at snapmaker\src\module\toolhead_laser.cpp:168
#6  0x0801740a in ToolHeadLaser::TurnOn (this=<optimized out>, pwm=<optimized out>) at snapmaker\src\module\toolhead_laser.cpp:124
#7  0x0802014a in SpindleLaser::set_ocr (ocr=<optimized out>, ocr@entry=1 '\001') at Marlin\src\feature\spindle_laser.cpp:49
#8  0x0800c546 in SpindleLaser::set_ocr_power (ocr=1 '\001') at Marlin\src\module\../feature/spindle_laser.h:98
#9  Stepper::block_phase_isr () at Marlin\src\module\stepper.cpp:1899
#10 0x0800cb32 in Stepper::isr () at Marlin\src\module\stepper.cpp:1430
#11 0x0800cc22 in stepTC_Handler () at Marlin\src\module\stepper.cpp:1356
#12 0x0800e89a in dispatch_general (dev=0x200001c8 <timer4>) at snapmaker\lib\GD32F1\system\libmaple/timer_private.h:195
#13 __irq_tim4 () at snapmaker\lib\GD32F1\cores\maple\libmaple\timer.c:546
#14 <signal handler called>
#15 0x0800f864 in prvPortStartFirstTask () at snapmaker\lib\GD32F1\libraries\FreeRTOS1030\utility\port.c:270
#16 0x0800f9d2 in xPortStartScheduler () at snapmaker\lib\GD32F1\libraries\FreeRTOS1030\utility\port.c:385

The code is broken here because of #2 in the stack trace:

void vPortEnterCritical( void )
{
	portDISABLE_INTERRUPTS();
	uxCriticalNesting++;

	/* This is not the interrupt safe version of the enter critical function so
	assert() if it is being called from an interrupt context.  Only API
	functions that end in "FromISR" can be used in an interrupt.  Only assert if
	the critical nesting count is 1 to protect against recursive calls if the
	assert function also uses a critical section. */
	if( uxCriticalNesting == 1 )
	{
		configASSERT( ( portNVIC_INT_CTRL_REG & portVECTACTIVE_MASK ) == 0 );
	}
}

Normally, no CAN functions would be sent during the Stepper::block_phase_isr () but with the new functionality the laser is being turned on from there.

One workaround this bug is immediately prior to calling G1 X50 P2 you simply issue a command that turns the laser fan on, like M3 P0.5

It appears some code needs to be changed somewhere, possibly in the ST HAL, that detects if communications are being sent from an ISR, and calls taskENTER_CRITICAL_FROM_ISR() instead of taskENTER_CRITICAL(). This possibly could be implemented in xQueueSemaphoreTake in queue.c, although maybe there’s a better place.

2 Likes

I hacked around this issue by manually controlling the fan at the time of gcode parsing, before the ISR starts handling the motion. This fixes the issue for the above reproduction steps.

It’s not as general of a fix as I was hoping for, and there may be other things pop up with additional testing.

1 Like

It’s more than just some code. Just from the stack trace it’s clear that the authors are completely inexperienced with this kind of programming. In most cases the ISR should do one or two atomic operations on something like a priority queue and then immediately exit. Bulk logic can be done outside an interrupt context. In simple system, you can get away with doing otherwise, but as the system gets more crowded, you end up with this kind of defective code causing faults, and, in this case, system hangs.

Yea. Since this issue crept in when I changed something I don’t think it’s Snapmaker’s fault. Also, this is deep in the FreeRTOS library, so I’m not really sure if it’s not a FreeRTOS issue either. I think many of the CAN functions are from the STM HAL library, so that might be the issue too?

There’s like 3 major libraries that could be at fault here, and also it could just be me.

But yea, I don’t really understand what the semaphores are being used for specifically here, so I don’t really have a good idea on where the fix goes.

Looks like it to me. Items #3 to #6 are in the module-specific code. The function ToolHeadLaser::TurnOn, well, first off all, it’s implemented on the controller side, not in the module itself where it really belongs. (The module should know when it needs cooling, after all.) But what the code does is to check some internal state and send out CAN commands if needed, which is absolutely what you should not do inside an ISR.

I have to add, however, that Marlin isn’t helping at all here. Marlin does way too much computation within its ISR for my taste. Marlin wasn’t ever really designed to be arbitrarily extensible, and it shows.

@brent113 Have your tip jar ready when you post a solid firmware and process to load it so I can give you money because that’s happening. Having fun following the work so far, keep it up.

4 Likes

I’ve been using this firmware for some time now, and it seems pretty stable.
image

Here it is if you want to try it out AT YOUR OWN RISK.
Snapmaker_Laser_Inline_FW_Beta.zip (141.4 KB)

Known issues:

  • The controller reported laser power will read 0-255%. Haven’t fixed this yet, but it’s a visual glitch only.
  • The touchscreen cannot sent gcode fast enough - it needs to be run from Lightburn directly in buffered mode. Running from the touchscreen will result in some jerky motion with unexpected darkening in some spots. However, there is a communications issue that as of now that makes the Lightburn communications unstable - running from Luban seems to work acceptably though.

This firmware works best using Lightburn to generate native Marlin firmware (with the S parameter, not the P parameter).
Lightburn Device Config:
image
image

On average each inline gcode is 20 bytes, so at 115200 baud a maximum of 720 lines can be sent per second. You can figure out your maximum DPI from the max travel speed. Assuming a max travel speed of 30mm/s, that would be roughly 0.05mm per line of gcode to not saturate the serial bus, which is 500DPI. The default DPI of 254 is fine, and if you need a bit higher you can push it a bit.

3 Likes

Uh

me want burn nice

me no understand

me will try to play with laser on stock first to know what is going on then bask in the glory of this very complicated intricate thing you did

the engineering efforts on this forum just astound me to no end

i wonder if snapmaker will take a page from your book here and make their shit better yeah?

1 Like