-
-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Removal of FLSun EEPROM logic related to power loss protection #6
Conversation
🚑️ Init FastIO before anything else (#22508) (MarlinFirmware/Marlin@bc68664)
Make F_CPU a compile time constant (and not a runtime constant). #21051 (https://github.com/MarlinFirmware/Marlin/pull/21051/files)
Fix Robin Nano V3 board X-diag pin define #22340 (https://github.com/MarlinFirmware/Marlin/pull/22340/files)
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. |
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
when FLSun should have done something like this
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_LOSS_RECOVERY" 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.
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 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. |
@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. |
@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. |
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. |
@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. |
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 Marlin-SuperRacer-MKS-Nano-V3/Marlin/Configuration.h Lines 1956 to 1974 in bf96d4d
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
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. Marlin-SuperRacer-MKS-Nano-V3/Marlin/src/gcode/host/M114.cpp Lines 192 to 219 in 9c49db0
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. |
@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. |
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. |
@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) |
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
STM32: Prevent possible crashes before HAL init #22508 (STM32: Prevent possible crashes before HAL init MarlinFirmware/Marlin#22508) -> This appears to reduce the frequency and duration of the bootup hanging
Make F_CPU a compile time constant (and not a runtime constant). #21051 (Make F_CPU a compile time constant (and not a runtime constant). MarlinFirmware/Marlin#21051) -> This seems to improve stability when performing eeprom operations, particularly when printing is currently in progress
Fix Robin Nano V3 board X-diag pin define #22340 (Fix Robin Nano V3 board X-diag pin define MarlinFirmware/Marlin#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 (PD15 for XDIAG and E1 step in MKS Robin Nano V3.0_003 PIN.pdf makerbase-mks/MKS-Robin-Nano-V3.X#17)
HAL eeprom cleanup (MarlinFirmware/Marlin@38b44e3) -> This modernizes the eeprom code used by our boards and corrects lots of bad type definitions and such
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.