Guilouz / Marlin-SuperRacer-MKS-Nano-V3

Marlin Firmware configured for FLSUN Super Racer with MKS Robin Nano V3 motherboard.
GNU General Public License v3.0
68 stars 23 forks source link

Removal of FLSun EEPROM logic related to power loss protection #6

Closed MaximumChungus closed 3 years ago

MaximumChungus commented 3 years ago

Description

In my first PR the power loss eeprom calls were left intact. Due to dependencies inside of the FLSun Marlin_Serial.cpp.o file it is not currently possible to fully disable Marlin's power loss protection feature via the normal means of undefining the flag. In this case I have located all eeprom calls FLSun makes inside of the power loss recovery logic and just commented them out.

I have also pulled in some other stability fixes from the main project where appropriate

  1. STM32: Prevent possible crashes before HAL init #22508 (https://github.com/MarlinFirmware/Marlin/pull/22508) -> This appears to reduce the frequency and duration of the bootup hanging

  2. Make F_CPU a compile time constant (and not a runtime constant). #21051 (https://github.com/MarlinFirmware/Marlin/pull/21051) -> This seems to improve stability when performing eeprom operations, particularly when printing is currently in progress

  3. Fix Robin Nano V3 board X-diag pin define #22340 (https://github.com/MarlinFirmware/Marlin/pull/22340) -> This discrepancy was discovered on the main project recently on 07/12/21 and corrects the pin definition for X_DIAG. PD15 was used due to a typo on the pin map that the manufacturer has since corrected (https://github.com/makerbase-mks/MKS-Robin-Nano-V3.X/issues/17)

  4. HAL eeprom cleanup (https://github.com/MarlinFirmware/Marlin/commit/38b44e3fc9cb96866f9c25058667888ae43cfc0c) -> This modernizes the eeprom code used by our boards and corrects lots of bad type definitions and such

  5. Fix smaller and larger I2C EEPROM support #22081 (https://github.com/MarlinFirmware/Marlin/pull/22081/files) -> Similar to item 4, this is just improvements to the eeprom logic that is present in newer versions of Marlin

Benefits

After extensive testing of my initial branch, I was able to locate one edge case where eeprom would still be lost due to eeprom writes that occurred during printing due to the remaining FLSUN power loss writes. This was (fortunately!) very hard to reproduce even trying to make it happen and I think was a combination of the additional FLSun writes during active printing + extra and inaccurate delays due to limited bandwidth when the board is tied up with the print calculations. With the changes in this branch I am no longer able to reproduce loss of the eeprom through any use case.

Additionally as a happy side effect of some of these updates from the main branch I am noticing that the slow bootup/bootup hangs happen much less frequently and when they do occur they are much shorter duration than before. Temperature readings and print time may disappear during the short hang, but they will restore to the correct values from the eeprom and the printer will work as intended.

@Guilouz as a side note I know none of these changes (in this PR or the last) are super fancy, but the testing, debugging, and finding of the correct patches to pull in from Marlin have been very time consuming. I would appreciate greatly to leave my name in the config author area if you find these changes helpful. I mostly went to the trouble to fix my own SR, but do appreciate some small credit for the time spent.

Anyways, I can use my printer to print now without worry and that's a major victory. I hope these changes also work well for you and other users. If I notice anything else I will continue to provide the updates.

Guilouz commented 3 years ago

Sorry I pushed other changes behind your PR and didn't pay attention that the credits were removed.

So, you confirm that with this PR it is no longer possible to use the Power loss function, that's right? Or is it just to prevent writing to EEPROM but the function can still be used?

And I have feedback from 3 people with your last PR, when temperature is changed during printing, machine freezing. All settings (PID etc.) are restored to default value after power off / power on. I think issue with all command send from TFT.

I think there is some errors in EEPROM and function EEPROM_AUTO_INIT reset EEPROM after power on.

MaximumChungus commented 3 years ago

No worries.

So on the topic of the power loss recovery, the tldr; is that it just disables the eeprom writes that FLSun added to the native Marlin power loss logic.

I tried a few other workarounds to more thoroughly disable the entire feature but none of them worked out, or they impacted system stability. The issue boils down to the fact that marlin_serial.cpp.o doesn't correctly respect the "POWER_LOSS_RECOVERY" flag. The best I can tell is that FLSun has something kind of like this in their binary code

#ifdef POWER_LOSS_RECOVERY
import "powerloss.h"
#endif
......

recovery.purge(....);

when FLSun should have done something like this

#ifdef POWER_LOSS_RECOVERY
import "powerloss.h"
#endif
......
#if ENABLED(POWER_LOSS_RECOVERY)
recovery.purge(....);
#endif

So FLSun's import is left out if we disable the "POWER_LOSS_RECOVERY" flag, but their code still tries to reference it and thus the undefined reference errors cause compilation to fail. powerloss.h actually defined empty stub functions for everything in the case that "POWER_LOSSRECOVERY" is not defined but since FLSun only seems to check for the flag on their import statement those aren't preventing errors as they should normally. It would be an easy fix if_ we had access to the source code for it, but we don't.

I tried adding an additional flag to basically override "POWER_LOSS_RECOVERY" everywhere it is used, but this touched too many parts of Marlin and very negatively impacted system stability.

So with this PR the FLSun additions to powerloss.cpp should be disabled, but the native Marlin powerloss logic is still there. In theory the native Marlin logic by itself shouldn't cause as many problems and its possible in the future we could get power loss recovery working properly by taking a look at what they currently do in powerloss.cpp on the main project. I suspect like FLSun's custom language and print time variables that their custom power loss recovery logic probably wasn't truly needed in the first place and came from a lack of understanding on how to access and work with the existing Marlin implementations from within their custom TFT code.

And I have feedback from 3 people with your last PR, when temperature is changed during printing, machine freezing. All settings (PID etc.) are restored to default value after power off / power on. I think issue with all command send from TFT.

I think there is some errors in EEPROM and function EEPROM_AUTO_INIT reset EEPROM after power on.

That's interesting. That's similar to the issue I would see before my changes in the last PR on my printer that initially led me to look into the firmware source files. However in my case it would just freeze while trying to reach the initial print temperature which is a little different.

So if I understand correctly they are starting a print, and then using the touchpad once the print head has started printing they are raising/lowering the temperature right? I will add that to my list of things to test going forward. I have adjusted the z-offset while running a print without issue, but I have not tried temperature yet since that doesn't seem to trigger an eeprom event.

One thing I would mention is that unlike changing z-offset, changing the temp shouldn't write to the eeprom it should just send a gcode command (just like changing fan speed). One thought I have is that it might be related to the remaining eeprom reset issue that this PR should address. It's possible that when they did the temp change something from powerloss.cpp triggered a eeprom write. The FLSun additions to that file trigger ~24 eeprom writes whenever FLSun's write_eeprom function is called. Due to slowness of commands while printing, its possible that even with the extra watchdog delays added in the last PR that this still caused the writes to timeout and leave the eeprom in a bad state (which would explain the freezing as well potentially)

If one of those users is able and willing, you might see if they can try building the firmware from this PR branch to see if that still happens.

MaximumChungus commented 3 years ago

@Guilouz did a little testing with the temperature changes during printing using firmware built from this branch and I wasn't able to trigger an eeprom issue so far.

There were a few times that I would press "Ok" after entering the new temperature value and it would take a second or two for the target temperature to update, but the screen never froze (current temperatures continued to update normally while I waited) and the new target temperature would update.

I toggled the temps up and down many times, also changed the fan speed many times, and adjusted the z offset while printing a couple of times. No freeze or crash and after a power cycle the eeprom was still intact.

I did one test changing all of the values many times and then killing power to the printer while the print job was still running. In this case the eeprom was also still intact when the system restarted.

I did all of the temp (bed and nozzle), fanspeed, and z-offset changes for these tests only using the touchscreen UI.

Guilouz commented 3 years ago

@MaximumChungus Yes I have share to few people a builded firmware with this changes, one people said me it's ok, I waiting for others.

I can't try, I no longer use the stock screen and the nano v3 board.

MaximumChungus commented 3 years ago

I don't blame you. Board is probably ok, but its weird they went to so much trouble in the software just to use this particular touch screen.

Guilouz commented 3 years ago

@MaximumChungus Do you have any idea why buzzer sound is not saved to eeprom? command is working but after restart printer settings is not loaded. User reported also E-Step not saved.

MaximumChungus commented 3 years ago

Hey @Guilouz . Do you know more details? I know there is M300 command to play a tone but I am not familiar with a command that can be saved for the buzzer/beeper sound. It looks like FLSun may have added M114 B enabling/disabling the beeper.

I do notice that "Speaker" is not enabled in SR firmware https://github.com/Guilouz/Marlin-SuperRacer-MKS-Nano-V3/blob/bf96d4d0cec2db179a1054bdcf28ddce5ab7c733/Marlin/Configuration.h#L1956-L1974 and also the local beeper is disabled in configuration.h as well.

If I know the command being used I might be able to use that to look for something. I do see a buzzer flag that it looks like FLSun added to the settings that I must have missed before in settings.cpp

extern bool buzzer_flag;//新增

persistentStore.write_data(960, (uint8_t*)&buzzer_flag, sizeof(buzzer_flag));//新增,写入蜂鸣器flag

persistentStore.read_data(960, (uint8_t*)&buzzer_flag, sizeof(buzzer_flag));//新增

It looks like the only way FLSUN implemented to change this flag is by using the "M114 B" which will disable the buzzer for 0 and enable it for any other value. https://github.com/Guilouz/Marlin-SuperRacer-MKS-Nano-V3/blob/9c49db07281d98d3dab578c22b082d3f950a4593/Marlin/src/gcode/host/M114.cpp#L192-L219

It should probably be fixed to save to the EEPROM correctly instead of using the hardcoded 960 address. That is what was fixed for the other two settings FLSun added to settings.cpp that was previously causing a lot of the problems.

Guilouz commented 3 years ago

@MaximumChungus Not needed to enable buzzer in configuration.h. Flsun use M114 command and add a new "B" argument to enable/disable buzzer sound of their screen. But I have no idea how to store this settings correctly.

MaximumChungus commented 3 years ago

Yeah, that was what it looked like in the code.

Probably it needs to be moved into the settings object the correct way like the other two were. I don't think this flag was in the firmware when I made the previous changes so I assume FLSun added it in a recent update maybe.

I will try to take a look when I have a chance. I have some long prints running on my SR right now but when it has some down time I will take a look.

Guilouz commented 3 years ago

@MaximumChungus Yes it's a new stuff from latest Flsun firmware. This is the changelog :

v1.3 2021.8.10(you need click "restore"or M502 & M500 command after update the firmware) 1.add a command to control screen buzzer. M114 B0 & M500 is turn off the buzzer. M114 B1 & M500 is turn on the buzzer. 2.fix skr v1.3 board save power-loss data while printing in v1.2.1 version. 3.change the number of FILAMENT_RUNOUT_DISTANCE_MM from 25 to 15. 4.support relative mode. 5.enable hotbed PID. 6.enable hotbed runaway protect. 7.change z step in adjust button, from 0.05 to 0.0125. 8.add a change function while printing. click pause first, then click operation, you will see the change button. 9.change motor current to 1.25A. 10.if you turn off power-loss function, page can't jump to the page that ask you if continue to print.