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

Fix / Remove Races in Machine Enabling and Disabling #1414

Merged
merged 1 commit into from May 16, 2022

Conversation

markmaker
Copy link
Collaborator

@markmaker markmaker commented May 16, 2022

Description

Machine enabling/disabling has been handled outside the regular Machine task threading framework. The reason (as stated in comments in code) was that disabling should act as sort of Emergency Stop, i.e. be immediately effective, even if machine tasks are still running and/or pending in the queue. By tearing down the driver connections, any pending tasks would be indirectly killed through IO exceptions.

Not aus
(Wikimedia)

When machine enabling/disabling was later expanded to include automatic machine homing and setting/initializing Actuators automatically in the Enabled/Disabled machine state change, this resulted in race conditions, as the generated machine tasks would then overlap with the special enable/disable thread and the command sequences sent there.

This PR refactors machine enabling/disabling to work inside the regular machine task threading framework whenever possible. Only when an Emergency Stop condition is detected will it actually perform as such. In this emergency case, some of the machine state change (disabled) actuations might not work (as before). This is now reported as error messages.

Changes

  • Move most of the enable/disable functionality from the Machine Controls to the Machine object, this includes:
  • Reading the initial machine position after enabling (as a result, org.openpnp.spi.Driver.isSyncInitialLocation() had to be moved to the spi).
  • Automatic machine state change actuation.
  • Automatic homing after enabling (and therefore everything that can be triggered by homing).
  • The org.openpnp.util.UiUtils.submitUiMachineTask(Callable<Object>, Consumer<Object>, Consumer<Throwable>, boolean) had to be added, to expose the ignoreEnabled boolean to allow running the enable task in the disabled machine.
  • The unit test SampleJobTest that is using explicit machine.setEnable() and subsequent machine tasks had to be adapted.
  • Simulation machine GcodeServer connection tear-down had to be made more robust, so a machine can be re-enabled after emergency stop.

Justification

Users reported race conditions:
https://groups.google.com/g/desktop-pick-and-place/c/xrl44HcTpHQ/m/0Cn7HVAUAwAJ

Instructions for Use

When the machine is enabled, or when an idle machine is disabled, the enable/disable task is now run in a regular machine task.

Only when a busy machine is disabled, will the old method be used. In this case, the disable task is run in a separate thread and will tear down the connections, indirectly killing running/pending tasks.

Implementation Details

  1. Tested in simulation, using mutliple simulated GcodeServer connections/drivers.
  2. Did follow the coding style.
  3. The org.openpnp.spi.Driver.isSyncInitialLocation() had to be moved to the spi, and implemented in all Driver classes.
    4.Successful mvn test before submitting the Pull Request.

@markmaker markmaker merged commit 9f7a165 into openpnp:test May 16, 2022
@markmaker markmaker deleted the fix/machine-enabled-state-race branch May 16, 2022 17:32
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

1 participant