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

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

Open
1 task
Stutchbury opened this issue Nov 3, 2024 · 14 comments
Open
1 task

Comments

@Stutchbury
Copy link

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:

  • I am using Debian 12 bookworm:
  • I am using this kernel version: 6.1.0-26-rt-amd64
  • I am running ...
    • A binary version from linuxcnc.org (including buildbot.linuxcnc.org)
  • I am using this LinuxCNC version: 2.9.3-2099-g9f5a1c8f50
  • I am using this user interface (GUI): qtdragon, gmoccapy
  • I am using this interface hardware vendor and chipset: various
@c-morley
Copy link
Collaborator

c-morley commented Nov 3, 2024

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
Copy link
Author

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
Copy link
Member

hansu commented Nov 3, 2024

The regarding commit from 12/2021: 3d005f6

@c-morley
Copy link
Collaborator

c-morley commented Nov 3, 2024

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
Copy link
Author

Stutchbury commented Nov 3, 2024

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
Copy link
Contributor

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
Copy link
Member

hansu commented Nov 6, 2024

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Collaborator

c-morley commented Nov 17, 2024

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.

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

No branches or pull requests

4 participants