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

Separate QTVCP logger from INI parser #3243

Open
hansu opened this issue Jan 3, 2025 · 6 comments
Open

Separate QTVCP logger from INI parser #3243

hansu opened this issue Jan 3, 2025 · 6 comments
Assignees

Comments

@hansu
Copy link
Member

hansu commented Jan 3, 2025

I think it would be good to separate the QTVCP logger from Qt_Istat (INI value parsing).
The logger is used in Gmoccapy, QtVCP, Gscreen, Gladevcp ...
Currently, when using the QtVCP logger it also invokes the reading of the INI file.
This is

  1. A bit a waste of resources and slows down the start. Every use of a VCP reads the INI file which is unnecessary.
    E.g. when running the gmoccapy_with_user_tabs.ini sample config, the INI file is read four times without reason.

  2. It fills the log / console with messages that are not relevant and makes it harder to see the important messages.
    For the above example it prints (when removing the "-q" option of the EMBED_TAB_COMMAND) :

[GladeVCP-button.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No DEFAULT_LINEAR_VELOCITY Entry in DISPLAY, Using: 25 (qt_istat.py:651)
[GladeVCP-button.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No MIN_LINEAR_VELOCITY Entry in DISPLAY, Using: 0 (qt_istat.py:651)
[GladeVCP-button.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No MAX_LINEAR_VELOCITY Entry in DISPLAY, Using: 125 (qt_istat.py:651)
[GladeVCP-button.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No DEFAULT_SPINDLE_0_SPEED Entry in DISPLAY, Using: 200 (qt_istat.py:651)
[GladeVCP-button.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No MIN_SPINDLE_0_SPEED Entry in DISPLAY, Using: 100 (qt_istat.py:651)
[GladeVCP-button.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No MAX_SPINDLE_0_SPEED Entry in DISPLAY, Using: 2500 (qt_istat.py:651)
[GladeVCP-button.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No MAX_SPINDLE_0_OVERRIDE Entry in DISPLAY, Using: 1 (qt_istat.py:651)
[GladeVCP-button.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No MIN_SPINDLE_0_OVERRIDE Entry in DISPLAY, Using: 0.5 (qt_istat.py:651)
[GladeVCP-vcp_box.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No DEFAULT_LINEAR_VELOCITY Entry in DISPLAY, Using: 25 (qt_istat.py:651)
[GladeVCP-vcp_box.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No MIN_LINEAR_VELOCITY Entry in DISPLAY, Using: 0 (qt_istat.py:651)
[GladeVCP-vcp_box.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No MAX_LINEAR_VELOCITY Entry in DISPLAY, Using: 125 (qt_istat.py:651)
[GladeVCP-vcp_box.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No DEFAULT_SPINDLE_0_SPEED Entry in DISPLAY, Using: 200 (qt_istat.py:651)
[GladeVCP-vcp_box.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No MIN_SPINDLE_0_SPEED Entry in DISPLAY, Using: 100 (qt_istat.py:651)
[GladeVCP-vcp_box.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No MAX_SPINDLE_0_SPEED Entry in DISPLAY, Using: 2500 (qt_istat.py:651)
[GladeVCP-vcp_box.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No MAX_SPINDLE_0_OVERRIDE Entry in DISPLAY, Using: 1 (qt_istat.py:651)
[GladeVCP-vcp_box.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No MIN_SPINDLE_0_OVERRIDE Entry in DISPLAY, Using: 0.5 (qt_istat.py:651)
[GladeVCP-dro.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No DEFAULT_LINEAR_VELOCITY Entry in DISPLAY, Using: 25 (qt_istat.py:651)
[GladeVCP-dro.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No MIN_LINEAR_VELOCITY Entry in DISPLAY, Using: 0 (qt_istat.py:651)
[GladeVCP-dro.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No MAX_LINEAR_VELOCITY Entry in DISPLAY, Using: 125 (qt_istat.py:651)
[GladeVCP-dro.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No DEFAULT_SPINDLE_0_SPEED Entry in DISPLAY, Using: 200 (qt_istat.py:651)
[GladeVCP-dro.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No MIN_SPINDLE_0_SPEED Entry in DISPLAY, Using: 100 (qt_istat.py:651)
[GladeVCP-dro.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No MAX_SPINDLE_0_SPEED Entry in DISPLAY, Using: 2500 (qt_istat.py:651)
[GladeVCP-dro.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No MAX_SPINDLE_0_OVERRIDE Entry in DISPLAY, Using: 1 (qt_istat.py:651)
[GladeVCP-dro.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No MIN_SPINDLE_0_OVERRIDE Entry in DISPLAY, Using: 0.5 (qt_istat.py:651)

@c-morley It would be great if you could implement that (I guess only a little thing for you).

@c-morley
Copy link
Collaborator

c-morley commented Jan 4, 2025

The Logger does not pull in the Info library, the Info library is imported later in the gladevcp.py file.
I disagree about completely removing the INI parsing.

  1. Some advanced VCPs require INI info - it would be nice to have standard information available.
  2. If the VCP is included with a screen, then the errors are relevant - One needs to fix the warnings in the INI. Obviously when we consolidate the INI entries then it is more relevant.

Once we agree on common INI entries this problem practically goes away.
Having both (all?) GUIs use the same (standard) INI Info and Status messages makes everything easier.

Maybe a better half way fix is to have the Info library check for gmoccpy relevant entries and convert them to Info ones. But I think it would be best in the long run if we just decide on the best entries and both use them.

@hansu
Copy link
Member Author

hansu commented Jan 4, 2025

The Logger does not pull in the Info library, the Info library is imported later in the gladevcp.py file.

Ok sorry, my fault.

1. Some advanced VCPs require INI info - it would be nice to have standard information available.

Can you give an example of one?

2. If the VCP is included with a screen, then the errors are relevant - One needs to fix the warnings in the INI. Obviously when we consolidate the INI entries then it is more relevant.

Yeah but isn't it the duty of the GUI to check if there are all required information available in the INI?
The VCP may need only a subset of them.

Once we agree on common INI entries this problem practically goes away. Having both (all?) GUIs use the same (standard) INI Info and Status messages makes everything easier.

Yes maybe the warnings disappear mostly but still a performance issue...

@c-morley
Copy link
Collaborator

c-morley commented Jan 5, 2025

  1. A GUI built with gladevcp. A VCP that needs to know about homing state/no-home-required. Is linuxcnc running? What macros are available, Is the machine metric .... The VCP can be as simple or complex as anybody wants to make it. Pretty much anything beyond simple widget displaying needs info from the INI.
    Having a standard interface for important INI and Status information just makes thing easier for new users.

  2. Well how are we to know what info ones needs? Gladevcp/Qtvcp is for general use and INI info is often required. If one fixes the warnings then all the noise stops (the point of the warnings) . If one sets the logging to -q on the vcp because one doesn't care then the noise stops or just ignore the warnings. Most users don't see the terminal anyways.

What is the performance hit if you start with -q verses with warnings printed?

I guess I don't really understand why you wouldn't just fix the INI to stop the warnings if it bothers you.
(obviously I understand right now where the GUIs don't quite align)

Now if you are talking about a VCP that stands alone, with no linuxcnc running, it already sets the logging to critical.

@hansu
Copy link
Member Author

hansu commented Jan 5, 2025

  1. But if you have a VCP that does a bit more, you would have an underlaying python script, right? And that should read the INI if needed IMHO.
    How was this accomplished before you introduced qt_istat to gladevcp?

What is the performance hit if you start with -q verses with warnings printed?

The INI file is still being read several times. Sure not a big thing but if nobody cares about code efficiency...

@rmu75
Copy link
Contributor

rmu75 commented Jan 10, 2025

The INI file is still being read several times. Sure not a big thing but if nobody cares about code efficiency...

Parsing ini files shouldn't impact startup time. What may be noticeable is writing log messages, a proper logging framework does buffer flush and fsync after each log entry, and that can be expensive.

@c-morley
Copy link
Collaborator

  1. INI could be read from handler code for advanced VCPs - yes. In Qtvcp initialization sequence is important. Maybe less so in glafevcp as it (so far) uses less interwoven libraries. If someone decides to use Info/Status library in a widget (IMHO should) then then Info/Status need to be initialized in gladevcp main code (where they are now)

  2. it's being read several times (in your example) because you have 4 separate instances of gladevcp. Better optimization would be to integrate the panels into the main screen. (Qtvcp does this for Qtvcp VCP panels in Qtvcp screens)

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

3 participants