Add OPTIMIZED_MESH_STORAGE option (for UBL)

[ubik2]

Description

Adds a configuration option to AUTO_BED_LEVELING_UBL that will cause the step values to be stored instead of the raw z values.

Before saving the values, they are converted into an array of z_step_values, and we store those instead. When we load a configuration, we convert them back into z_values.

I think the granularity of the z_step_values is the most precision you can take advantage of, so the extra range of a float isn't needed. On the other extreme, even with a steps_per_unit of 4000 on Z, you still have room for +/- 8mm in the int16_t.

After enabling this option, the existing stored values (stored without the option) will not be correct, and you will need to probe again and save the results. Similarly, after disabling it, those stored values created with it enabled will not be correct. This is similar to other changes, like the number of grid points, which also invalidate the stored data.

Benefits

For some machines, like the Ender 3, you only have 2k of EEPROM, and with the 10x10 grid, each saved bed takes 400 bytes, since the z value is a float. The same configuration storing int16_t values takes 200 bytes, which lets me store the results for different beds.

Configurations

I left the feature turned off, since I think that's the appropriate default. Users running low on EEPROM can enable it if needed, by uncommenting the option in Configuration_adv.h

#if ENABLED(AUTO_BED_LEVELING_UBL)
  // Store mesh offsets as steps rather than mm.
  // This will also store the values as int16_t to save EEPROM space
  #define MESH_STORE_STEPS
#endif

Related Issues

[sjasonsmith]

This seems like a positive change. I have a few thoughts which may or may not be issues...

1. Will int16_t always be enough? If so, should we always store as steps, rather than making it an option?

2. Does anything prevent an old float mesh from being loaded as an int16_t mesh, when this option is enabled?

3. Could we identify the format of a stored mesh, in order to load either float or int16_t meshes which already exist? Perhaps convert from one to the other.
   I suspect this is using fixed offsets for each slot, so this likely is not possible.

I'm not asking for you to change anything right now, these are just items to think about.

[ubik2]

1. Will int16_t always be enough? If so, should we always store as steps, rather than making it an option?

I can't speak for all hardware, so I suspect there are some environments where this wouldn't work. For typical hardware, it should be sufficient, but people use Marlin on all sorts of custom machines.

2. Does anything prevent an old float mesh from being loaded as an int16_t mesh, when this option is enabled?

Loads of an an old float mesh will lead to bad values when loaded as an int16_t mesh. There's no information in a header that would keep this from happening. The overall setting version number could be bumped, but that doesn't seem appropriate. This is basically the same as what would happen if you recompiled with a different grid size, since you'll be reading another mesh's height data and be at a different offset.

3. Could we identify the format of a stored mesh, in order to load either float or int16_t meshes which already exist? Perhaps convert from one to the other.
   I suspect this is using fixed offsets for each slot, so this likely is not possible.

There's currently no good way to do this. There is a proposal for an envelope format for individual EEPROM settings, which would provide a mechanism for doing so. You could guess that the data looks more like float32 than int16 (particularly by looking at multiple points, and the differences between them), but it would only be a guess. I think that wouldn't be worth it overall, since reinterpretation like this could lead to some very hard to diagnose behavior.

[sjasonsmith]

Your responses match what I was thinking. UBL meshes don’t get cleared with eeprom version upgrades, it would be good to have a mechanism to invalidate them if key settings change. That seems out of scope in the PR though.

I was just trying to think through the side-effects of using something like this as a default.

[thinkyhead]

Storing the mesh values in steps is actually kind of arbitrary. It may be simpler to just store the offsets in 1000ths of a millimeter (LROUND(z * 1000.0f)), since we are sure that a valid mesh will have no values near ±16.384mm. To help ensure that values are in range, you could store the median value first, and then store the rest as offsets to that value.

[qwewer0]

Shouldn't this reduce the firmware size? Because it increased it from 244940 bytes to 245164 bytes when OPTIMIZED_MESH_STORAGE is enabled.
Or am I misunderstanding it?

[Roxy-3D]

Your responses match what I was thinking. UBL meshes don’t get cleared with eeprom version upgrades...

This is by design. It is very common for a person (and especially a developer) to change firmware versions. The Mesh data was deliberately moved out of the normal configuration data so it could be preserved across firmware upgrades. It is a real pain to get a mesh built and fine tuned and have it disappear just because the firmware version changed.

[sjasonsmith]

This is by design.

I understand. I wasn't suggesting that they always be invalidated, only if something significant changed that made the stored values no longer valid. For example, switching between storing floats vs. uint16_t, or changing the boundaries of the mesh.

[ubik2]

Shouldn't this reduce the firmware size? Because it increased it from 244940 bytes to 245164 bytes when OPTIMIZED_MESH_STORAGE is enabled.
Or am I misunderstanding it?

This reduces the amount stored in the EEPROM, but involves extra code, so it increases the firmware size.

[ubik2]

Storing the mesh values in steps is actually kind of arbitrary. It may be simpler to just store the offsets in 1000ths of a millimeter (LROUND(z * 1000.0f)), since we are sure that a valid mesh will have no values near ±16.384mm. To help ensure that values are in range, you could store the median value first, and then store the rest as offsets to that value.

I stored steps, since this is the resolution of the printer. There's no point in storing a value that is more precise, since we can't position things more precisely. The downside of this choice is that if you alter your stepper (and rebuild your firmware), you end up with invalid measurements.

[qwewer0]

This reduces the amount stored in the EEPROM, but involves extra code, so it increases the firmware size.

How should I interpret "to save EEPROM space"?

Marlin/Marlin/Configuration_adv.h

Line 1743 in 1fc0dcd

//#define OPTIMIZED_MESH_STORAGE // Store mesh with less precision to save EEPROM space

Apologies if I'm misunderstanding it, but if the compiled firmware is larger than without it, then how it is saving EEPROM space?

[ellensp]

@qwewer0 on a micro controller the program goes into flash storage, the variables go into ram. These are reported at compile time
Long term data is put into eeprom, this is not reported at compile time.
Of course this gets more complicated when eeprom is emulated on sd and in flash, but it still not reported at compile time

So eeprom storage is reduced, by you can't see it at compile time.
But you can see the size of the structures is reduced when looking at the code and examining what is in eeprom.

[stef-ladefense]

Hello,
besides the fact that this way of storing these data takes half the space in eeprom, and since, if I understood correctly, the reproduction granularity is better in precision,
would it make sense to enable this option for other modes and not just for UBL?

[Roxy-3D]

besides the fact that this way of storing these data takes half the space in eeprom, and since, if I understood correctly, the reproduction granularity is better in precision, would it make sense to enable this option for other modes and not just for UBL?

I think I would argue we should store all the mesh data in the same format and using as much of the common infrastructure as possible. A long time ago we put the mesh data for all the leveling systems in the same place and switched everything to use the same #defines to specify size.

If you have the energy to get the other bed leveling systems to use this... I'll help get it merged.