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

[BUG] mc-apply script doesn't function #27612

Closed
1 task done
CRCinAU opened this issue Dec 29, 2024 · 14 comments · Fixed by #27628
Closed
1 task done

[BUG] mc-apply script doesn't function #27612

CRCinAU opened this issue Dec 29, 2024 · 14 comments · Fixed by #27628

Comments

@CRCinAU
Copy link
Contributor

CRCinAU commented Dec 29, 2024

Did you test the latest bugfix-2.1.x code?

Yes, and the problem still exists.

Bug Description

If CONFIGURATION_EMBEDDING is enabled, the M503 C command will write MC.ZIP to the printers SD card with the compile time settings used.

According to docs/ConfigEmbedding.md, the following should be used to then apply that config template back to the .h files:

$ git checkout -f
$ unzip mc.zip
$ python buildroot/share/PlatformIO/scripts/mc-apply.py

When attempting this, the script always returns:

$ python3 ../buildroot/share/PlatformIO/scripts/mc-apply.py 
No marlin_config.json found.

Removing the main try / except block, I get:

$ python3 buildroot/share/PlatformIO/scripts/mc-apply.py 
Traceback (most recent call last):
  File "/home/netwiz/git/Marlin/buildroot/share/PlatformIO/scripts/mc-apply.py", line 22, in <module>
    for k, v in sorted(conf[key].items()):
                       ^^^^^^^^^^^^^^^
AttributeError: 'str' object has no attribute 'items'

When adding a debug to see what key contains, I see:

$ python3 buildroot/share/PlatformIO/scripts/mc-apply.py 
Key: Z_SAFE_HOMING_X_POINT
Traceback (most recent call last):
  File "/home/netwiz/git/Marlin/buildroot/share/PlatformIO/scripts/mc-apply.py", line 23, in <module>
    for k, v in sorted(conf[key].items()):
                       ^^^^^^^^^^^^^^^
AttributeError: 'str' object has no attribute 'items'

As a result, no configuration files are modified and the marlin_config.json file cannot be folded back into the configuration.

Bug Timeline

Likely a while.

Expected behavior

The script should restore config settings from marlin_config.json to the related .h files.

Actual behavior

Error in output.

Steps to Reproduce

See main description.

Version of Marlin Firmware

bugfix-2.1.x

Printer model

No response

Electronics

No response

LCD/Controller

No response

Other add-ons

No response

Bed Leveling

None

Your Slicer

None

Host Software

None

Don't forget to include

  • A ZIP file containing your Configuration.h and Configuration_adv.h.

Additional information & file uploads

Added example marlin_config.json as per output of M503 C.
marlin_config.json

@ellensp
Copy link
Contributor

ellensp commented Dec 29, 2024

in older marlin CONFIG_EXPORT 1 the file marlin_config.json used to start with

'{"__INITIAL_HASH":"6c2dd0c6a928862c1c0c","Configuration.h":{"VALIDATE_HOMING_ENDSTOPS" 

and the script got Configuration.h from here.

now CONFIG_EXPORT 1 (marlin_config.json) starts with

{"Z_SAFE_HOMING_X_POINT":"X_CENTER","VALIDATE_HOMING_ENDSTOPS":"","Y_DRIVER_TYPE":"TMC2209","PREHEAT_1_LABEL":"\"PLA\"","EXTRUDE_MAXLENGTH":"600","MPC_STEADYSTATE":"0.5f","TEMP_WINDOW":"1","E

CONFIG_EXPORT 101 adds the filenames back in.

@ellensp
Copy link
Contributor

ellensp commented Dec 29, 2024

first attempt removed...

@CRCinAU
Copy link
Contributor Author

CRCinAU commented Dec 29, 2024

Yeah, there's still a few cases that this doesn't cover (did it ever?)

  1. Configuration_adv.h as noted. Maybe its just better to load both into different string variables and search both for key?

Or am I misunderstanding and no values from Configuration_adv.h are actually making it into the embedded config? If this is the case, then we probably should look at fixing that too.

  1. Values that are added but are not mentioned in any .h file present

There are a bunch of opt_* files in buildroot/bin/ that seem to basically be the shell script equivalents to what mc-apply.py would probably need to have for proper functionality - however these are obviously designed to be run individually on a per config option basis.

@ellensp
Copy link
Contributor

ellensp commented Dec 29, 2024

--opt creates Configuration.h.sh which contains

e.g.,

opt_enable ARC_SUPPORT
opt_enable AUTOTEMP
opt_set AUTOTEMP_FACTOR 0.1f
opt_set AUTOTEMP_MAX 250
opt_set AUTOTEMP_MIN 210
opt_set AUTOTEMP_OLDWEIGHT 0.98

etc.

@ellensp
Copy link
Contributor

ellensp commented Dec 29, 2024

I see a light at the end of the tunnel...

In Configuration_adv.h is

 *  1 = marlin_config.json - Dictionary containing the configuration.
 *      This file is also generated for CONFIGURATION_EMBEDDING.
 *  2 = config.ini - File format for PlatformIO preprocessing.
 *  3 = schema.json - The entire configuration schema. (13 = pattern groups)
 *  4 = schema.yml - The entire configuration schema.
 *  5 = Config.h - Minimal configuration by popular demand.
 */
#define CONFIG_EXPORT 105 // :[1:'JSON', 2:'config.ini', 3:'schema.json', 4:'schema.yml', 5:'Config.h']

but the value is > 5
So what gives there ?

The answer is

    # Get the CONFIG_EXPORT value and do an extended dump if > 100
    # For example, CONFIG_EXPORT 102 will make a 'config.ini' with a [config:] group for each schema @section

101 processes both Configuration.h and Configuration_adv.h and puts the file name in as expected
(and Version.h, but thats a little broken)

This is a red herring, we are only looking at type 1

@Ferrograph
Copy link

I tried the new scrip above but it still outputs not found.

PS D:\Working\PROJECTS\Marlin\Code> python buildroot/share/PlatformIO/scripts/mc-apply.py 
Error: marlin_config.json not found.
PS D:\Working\PROJECTS\Marlin\Code> 

@ellensp
Copy link
Contributor

ellensp commented Dec 31, 2024

it expects marlin_config.json in the directory you invoke the script from ie in this case
D:\Working\PROJECTS\Marlin\Code\marlin_config.json

But its not complete yet...

@Ferrograph
Copy link

Ferrograph commented Dec 31, 2024

Ok, Ive tried it in there (the root folder with configs h, and /src. I'll wait for an update.

Thanks for all your hard work.

@ellensp
Copy link
Contributor

ellensp commented Jan 7, 2025

Moved code updates to pull request #27628

Note mc-apply.py only works with CONFIG_EXPORT set to 1, as originally intended.

I've tested this lots, apart from some minor formatting issues, which i'm not concerned about. This now seems to work as expected

Note any extra defines that are not in the stock configs are appended to the end of Configuration.h

Please give this a try.

@CRCinAU
Copy link
Contributor Author

CRCinAU commented Jan 7, 2025

I'm curious - does CONFIG_EXPORT 1 also store settings from Configuration_adv.h?

I was under the impression that without the extended dump (ie 101), it would only do Configuration.h values?

Of course, I could be greatly misunderstanding...

@ellensp
Copy link
Contributor

ellensp commented Jan 7, 2025

yes 1 contains both set of data.
101 splits the defines into containers for configure file and section headers
1 everything is now in a mostly flat json file. (the original version had containers for each config file)

@thisiskeithb thisiskeithb linked a pull request Jan 7, 2025 that will close this issue
@ellensp
Copy link
Contributor

ellensp commented Jan 8, 2025

So thinkyhead goes in the opposite direction, changes marlin so CONFIG_EXPORT 1 has Configuration file containers...

@CRCinAU
Copy link
Contributor Author

CRCinAU commented Jan 8, 2025

I'm confused now - the PR for updating mc-apply.py was merged, but also other changes?

Does that mean that mc-apply.py needs more changes to work now? Or should it now 'just work'?

@Ferrograph
Copy link

Im also confused is this fixed with updated documentation or not?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants