LinuxCNC / linuxcnc

LinuxCNC controls CNC machines. It can drive milling machines, lathes, 3d printers, laser cutters, plasma cutters, robot arms, hexapods, and more.
http://linuxcnc.org/
GNU General Public License v2.0
1.81k stars 1.16k forks source link

Documentation: Deprecated ini file option(s) do not exist in new section SPINDLE_n #3171

Open Stutchbury opened 2 weeks ago

Stutchbury commented 2 weeks ago

Here are the steps I follow to reproduce the issue:

In the [DISPLAY] section of the ini file documentation the options DEFAULT_SPINDLE_SPEED and DEFAULT_SPINDLE_0_SPEED are deprecated as moved to the [SPINDLE_n]

This is what I expected to happen:

DEFAULT_VELOCITY described in the [SPINDLE_n] section of the documentation.

This is what happened instead:

There are no default spindle speed or velocity options described in the [SPINDLE_n] section

It worked properly before this:

[SPINDLE_n] is a new ini section in 2.9

Information about my hardware and software:

c-morley commented 2 weeks ago

Default_spindle_0_soeed is not deprecated and does belong in the display section. It relates to what rpm the manual spindle start button in the GUI starts at.

Stutchbury commented 2 weeks ago

Ah, it which case, the deprecated notice is in error?

Screenshot from 2024-11-03 15-09-44

I notice INCREMENT is already in the [SPINDLE_n] section - should that be back in the [DISPLAY] section as both default & increment are GUI options? (There is currently no SPINDLE_0_INCREMENT though - only a SPINDLE_INCREMENT)

hansu commented 2 weeks ago

The regarding commit from 12/2021: https://github.com/LinuxCNC/linuxcnc/commit/3d005f685d5abb7928bf37be66c946acc972636e

c-morley commented 2 weeks ago

The [SPINDLE] section is for the motion controller. That doesn't make the entries in the DISPLAY section depreciated. GUI and Motion can be different If we want to consolidate the settings then that is something that needs to be discussed and code changed.

Yes spindle increments is still a GUI setting afaicr

Stutchbury commented 2 weeks ago

I'd agree there should not be DISPLAY options in any of the motion controller ini sections (although GUIs could/should use options from TRAJ, SPINDLE etc such as max velocity where appropriate).

If this is the case, both the deprecation notices and the [SPINDLE_n] INCREMENT are incorrect in the documentation.

As an unqualified contribution to the discussion, it may be appropriate for GUIs to create their own sections in the ini file for options that are not common/universal/'official' DISPLAY options.

Sigma1912 commented 2 weeks ago

I do find it somewhat confusing to have all these [DISPLAY] entries that may or may not be used by a particular GUI. Wouldn't it make more sense to move the documentation of GUI-specific entries to the docs of that particular GUI?

hansu commented 2 weeks ago

Wouldn't it make more sense to move the documentation of GUI-specific entries to the docs of that particular GUI?

They are included in the GUI specific docs (as far as I can see with a quick view). It seems that they are listed additionally in the Ini Config page which makes it a bit difficult to sync them.

Sigma1912 commented 2 weeks ago

In that case I would suggest to remove those entries from the ini documentation and maybe add a list with links to the relevant gui docs. The current situation is really very unfortunate as it makes maintenance of the general documentation much more difficult than it needs to be.

Stutchbury commented 2 weeks ago

In that case I would suggest to remove those entries from the ini documentation and maybe add a list with links to the relevant gui docs.

By extension, options that in the ini file that are for one particular GUI should have their entries in a separate section (eg [GMOCCAPY]), reserving the [DISPLAY] section solely for a curated set of entries that are common between GUIs.

The current situation is really very unfortunate as it makes maintenance of the general documentation much more difficult than it needs to be.

Agreed - it also makes understanding the ini file more difficult because you have entries in [DISPLAY] that do nothing for some GUIs. There are multiple similarly named options to cause confusion and changing a value will often have no effect on your chosen GUI.

c-morley commented 5 days ago

reference to use for discussion and eventual inclusion: https://github.com/LinuxCNC/linuxcnc/blob/gui-reference-docs/docs/src/gui/gui-dev-reference.adoc

Sigma1912 commented 5 days ago

Thanks for that input! The document aims at harmonizing the gui related INI entries which is certainly a valid approach. I guess the underlying idea is that a user can more easily switch from one gui to another. However, personally I would have favored the opposite where the [DISPLAY] section only defines which GUI to call and move all the settings into a separate section that is uniquely used by that particular GUI. If a GUI becomes unmaintained it's much easier to remove it from the source if things are compartmentalized rather than having 'harmonized' entries that are effectively still only used by one particular gui.

Stutchbury commented 4 days ago

reference to use for discussion and eventual inclusion: https://github.com/LinuxCNC/linuxcnc/blob/gui-reference-docs/docs/src/gui/gui-dev-reference.adoc

This is excellent reference, thank you for taking the time to write it. On the SPINDLE_x vs SPINDLE_0_x there is still some ambiguity. eg in [DISPLAY] there is only a DEFAULT_SPINDLE_0_SPEED and only a SPINDLE_INCREMENT - as I'm currently finding out, spindle increment requirements are very different across speed ranges, so if we are providing the ability to have more than one spindle, we should be able to set the increment separately. Following the 80-20 rule (or more likely 99-1 rule), should we also allow just a single SPINDLE_ (ie without the '0') for all options in [DISPLAY]?

I may be in a minority but I'd also like to see the document advise developers that options defined solely for a specific GUI should have their own section. I would humbly suggest that all the 'Extended Configuration' options fall into that category.

c-morley commented 4 days ago

Thanks for that input! The document aims at harmonizing the gui related INI entries which is certainly a valid approach. I guess the underlying idea is that a user can more easily switch from one gui to another. However, personally I would have favored the opposite where the [DISPLAY] section only defines which GUI to call and move all the settings into a separate section that is uniquely used by that particular GUI. If a GUI becomes unmaintained it's much easier to remove it from the source if things are compartmentalized rather than having 'harmonized' entries that are effectively still only used by one particular gui.

Thanks for the feedback. I don't preclude the idea of another section particular to a specific GUI. The idea would be common entries go in [DISPLAY], specific into another specific section. I think just using a specific section would promote variations.

Regardless of which way we go, this just helps with discussing what should be in it.

c-morley commented 4 days ago

reference to use for discussion and eventual inclusion: https://github.com/LinuxCNC/linuxcnc/blob/gui-reference-docs/docs/src/gui/gui-dev-reference.adoc

This is excellent reference, thank you for taking the time to write it. On the SPINDLE_x vs SPINDLE_0_x there is still some ambiguity. eg in [DISPLAY] there is only a DEFAULT_SPINDLE_0_SPEED and only a SPINDLE_INCREMENT - as I'm currently finding out, spindle increment requirements are very different across speed ranges, so if we are providing the ability to have more than one spindle, we should be able to set the increment separately. Following the 80-20 rule (or more likely 99-1 rule), should we also allow just a single SPINDLE_ (ie without the '0') for all options in [DISPLAY]?

I may be in a minority but I'd also like to see the document advise developers that options defined solely for a specific GUI should have their own section. I would humbly suggest that all the 'Extended Configuration' options fall into that category.

SPINDLE_INCREMENT only has one entry basically because no one has built a screen that caters to multiple spindles. I'm not sure why you would want different increments for different spindles, but I am not opposed to it. I don't see a hardship to add 0 to the spindle entries. While it is certainly possible to use both 'DEFAULT_SPINDLE_SPEED' and 'DEFAULT_SPINDLE_0_SPEED' (Qtvcp does this now) it's a bunch of code that would be nice to remove and a variation that might confuse when comparing INI files.

I am not against a specific section for non standard GUI defines, though I feel it would be pretty easy to abuse it. The reason for adding the 'extended configuration' was because they are very common, and it would be nice that if a developer is going to add the capability, that they try to use what is already defined, or at least can be aware of a standard and discuss why they need it changed.