First pass at fixing FLSun's mishandling of EEPROM data

[MaximumChungus]

This PR is not ready for prime time yet, but its mostly there. I wanted to share the changes with your for your review. I am not a Marlin expert and I am sure you know more than I do about it. There are also some files like the readme that are intentioned for my fork and I need to prune them from this request.

I would also like to test it some more on my printer (have been testing only for a couple of days since I finished all the fixes) but so far its working great.

I think I got all the files I changed in this PR but if you get a build error or anything let me know the details and I will see if I left anything out.

Description

So as I have also now commonly seen complained about on the Facebook group, I too was suffering the issue where all configurations would clear upon power cycling the printer, and text would also disappear. This could be solved by going to Language and selecting "English", and then resetting all my calibrations but that's foolish and should not be required.

So I made a large mistake, and started poking around the source code. After adding much logging and probing all of the settings logic I discovered a myriad of issues.

1. In settings.cpp FLSun did not correctly add their custom values to the EEPROM definition. Instead they opted to very smartly just write them to hardcoded EEPROM locations like so

persistentStore.write_data(960, (uint8_t*)&Flsun_language, sizeof(Flsun_language));//新增，在eeprom960位置写入语言数据
persistentStore.write_data(970, (uint8_t*)&total_time, sizeof(total_time));//新增，在eeprom970位置写入总打印时间

If one peruses the rest of settings.cpp you will find that all other original Marlin settings are located in EEPROM based on relative size and position and in reference to the "SettingsDataStruct". FLSun did not update this struct, and instead short-circuited the logic to directly write to the persistent storage using hardcoded values as above into locally defined external vars declared as so (not in the data layout struct)

extern uint16_t Flsun_language;//新增
  extern millis_t total_time;//新增
  extern uint8_t sd_filename_size;//新增
  bool MarlinSettings::save() {

Why is this important? Well one of the reasons that EEPROM storage was unstable, and also why language and print time would so frequently vanish is because the regular marlin settings would end up overwriting this hardcoded 960/970 space when running your custom firmware. I suspect that some additional settings you may have enabled/disabled caused the problem to show up but at the end of the day the implementation they did was just crude and bad. Here are logs that show:

SENDING:M500
echo:L574 POS is :100"
echo:settings: 624 Flsun language is :1"
echo:settings: 625 Total time is :0"
echo:attempting to write value :1"
echo:existing value is :0"
echo:attempting to write value :0"
echo:existing value is :0"
echo:L574 POS is :106"
echo:L574 POS is :107"
echo:L574 POS is :179"
echo:L574 POS is :195"
echo:L574 POS is :199"
echo:L574 POS is :211"
echo:L574 POS is :212"
echo:L574 POS is :216"
echo:L574 POS is :220"
echo:L574 POS is :224"
echo:L574 POS is :225"
echo:L574 POS is :226"
echo:L574 POS is :230"
echo:L574 POS is :234"
echo:L574 POS is :238"
echo:L574 POS is :242"
echo:L574 POS is :246"
echo:L574 POS is :250"
echo:L574 POS is :254"
echo:L574 POS is :258"
echo:L574 POS is :262"
echo:L574 POS is :274"
echo:L574 POS is :278"
echo:L574 POS is :282"
echo:L574 POS is :286"
echo:L574 POS is :290"
echo:L574 POS is :294"
echo:L574 POS is :298"
echo:L574 POS is :302"
echo:L574 POS is :306"
echo:L574 POS is :310"
echo:L574 POS is :311"
echo:L574 POS is :312"
echo:L574 POS is :320"
echo:L574 POS is :328"
echo:L574 POS is :652"
echo:L574 POS is :653"
echo:L574 POS is :654"
echo:L574 POS is :670"
echo:L574 POS is :671"
echo:L574 POS is :675"
echo:L574 POS is :687"
echo:L574 POS is :691"
echo:L574 POS is :695"
echo:L574 POS is :699"
echo:L574 POS is :711"
echo:L574 POS is :723"
echo:L574 POS is :735"
echo:L574 POS is :755"
echo:L574 POS is :757"
echo:L574 POS is :769"
echo:L574 POS is :770"
echo:L574 POS is :772"
echo:L574 POS is :778"
echo:L574 POS is :779"
echo:L574 POS is :811"
echo:L574 POS is :812"
echo:L574 POS is :813"
echo:L574 POS is :817"
echo:L574 POS is :821"
echo:L574 POS is :853"
echo:L574 POS is :917"
echo:L574 POS is :933"
echo:L574 POS is :949"
echo:L574 POS is :953"
echo:attempting to write value :0"
echo:existing value is :1"
echo:L574 POS is :965"
echo:attempting to write value :0"
echo:existing value is :0"
echo:L574 POS is :1073"
echo:L574 POS is :1085"
echo:L574 POS is :1093"
echo:L574 POS is :1105"
echo:L574 POS is :1106"
echo:L574 POS is :100"
echo:L574 POS is :104"
//action:notification Settings Stored
Not SD printing
Disconnected.

The "attempting to write" echo was placed in the low level eeprom_wired file right before the write operation and only fired with the original and proposed values when the "pos" variable was 960 or 970, and the "flsun language is" log message was placed right above the two write_data calls I quoted above. As you can see towards the end of the M500 operation a write was done to the 960/970 sectors that was not related to FLSun's explicit write_data commands.

My fix was just to throw out FLSun's settings.cpp and rebase from a clean copy pulled from Marlin's 2.0.8.1 release. I then added the two relevant FLSun specific fields (total time and language) back in by properly defining them in the data struct and having them written/read by the same relative position logic as all the other fields.

That was one of the problems

2. The default I2C pins for the EEPROM are not appropriate for the Robin Nano V3 board. This is a known and fixed issue in the main Marlin repo. I simply applied that fix in this PR. (https://github.com/MarlinFirmware/Marlin/pull/21174). Don't ask me what the deal is regarding this on the OEM FLSun firmware. They use the wrong pins too I think, so :shrug: :shrug: :shrug:

3. Due to the size of the mesh generated by the "Auto Level" UI function, the write operation to the EEPROM would time out. This would corrupt the EEPROM. In this case after power cycling the screen would hang with no text or temperature readout for 30-60 seconds. It would then restore the EEPROM to the default values in Config.h and resume operation. This is a known and fixed issue on the main Marlin project for the Nano V3 board but FLSun did not utilize the fix for whatever reason. This is a nasty bug that could cause all sorts of unpredictable weirdness even if you didn't turn your printer off after auto-leveling as the EEPROM would be entirely corrupted until the next reboot. (https://github.com/MarlinFirmware/Marlin/pull/21436)

4. I also pulled in a couple of adjacent fixes from the main project which may or may not improve stability Prevent watchdog reset in setup() #21776 (https://github.com/MarlinFirmware/Marlin/pull/21776) Blocking Move Followup 2 (fix delta failed probe) #21781 (https://github.com/MarlinFirmware/Marlin/pull/21781)

I also did some very minor cleanup in the core marlin file to put the FLSun language logic (I don't know why they didn't just tap into the existing Marlin language logic) and the FLSun print time update logic into their own functions. This made syncing them up with the data pulled from settings.cpp easier/cleaner.

Benefits

I can turn my printer on and off now without fear for my, my cat's, or my printer's safety. Settings (including language!) do not reset to the configuration.h constants on power cycle. Missing text no longer occurs.

Configurations

I use the same configuration as in the github project, but with a higher max_temp set.

[MaximumChungus]

Oh, one more thing.

The FLSun power restore stuff probably won't work under this PR. In fact most likely it would introduce the exact same EEPROM overwrite issues. They do the same static position writing to the EEPROM for that in void PrintJobRecovery::write_eeprom()//新增

Your firmware defaults power recover to off and I also do not use it, so I did not open that can of worms. If someone turns it to "on" I imagine it would cause problems (as it probably does on the stock firmware in the background too).

Honestly due to the limited erase/write cycles of EEPROM and the fact it can't exactly be unplugged and replaced, I have no idea why they are trying to use it to store power restore data instead of the SDCard. In reality I think nobody should enable this feature as it will drastically shorten the life of the EEPROM.

[Guilouz]

Good job but do not change readme please.

[MaximumChungus]

@Guilouz the readme changes should be out of the PR now. I think this is good from my end, I double checked and all the debugging/logging code should not be present in the PR changes.

Let me know if you see any issues. So far after numerous power cycles and a couple of days of printing I am not noticing any problems. It's hard to say for sure, but I think this should solve a lot of problems for a lot of people as long as they don't try to enable the power restore feature.

I might go and make a pass at that code too in the future, but I think it would need to be a separate fix/PR as I don't think that should be using EEPROM to begin with.

[Guilouz]

@Guilouz the readme changes should be out of the PR now. I think this is good from my end, I double checked and all the debugging/logging code should not be present in the PR changes.

Let me know if you see any issues. So far after numerous power cycles and a couple of days of printing I am not noticing any problems. It's hard to say for sure, but I think this should solve a lot of problems for a lot of people as long as they don't try to enable the power restore feature.

I might go and make a pass at that code too in the future, but I think it would need to be a separate fix/PR as I don't think that should be using EEPROM to begin with.

Yes it's not normal to store in EEPROM to use power recovery. EEPROM is not designed to record data repeatedly.

[Guilouz]

And just try your PR impossible to compile it :

[dominickp]

It built for me @Guilouz but I'm not sure if I'm doing the same thing

[Guilouz]

There is some issue in MarlinCore.cpp Try to recompile it after deleted .pio and .vscode folders to see.

First opening project folder without compile, there is issue :

[MaximumChungus]

@Guilouz that is strange. That recovery function is an existing line of code from the original source.

[Guilouz]

@MaximumChungus Not the only one issue, there is also issue with sensitive pins and warning with converting strings.

Do you use .pio and .vscode folders from FLSUN or created by vscode ?

[MaximumChungus]

@Guilouz I cloned this project and then opened it with ProjectIO/VS Code via "open folder"

Would that mess something up? The errors you are seeing all seem to point at things that were already in MarlinCore and are unchanged.

For the string warnings I saw those when I initially pulled your project, and also in the original FLSun source. I didn't think anything of them since they were already there (at least when I did my initial build before making any changes)

[Guilouz]

@MaximumChungus You have moved some parts like :

setFLSunLanguage(Flsun_language); setFLSunHours(total_time); print_thr_adress_string(0x13,0x40,"V1.2.1");//显示版本号 print_thr_adress_string(0x13,0x30,"Marlin 2.0.8");//显示版本号 if(recovery.exists())//新增 { jump_to(04); my_print_state = PAUSING;//暂停 } else { jump_to(01); }

[MaximumChungus]

@Guilouz

All I changes was moving the language and fl sun print time logic to their own functions

void setFLSunHours(const millis_t time) {
  char cmd[5] = {0};//新增
  sprintf_P(cmd,"%ld",time/60);
  print_thr_adress_string(0x17,0x00,cmd);
}

void setFLSunLanguage(const uint16_t lang) {
  if(lang == 1)//英语 新增
  {
    change_en();
  }
  else if(lang == 2)//中文简体
  {
    change_zh_CN();
  }
  else if(lang == 3)//中文繁体
  {
    change_zh_TW();
  }
  else if(lang == 4)//俄语
  {
    change_ru();
  }
  else if(lang == 5)//法语
  {
    change_fr();
  }
  else if(lang == 6)//西班牙语
  {
    change_es();
  }
  else if(lang == 7)//德语
  {
    change_de();
  }
  else if(lang == 8)//日语
  {
    change_jp();
  }
}

Which changes

SETUP_LOG("setup() completed.");

  if(Flsun_language == 1)//英语 新增
  {
    change_en();
  }
  else if(Flsun_language == 2)//中文简体
  {
    change_zh_CN();
  }
  else if(Flsun_language == 3)//中文繁体
  {
    change_zh_TW();
  }
  else if(Flsun_language == 4)//俄语
  {
    change_ru();
  }
  else if(Flsun_language == 5)//法语
  {
    change_fr();
  }
  else if(Flsun_language == 6)//西班牙语
  {
    change_es();
  }
  else if(Flsun_language == 7)//德语
  {
    change_de();
  }
  else if(Flsun_language == 8)//日语
  {
    change_jp();
  }
  char cmd[5] = {0};//新增
  sprintf_P(cmd,"%ld",total_time/60);
  print_thr_adress_string(0x17,0x00,cmd);
  print_thr_adress_string(0x13,0x40,"V1.2.1");//显示版本号
  print_thr_adress_string(0x13,0x30,"Marlin 2.0.8");//显示版本号
  if(recovery.exists())//新增
  {
    jump_to(04);
    my_print_state = PAUSING;//暂停
  }
  else
  {
    jump_to(01);
  }

to

SETUP_LOG("setup() completed.");

  setFLSunLanguage(Flsun_language);
  setFLSunHours(total_time);
  print_thr_adress_string(0x13,0x40,"V1.2.1");//显示版本号
  print_thr_adress_string(0x13,0x30,"Marlin 2.0.8");//显示版本号
  if(recovery.exists())//新增
  {
    jump_to(04);
    my_print_state = PAUSING;//暂停
  }
  else
  {
    jump_to(01);
  }

}

void setFLSunHours(const millis_t time) {
  char cmd[5] = {0};//新增
  sprintf_P(cmd,"%ld",time/60);
  print_thr_adress_string(0x17,0x00,cmd);
}

void setFLSunLanguage(const uint16_t lang) {
  if(lang == 1)//英语 新增
  {
    change_en();
  }
  else if(lang == 2)//中文简体
  {
    change_zh_CN();
  }
  else if(lang == 3)//中文繁体
  {
    change_zh_TW();
  }
  else if(lang == 4)//俄语
  {
    change_ru();
  }
  else if(lang == 5)//法语
  {
    change_fr();
  }
  else if(lang == 6)//西班牙语
  {
    change_es();
  }
  else if(lang == 7)//德语
  {
    change_de();
  }
  else if(lang == 8)//日语
  {
    change_jp();
  }

It should not break compilation.

[Guilouz]

Try to compile it you will see. Tested with .pio & .vscode folders from repo and also new one from VSCode, same issue.

[MaximumChungus]

@Guilouz I have compiled it and have the binary running on my SR. It sounds like @dominickp was also able to compile it.

I will clone this PR from scratch in a few to my desktop and see if I notice anything weird. Its possible something minor got left out of the PR but nothing is jumping out to me looking at the diff. I will capture the process just in case we are doing things differently.

[Guilouz]

@MaximumChungus Just download full zip from your repo and just compile https://github.com/MaximumChungus/Marlin-SuperRacer-MKS-Nano-V3

Exactly the same issue.

[MaximumChungus]

@Guilouz From zip to binary I am not seeing any unexpected error (just the error where you have to manually copy over the FLSun .o file)

https://streamable.com/i3cmvf

Here is a video capture. Let me know if I am doing something stupid during the build process that is causing problems on your end and I will try to fix it. Right now I just cannot reproduce what you are seeing =/

[Guilouz]

@MaximumChungus I have find what cause this compilation issue. Impossible to compile when //#define POWER_LOSS_RECOVERY is disabled in Configuration_adv.h.

I have disabled it to avoid issue with EEPROM.

[MaximumChungus]

@Guilouz Ah! Understood. That's not a bad idea. I will do something similar when I push my next update to this branch.

[Guilouz]

@MaximumChungus Trying this thing with //#define POWER_LOSS_RECOVERY but compilation not working

ifdef POWER_LOSS_RECOVERY

{ jump_to(04); my_print_state = PAUSING;//暂停 }

else

{ jump_to(01); }

endif

[MaximumChungus]

@Guilouz looking at the errors, it looks like they are keying off of power loss being defined from within their MarlinSerial.cpp.o file. I have a couple of ideas I will try though.