Skip to content
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

Backlash cleanup. Save backlash, fil. sensor, ExtUI userdata to EEPROM #13659

Conversation

Projects
None yet
3 participants
@marcio-ao
Copy link
Contributor

commented Apr 11, 2019

  • Consolidated all the backlash related code into its own ".h" and ".cpp" file
  • Added backlash settings to EEPROM (backlash_correction, backlash_distance_mm[XYZ], backlash_smoothing_mm = 17 bytes)
  • Added filament runout settings to EEPROM (runout_sensor_enabled + runout_distance_mm = 5 bytes)
  • Added 32-bytes [1] of user storage for ExtUI.

@thinkyhead: My understanding on how the EEPROM macros work is limited, please look this over and make sure I am doing things correctly.

[1] The size of 32 bytes was chosen because this is about how much the Lulzbot touch UI will require to be made compatible with all boards. This storage will be used for sound volume, screen brightness, touch point calibration matrix, sound scheme preferences and screen lock password. It is anticipated that each ExtUI will have different needs, but 32 bytes seems like a reasonable starting point.

@marcio-ao marcio-ao force-pushed the marcio-ao:pr-backlash-cleanup-and-new-eeprom-settings branch from aa8f81a to 6401f25 Apr 11, 2019

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

Settings stored in EEPROM need G-code backing, so for runout_distance_mm we ought to add M412 D and include this in the EEPROM output for M503.

@marcio-ao marcio-ao force-pushed the marcio-ao:pr-backlash-cleanup-and-new-eeprom-settings branch 2 times, most recently from e375586 to 356bc9d Apr 11, 2019

@marcio-ao

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

@thinkyhead: I have a question about the EEPROM code. It seems like there are two possibilities for dealing with options that are enabled or disabled:

  1. The layout of the EEPROM is the same regardless of which options are enabled. This would mean that if an option is disabled at compile-tine, padding values are still written to the EEPROM
  2. The layout of the EEPROM varies depending on which options are enabled.

Looking at "configuration_store.cpp", some things suggest that 1) is the goal, while other things make it seems like 2) is the goal.

For example, if 1) is the goal, then the struct SettingsData should not have any conditional statements. But yet it does. The size of the SettingsData structure varies based on HAS_HOTEND_OFFSET, ENABLED(MESH_BED_LEVELING), GRID_MAX_POINTS_X, GRID_MAX_POINTS_Y, AUTO_BED_LEVELING_BILINEAR, ENABLED(DELTA), EXTUDERS > 1

So that suggests that in practice, 2) is true. If that is the case, all the conditional later on in the file which write padding bytes does not make sense.

So, I'm at a loss here. I don't really understand what the goal is.

@marcio-ao

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

@thinkyhead: Also, it seems like the _FIELD_TEST() stuff might want to make use of static_asserts, although I do not know if that feature is getting held back for now for compatibility reasons.

@marcio-ao marcio-ao force-pushed the marcio-ao:pr-backlash-cleanup-and-new-eeprom-settings branch 2 times, most recently from 49f2753 to 5010f3a Apr 12, 2019

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

The goal has been to keep the EEPROM structure the same if the machine architecture is the same. So on a Cartesian enabling or disabling options should ideally leave the structure unchanged. Exceptions exist. Adding an EXTRUDER alters the structure.

Not every contributor has followed this maxim, and it is not always very easy to follow.. So there may be holes. Fill them in when you find them.

I don’t think FIELD TEST can be made static constexpr. It compares a variable.

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

I plan to rewrite the EEPROM code soon. It will use IFF format instead of being a blind binary blob.

@marcio-ao

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

The goal has been to keep the EEPROM structure the same if the machine architecture is the same. So on a Cartesian enabling or disabling options should ideally leave the structure unchanged.

@thinkhead: I see. So in the case of the stuff I am adding, backlash and runout sensor variables should reserve space.

How about the ExtUI? Would that be part of the machine architecture, or part of the options? That's kind of a gray area.

I don’t think FIELD TEST can be made static. It compares a variable.

Gotcha. I missed that. There is a index variable. Sometimes I really wish C++ had the concept of a compile-time variable that could be incremented, but would otherwise behave like a constexpr.

Although why not simply create a SettingsDataStruct on the stack, copy all the values into it, then write the whole data block to EEPROM with a loop? Rather than having to verify that data is written in the order that corresponds to SettingsDataStruct, just write SettingsDataStruct itself.

@marcio-ao

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

I plan to rewrite the EEPROM code soon. It will use IFF format instead of being a blind binary blob.

It seems like IFF requires tags and lengths for the various chunks. Given that EEPROM is only 1024 bytes, wouldn't it be wasteful even if the tags were just a byte? I don't think the current scheme is bad per se...

@marcio-ao

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

The goal has been to keep the EEPROM structure the same if the machine architecture is the same. So on a Cartesian enabling or disabling options should ideally leave the structure unchanged.

@thinkyhead: Maybe "machine architecture" should be interpreted as "things that require hardware changes". So in that case, backlash compensation would be an option that reserves space in the EEPROM, but filament sensor and ExtUI would be hardware changes that wouldn't. I'll make that adjustment.

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

Given that EEPROM is only 1024 bytes, wouldn't it be wasteful even if the tags were just a byte?

We have to "waste" some space if we want structured data that can be interpreted in a forward- and bacward-compatible way. It's likely that a good amount of space will be saved by not having placeholders for disabled features. It is worth developing, and then we will see whether it fills up 1024 bytes more quickly than the current schema.

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

filament sensor and ExtUI would be hardware changes that wouldn't

Under the current scheme, any feature that would add data to the EEPROM should get a placeholder when disabled.

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

ExtUI?

We can consider changing the screen a big enough change that we don't need placeholding for that.

@marcio-ao

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

Under the current scheme, any feature that would add data to the EEPROM should get a placeholder when disabled.

Do you want me to change the filament sensor back to reserving space as it was before? It makes more sense to me for it not to reserve space, because a printer either has or does not have a filament sensor. Having placeholders is a convenience to people who may be trying different software settings, but if a change already requires adding or removing hardware, or rewiring their board, it doesn't seem like clearing the EEPROM and starting from scratch is that inconvenient. Plus it ultimately saves on EEPROM space, because not every printer is going to have every hardware feature that is available.

We can consider changing the screen a big enough change that we don't need placeholding for that.

Sounds good. Once you move to using IFF, we can consider giving the ExtUI a unique tag, or possibly giving each ExtUI display type a unique tag.

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

Do you want me to change the filament sensor back to reserving space as it was before?

It's up to your discretion. If the feature is small and only saves 10 bytes then it's no big deal to retain the placeholder, and it will not raise alarms if someone notices the inconsistency w/r/t other settings.

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

possibly giving each ExtUI display type a unique tag

The more generic the EEPROM storage is, the better. And remember, everything in EEPROM requires a G-code so that settings can be reported/saved/restored in the form of G-code.

@marcio-ao

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

It's up to your discretion. If the feature is small and only saves 10 bytes then it's no big deal to retain the placeholder, and it will not raise alarms if someone notices the inconsistency w/r/t other settings.

@thinkyhead: I was the one who noticed the inconsistency and I feel it would have made sense if there had been a code comment explaining the rationale. I added some comments. Let me know if it is you're okay with the things how they are now.

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

In spite of my statement of justification, there's no hard and fast rule about these settings related to "hardware changes" per se, but we have been aiming to try to keep the EEPROM structure consistent so it doesn't break so easily on common config changes. Some types of hardware add-ons are so common that it is sensible to have a placeholder for it. Filament sensors would be a classic example.

@marcio-ao

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

Some types of hardware add-ons are so common that it is sensible to have a placeholder for it. Filament sensors would be a classic example.

@thinkyhead: Okay. I'll make the change then.

@thinkyhead thinkyhead force-pushed the marcio-ao:pr-backlash-cleanup-and-new-eeprom-settings branch from 858440c to 78a5d70 Apr 12, 2019

@marcio-ao marcio-ao force-pushed the marcio-ao:pr-backlash-cleanup-and-new-eeprom-settings branch from e949f48 to 6d4a1b3 Apr 15, 2019

@marcio-ao marcio-ao force-pushed the marcio-ao:pr-backlash-cleanup-and-new-eeprom-settings branch from f3ef63b to 479b62d May 2, 2019

thinkyhead and others added some commits May 2, 2019

DO NOT wrap whole headers in config conditions
Instead, just don't include the header when the feature is not enabled.

Since is often necessary to use configured values in a header, of course it's appropriate to include `MarlinConfigPre.h` and use `ENABLED`, etc.
Merge branch 'pr-backlash-cleanup-and-new-eeprom-settings' of github.…
…com:marcio-ao/Marlin into pr-backlash-cleanup-and-new-eeprom-settings
@marcio-ao

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

@thinkyhead: It looks like travis is borked at an "apt-get install"

E: Unable to locate package g++-7
E: Couldn't find any package by regex 'g++-7'
@p3p

This comment has been minimized.

Copy link
Member

commented May 3, 2019

poked it with a stick, appears to be running again.

@marcio-ao

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

poked it with a stick, appears to be running again.

Travis is now blind in one eye.

@marcio-ao

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

Oooooo. It's green again. Excellent! This PR has been more in the red than in the green...

@thinkyhead

This comment has been minimized.

Copy link
Member

commented May 3, 2019

We love working for the green checkmarks, even if sometimes it is just fighting Travis implosions.

@thinkyhead thinkyhead changed the title Backlash code cleanup; save settings for backlash, filament sensor and ExtUI user data to EEPROM Backlash cleanup. Save backlash, fil. sensor, ExtUI userdata to EEPROM May 3, 2019

@thinkyhead

This comment has been minimized.

Copy link
Member

commented May 3, 2019

Ok, this seems pretty solid now! Just waiting for one more pass, and then it will be merged.

@marcio-ao

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

Travis is temperamental today.

@p3p

This comment has been minimized.

Copy link
Member

commented May 3, 2019

That would be easier if PlatformIO hadn't just decided to break in new and exciting ways, even running the tests (or building) locally is failing

*** [.pioenvs/LPC1768/firmware.elf] Implicit dependency `/home/p3p/.platformio/platforms/nxplpc-arduino-lpc176x/builder/${common.build_flags}' not found, needed by target `.pioenvs/LPC1768/firmware.elf'.

@p3p

This comment has been minimized.

Copy link
Member

commented May 3, 2019

Probably something to do with PlatoformIO 4.0.0a9 and Include external configuration files with "extra_configs", why we use the dev branch for the platformio install in Travis is an unrelated issue ;)

@thinkyhead

This comment has been minimized.

Copy link
Member

commented May 3, 2019

Don't bring Plato into this. Aristotle is clearly the more at fault.

@marcio-ao

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

What's a Malyan display? The one that comes up when I search looks like a clone of the reprapdiscount display

@thinkyhead

This comment has been minimized.

Copy link
Member

commented May 4, 2019

It comes with the Malyan M200, apparently.

@thinkyhead thinkyhead force-pushed the marcio-ao:pr-backlash-cleanup-and-new-eeprom-settings branch from 9b2c3e9 to 6fcc3bd May 4, 2019

@thinkyhead thinkyhead merged commit 15357af into MarlinFirmware:bugfix-2.0.x May 4, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@marcio-ao marcio-ao deleted the marcio-ao:pr-backlash-cleanup-and-new-eeprom-settings branch May 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.