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

Adds versioning for Configuration.h & Configuration_adv.h #3609

Merged
merged 2 commits into from
Jun 16, 2016

Conversation

jbrazio
Copy link
Contributor

@jbrazio jbrazio commented Apr 25, 2016

We try our best to include sanity checks for all the changes configuration directives because people have a tendency to use outdated config files with the bleding edge source code, but sometimes this is not enough. This PR will force a minimum config file revision, otherwise Marlin will not build.

PS: It also includes a much improved generate_version_header_for_marlin.

@jbrazio jbrazio added PR: Improvement T: Development Makefiles, PlatformIO, Python scripts, etc. T: Design Concept Technical ideas about ways and methods. PR: New Feature and removed T: Design Concept Technical ideas about ways and methods. labels Apr 25, 2016
@thinkyhead
Copy link
Member

Actually if you try to use a 1.0.x configuration with Marlin now it will always bark. But this is a good possible additional measure.

* the configuration files.
*/
#define REQUIRED_CONFIGURATION_H_VERSION 1
#define REQUIRED_CONFIGURATION_ADV_H_VERSION 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we could also use 010100 (i.e., version 1.1.0) etc. Two digits for each part of the version, still an integer that can be compared with < == >. A thought…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem I see is that we could have multiple config updates during a tag release.. I leave it up to you to decide which is best.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should only put version numbers the configurations in real release versions. This might be version 1, and the configs with 1.1.1 might be version 2 if they change very much. But in between they should just be considered works-in-progress with options that come and go as we figure things out.

@jbrazio
Copy link
Contributor Author

jbrazio commented Apr 26, 2016

Consider this as a safe net under SanityCheck.h.

@tkurbad
Copy link
Contributor

tkurbad commented Apr 29, 2016

+1 from me!
I've just had a serious issue because the "DISABLE_...ENDSTOPS" options were turned into "USE...PLUG".
While transitioning to a positive configuration syntax is generally a good idea, not having set any USE
...PLUG options in my Configuration.h was not caught by SanityCheck.h, because I also had both "DISABLE..._ENDSTOPS" options commented out.

Luckily, I tested with one hand on the "off" switch of the printer...

Demanding a minimum config version could avoid such issues.

@jbrazio jbrazio modified the milestone: 1.1.0 (Gold) Apr 29, 2016
@jbrazio
Copy link
Contributor Author

jbrazio commented Apr 29, 2016

@thinkyhead one more use case.

@thinkyhead
Copy link
Member

@tkurbad I'm surprised this didn't catch your error:

/**
 * Endstops
 */
#if DISABLED(USE_XMIN_PLUG) && DISABLED(USE_XMAX_PLUG) && !(ENABLED(Z_DUAL_ENDSTOPS) && Z2_USE_ENDSTOP >= _XMAX_ && Z2_USE_ENDSTOP <= _XMIN_)
 #error You must enable USE_XMIN_PLUG or USE_XMAX_PLUG
#elif DISABLED(USE_YMIN_PLUG) && DISABLED(USE_YMAX_PLUG) && !(ENABLED(Z_DUAL_ENDSTOPS) && Z2_USE_ENDSTOP >= _YMAX_ && Z2_USE_ENDSTOP <= _YMIN_)
 #error You must enable USE_YMIN_PLUG or USE_YMAX_PLUG
#elif DISABLED(USE_ZMIN_PLUG) && DISABLED(USE_ZMAX_PLUG) && !(ENABLED(Z_DUAL_ENDSTOPS) && Z2_USE_ENDSTOP >= _ZMAX_ && Z2_USE_ENDSTOP <= _ZMIN_)
 #error You must enable USE_ZMIN_PLUG or USE_ZMAX_PLUG
#elif ENABLED(Z_DUAL_ENDSTOPS) && !Z2_USE_ENDSTOP
 #error You must set Z2_USE_ENDSTOP with Z_DUAL_ENDSTOPS
#endif

* will force a minimum config file revision, otherwise Marlin will not build.
*/
#if ! defined(CONFIGURATION_H_VERSION) || CONFIGURATION_H_VERSION < REQUIRED_CONFIGURATION_H_VERSION
#error You are using an old Configuration.h file, updated it before building Marlin.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"update" here instead of "updated"

@thinkyhead
Copy link
Member

I'm still hesitant to incorporate this because I know we will get a huge raft of complaints about how hard it is to update configurations and how we don't provide any tools to make it easier. Users would prefer to just drop in their old configurations and have them work. Or, barring that, they want some helpful message to tell them what needs changing. Although this prevents the use of old configuration files (which is the safest route) it doesn't give any help with updating. We ought to compare the config options from 1.0.2-1 to 1.1.0 soon and list all the changes in the documentation someplace. Ultimately, people want to drop their old config files onto a web page and have it give them shiny new configs. We should look at that possibility too. @MagoKimbra has done some of this before.

@jbrazio
Copy link
Contributor Author

jbrazio commented Apr 30, 2016

@thinkyhead your js configuration is working with all the conf options or does it need any updating ?

@tkurbad
Copy link
Contributor

tkurbad commented Apr 30, 2016

@tinkyhead Well, what keeps us from checking the current config version A of the user vs. the one (B) needed for the state of Marlin he/she wants to compile? One could error out with all the changes necessary to transition from A to B.

The only downside of this approach would be that we would have to handle unversioned configs (which may be from yesterday or from the stone age) somehow and find the necessary diff or just hint the user to read the documentation...

Perhaps a combination of the current SanityCheck approach (for older configs) and versioned configs (from now on) would be feasible.

@thinkyhead
Copy link
Member

thinkyhead commented May 1, 2016

your js configuration is working with all the conf options or does it need any updating?

@jbrazio I wouldn't use my configurator as a starting point for a simple converter. It's still in the experimental phase and needs updates for the reformatted configuration files. It was never an "expert system" but so far built to parse the configurations and put all the settings into a structured format, capture dependencies, etc.

For an "upgrade tool" I would do things differently. First I would just scrape the configs from older versions of Marlin (1.0.2-1 for example) and capture all the configuration options in a raw form – just get their names basically, and then capture their values. (e.g., For commented-out options the value is "off.") Then I would scrape the newest configurations and get a full list of all new settings that have been added since the older config. Then from that we would make a spreadsheet with all the old and new settings and how they relate. For most they will just move over literally. But there will be a few needing code to rearrange or rename them.

From that information we could build a drag-and-drop tool to convert old configs into the new form.

@tkurbad
Copy link
Contributor

tkurbad commented May 1, 2016

Wouldn't something like repetier's online configuration generator be feasible for Marlin? I guess that would mean a lot of work, though...

@thinkyhead
Copy link
Member

@tkurbad We have a great wealth of excellent ideas.
All we lack is people with the time and expertise to implement them.

@MagoKimbra
Copy link
Contributor

Great @thinkyhead ....

@tkurbad
Copy link
Contributor

tkurbad commented May 1, 2016

@thinkyhead ... as it is with all community driven open source projects I know. sigh
I'd like to help where I can, but unfortunately, budgeted time is a problem for me, too.

@jbrazio
Copy link
Contributor Author

jbrazio commented May 3, 2016

One more for reference: #3573

@jbrazio jbrazio added Needs: Rebase S: Don't Merge Work in progress or under discussion. and removed PR: New Feature labels May 17, 2016
@thinkyhead
Copy link
Member

@jbrazio I guess this needs a rebase, there have been so many changes lately. Whenever you find the time…. Maybe it can get merged soon…?

@jbrazio
Copy link
Contributor Author

jbrazio commented Jun 8, 2016

@thinkyhead overall time this week has been short, will rebase Friday or so.

@jbrazio jbrazio removed the T: Development Makefiles, PlatformIO, Python scripts, etc. label Jun 8, 2016
@jbrazio jbrazio force-pushed the feature/config-version branch 3 times, most recently from 9a2daaf to 04bdec8 Compare June 11, 2016 23:10
@jbrazio jbrazio removed Needs: Rebase S: Don't Merge Work in progress or under discussion. labels Jun 11, 2016
@jbrazio
Copy link
Contributor Author

jbrazio commented Jun 11, 2016

@thinkyhead rebased

@thinkyhead
Copy link
Member

I guess you can see already… Failing Travis CI.

@jbrazio
Copy link
Contributor Author

jbrazio commented Jun 16, 2016

@thinkyhead It must be related with another thing, travis is green and I've forced travis to run again without issues.

https://travis-ci.org/MarlinFirmware/Marlin/builds/136990199

@thinkyhead thinkyhead merged commit f639044 into MarlinFirmware:RCBugFix Jun 16, 2016
thinkyhead added a commit to thinkyhead/Marlin that referenced this pull request Jun 16, 2016
thinkyhead added a commit that referenced this pull request Jun 17, 2016
@thinkyhead thinkyhead mentioned this pull request Jul 8, 2016
@jbrazio jbrazio deleted the feature/config-version branch July 13, 2016 19:51
CONSULitAS pushed a commit to CONSULitAS/Marlin-K8200 that referenced this pull request Aug 18, 2016
…esio UI Support)

・Add implimantation of PR MarlinFirmware#3609 to configuration files of Cartesio
・Standardize macro names
CONSULitAS pushed a commit to CONSULitAS/Marlin-K8200 that referenced this pull request Aug 18, 2016
drewmoseley pushed a commit to drewmoseley/Marlin that referenced this pull request Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants