moggieuk / Happy-Hare

MMU software driver for Klipper (ERCF, Tradrack, Box Turtle, Night Owl, Angry Beaver, 3MS, ...)
GNU General Public License v3.0
505 stars 128 forks source link

Remove duplicate section in espooler #516

Closed wdcocq closed 1 week ago

wdcocq commented 1 week ago

The section [gcode_macro _MMU_ESPOOLER_VARS] was defined in both dc_espooler.cfg and dc_espooler_hw.cfg I presume it was some leftover from a refactor. I removed the redundant section in dc_espooler_hw.cfg

wdcocq commented 1 week ago

@ammmze If this is not the case, please let me know!

ammmze commented 1 week ago

@wdcocq Thanks for bringing this up. It was actually intentional. With the HH addons the intent (as I understand from Mr Happy Hare) is that the non-hw file (e.g. dc_espooler.cfg) should be read-only and users should make changes only to the hw files (e.g. dc_espooler_hw.cfg). So in the dc_espooler.cfg I defined all the default values and then in the dc_espooler_hw.cfg users are intended to override variables as-needed. And I chose to leave most of them commented out by default so that by defaults users would use and inherit the "managed" default values. However, as you bring this up, I suppose as long as we update the control (aka ctl) macro itself to handle default values, then I think we can remove the ones in the read-only file (dc_espooler.cfg), but leave the ones in the user managed file (dc_espooler_hw.cfg).

ammmze commented 1 week ago

To clarify my comment above about read-only and user managed files. Technically in mainsail currently the user is able to edit both files, but from what I understand, when you run the installer, the hw file will get left alone, but the non-hw file will get updated. I didn't actually verify that, so might be worth verifying. And if it does get updated, does it preserve the variables? If it does preserve them, then I think we could certainly just remove them from the hw file as you have them.

ammmze commented 1 week ago

I just verified that it does indeed preserve the values for variable_... lines on when running ./install.sh. So yea, I think we can go ahead and remove these from hw file. Thanks again for bringing this up!

moggieuk commented 1 week ago

LMK if this PR is ready to merge... just want to be sure.

Note on cfg file philosophy with HH..

wdcocq commented 1 week ago

It's because I was reworking the install script that I hit on this. as the double entry of the same section created warnings of unused parameters (though they were used in the other entry)

For addon's the *_hw.cfg is never upgraded because I don't want to overwrite the users configuration and it is often hard to parse out previous configuration because of duplicated terms (at least with the borne shell script currently used).

The issue with duplicated terms has been addressed (in my PR anyway) by parsing parameters by not just name but also by their section. So similar parameters aren't an issue anymore

Other ("read-only") cfg files follow the pattern of reading previous config into memory, transforming any changes like name change, writing the NEW cfg file substituting the in-memory values. This allows for upgrade of base file, for example to add new parameters, but retain user settings.

So this can also be applied to _hw.cfg files too if needed. and I currently do.

ammmze commented 1 week ago

LMK if this PR is ready to merge... just want to be sure.

I just reviewed the latest updates and it looks good to me!

moggieuk commented 1 week ago

I've pushed this to the v3beta branch. @wdcocq I recommend you switch to that because really "development" is my personal dev branch and often unstable:

./install.sh -b v3beta