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

Gmoccapy - added self.command.wait_complete() + self.stat.poll() #2552

Open
wants to merge 1 commit into
base: 2.9
Choose a base branch
from

Conversation

zz912
Copy link
Contributor

@zz912 zz912 commented Jul 1, 2023

Hello everybody,

I added self.command.wait complete() + self.stat.poll() to avoid race condition.

This PR will not make any change in normal use, but should help in non-standard situations and/or complicated LCNC integrations.

Here is an example of how LCNC behaves when wait_complete() is omitted:
https://forum.linuxcnc.org/gmoccapy/49335-self-command-mdi-without-self-command-wait-complete?start=10#273798

It is very important to note that not using wait complete() can cause not only a bug in the given part of the code, but can cause unpredictable behavior of the entire LCNC. In this example, the LCNC remains in AUTO mode, even though this mode is not called.

If it seems to you that I used wait_complete() or self.stat.poll() only randomly and in as many places as possible, that's not the case. For each wait_complete() or self.stat.poll() I was wondering if it is necessary and if it will cause a downstream problem.

I think you can find cases where waiting_complete() or self.stat.poll() is duplicated unnecessarily. I'm convinced that it's better to use wait_complete() or self.stat.poll() multiple times than to miss somewhere.

Gmoccapy is often mentioned in the LCNC manual as a template for developing custom GUIs or custom LCNC modules. That's why I also added wait_complete() or self.stat.poll() to every function where it was needed. If someone copies such a function into their project, it will contain wait_complete() or self.stat.poll().

I know self.stat.poll() is periodic in the function so it should still be current, but with this PullRequest I'm trying to eliminate random errors.

Zdenek

@andypugh andypugh requested a review from gmoccapy July 1, 2023 17:01
@gmoccapy
Copy link
Collaborator

gmoccapy commented Jul 3, 2023

Please do not push!

Just adding stat.poll and wait complete to nearly every place in the code is not only a very bad coding style, but als unnessasary and sure not the solution to the described situation. Additionally as the problem can not be reproduced on "normal" machines.

I will try to review the code, but will need some time.

Norbert

@zz912
Copy link
Contributor Author

zz912 commented Jul 3, 2023

Please do not push!

Sorry if I push anyone. I appreciate the work of all people from this community.
This coding style is the coding style of the desperate (me).
I retrofitted the machine to LCNC and can't finish it.
The solution would be to use the AXIS GUI, but I don't like that.
I will use these modifications on my machines for now. I just wanted to offer them to others.
It is up to you whether you use any of my suggestions or not.
You can easily throw away the entire PR. I will not mind.

Additionally as the problem can not be reproduced on "normal" machines.

I can reproduce 3 random bug on "simulation" machines.
#2453
#2448
#2489

If there are bugs in the simulation, I believe they will show up in "normal" machines as well.

This PR won't fix those bugs, but it might help.

@c-morley
Copy link
Collaborator

c-morley commented Jul 3, 2023

'Please do not push!' means Norbert does not want the changes 'pushed' to the repository. I don't think he means that you are being 'too pushy' about your continued investigation.

@gmoccapy
Copy link
Collaborator

gmoccapy commented Jul 4, 2023

I did not want to offent anybody or critisise the ongoing investment!
Just I did not want the changes to be pushed in the way it is done actauly!

I agree, that adding a wait complite in many places would be a good way to test for a changed behavior, but adding poll all around will slow the reaction of the GUI down in a unnecessary way.

Unfortunately I do not have time to check that in detail. I will meet (hope in september) with some other LinuxCNC people for a weekend. I have a list of bugs in gmoccapy I want to investigate about and solve them if possible.

Norbert

@zz912
Copy link
Contributor Author

zz912 commented Jul 4, 2023

Thank you for your response. I would blame the problem of misunderstanding on Google translator. I think you understand my idea, but you want to do the implementation differently. That's why I'm closing this PR and I'll look forward to the implementation from you when you have time for it.

It's good that Norbert has a list of bugs. Unfortunately, it's a shame that it's not public. I wouldn't have to point out the mistakes he knows about.

@zz912 zz912 closed this Jul 4, 2023
@gmoccapy
Copy link
Collaborator

gmoccapy commented Jul 5, 2023

As this is not solved, I do reopen it.
@zz912
The "bug" and feature request list is public ;-)
Just see the issues list of this git repo.

Norbert

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

Successfully merging this pull request may close these issues.

None yet

3 participants