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(api): maintain speed state after collision recovery #1953

Merged
merged 2 commits into from
Jul 30, 2018

Conversation

andySigler
Copy link
Contributor

overview

Fix a bug involving pushing/popping gantry speeds, causing one the the QC scripts to move very slowly after 1 cycle. This PR avoids the issue in the QC script by explicitly setting the speed there. But then also we address the underlying problem in the driver, which simply involved replacing any calls to self.push_speed() to be instead self.push_axis_max_speed()

The bug happens in the following scenario:

try:
    driver.push_speed()  # save the current combined speed (400 mm/sec)
    driver.set_speed(10)  # set to a slower speed
    driver.move({'X': 430})  # hits the switch, forcing a home
except SmoothieError:
    driver.pop_speed()  # ERROR OCCURS HERE, b/c previously "pushed" speed was overwritten
    # the driver has now been set to the (unexpected) speed of 10 mm/sec

Here is the sequence of the events in the driver that cause this:

  1. The X axis hits its limit switch, which...
  2. forces an automatic home on the X, which...
  3. then runs driver._home_x(), which...
  4. then calls driver.push_speed() on whatever the current speed happens to be
  5. in the example above, that means that the pushed speed is now 10mm/sec

@andySigler andySigler added api Affects the `api` project easyfix Ticket is an easy fix; good for first time contributors labels Jul 27, 2018
@andySigler andySigler force-pushed the api-qc-explicit-set-speed-gantry-test branch from fa30a45 to ea4dd16 Compare July 27, 2018 18:47
@codecov
Copy link

codecov bot commented Jul 27, 2018

Codecov Report

Merging #1953 into edge will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #1953      +/-   ##
==========================================
- Coverage   32.36%   32.34%   -0.02%     
==========================================
  Files         423      423              
  Lines        6788     6801      +13     
==========================================
+ Hits         2197     2200       +3     
- Misses       4591     4601      +10
Impacted Files Coverage Δ
...tocol-designer/src/file-data/selectors/commands.js 21.51% <0%> (+0.3%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fb0725...ea4dd16. Read the comment docs.

Copy link
Contributor

@btmorr btmorr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏎

@btmorr btmorr merged commit 37c6f63 into edge Jul 30, 2018
@btmorr btmorr deleted the api-qc-explicit-set-speed-gantry-test branch July 30, 2018 14:56
b-cooper added a commit that referenced this pull request Aug 6, 2018
* feat(protocol-designer): make pipettes eagerly drop tips (#1946)

- drop tips at the end of each step unless the next step for a pipette uses change tip: "never"

Closes #1706

* fix(docker): Switch out dumb-init, add modules tools & udev config (#1952)

- Switch to Resin's default init system
- add avrdude apk & avrdude config file
- add udev rules for modules
Closes #1822

* feat(comp): Add IntervalWrapper to interaction enhancers (#1942)

* fix(protocol-designer): fix styling of pause and mix step items (#1948)

Closes #1947

* feat(protocol-designer): add help link to PD nav (#1945)

- add 'help-circle' icon
- create OutsideLinkButton component for nav
- remove negative margin from navbar .bottom
- update PD knowledge base URLs

Closes #1941

*  feat(protocol-designer): add i18next and start with tooltips (#1949)

Add the i18next library to help organize our copy, and start off by moving tooltip copy into
translations json file.

* feat(protocol-designer): refactor and restyle LabwareSelectionModal (#1929)

* LabwareSelectionModal component matches design (was AddLabwareModal)
* factor out shadows and borders to complib css vars
* fix IngredientsList; cleanup borders
* make IngredIndividual "hoverable" (presentation only)

*  fix(api): maintain speed state after collision recovery (#1953)

* fix(api): Explicitely set gantry speed after testing an axis for skipping

* removes calls to self.set_speed() from driver

* fix(api): check virtual smoothie before copying udev file on server start (#1960)

* feat(app): Add continuous polling to modules during run (#1961)

* feat(api): Add clear method to RPC SessionManager (#1969)

* feat(protocol-designer): refactor and restyle timeline terminal items (#1967)

- fix bug with `robotStateTimeline` selector creating extra "steps" in the timeline for eager drop tip, introduced in #1706 
- fix related bug where substeps didn't generate for `changeTip: "never"` steps
- fix bug where hovering over Mix highlights wells of all labware, not just the relevant labware
- renamed a handful of selectors touched by this PR to `getSpam` instead of `spam`
- create TerminalItem component
- change copy of text in initial deck setup, final deck state, and corresponding headers
- remove flask icon from initial deck setup
- remove `StepTypeWithEnd` and remove `any` typing that was a workaround for it
- remove `deckSetupMode` selector
- update all places timeline index is used to no longer add 1 to the index to skip over deck setup step (timeline warnings, timeline errors, liquids on deck at step in timeline, well selection)

Closes #1930 and closes #1974
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the `api` project easyfix Ticket is an easy fix; good for first time contributors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants